Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds an account settings management helper object #70

Merged
merged 11 commits into from
Apr 17, 2017

Conversation

kernkw
Copy link
Contributor

@kernkw kernkw commented Jul 5, 2016

This PR should simplify the ability to retrieve or update account settings with just the name of the setting, instead of needing to know if a setting is a tracking, mail, user, or partner. This PR also pulls in rspec and faker gem to increase test thoroughness.

See example or readme for example.

Let me know your thoughts on this, I also have plans to add wrappers around other account management endpoints, such as subuser creation and management, stats, etc

README.md Outdated
@@ -81,6 +81,27 @@ puts response.status_code
puts response.body
puts response.headers
```
## Manage Account Settings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in the README in the helper file, see the mail helper for an example: https://github.com/sendgrid/sendgrid-ruby/tree/master/lib/sendgrid/helpers/mail. Then you can link to it from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool makes sense, any preference on how I link to it from the main README? I didnt see a link to the mail helper there currently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thinkingserious
Copy link
Contributor

Very nice @kernkw!

We just need some things to be moved around, please see the comments. Generally, please follow the conventions that the mail helper uses. Thanks!

@kernkw
Copy link
Contributor Author

kernkw commented Jul 7, 2016

@thinkingserious it looks like this build is failing because its not exporting the env variables from the repo settings, my guess is that this because my pr is coming from a fork so when it builds the pr its not picking up those variables, i could be wrong but thats a theory

This line is missing from my PR build and thus all of the tests are failing with 403 status code

Setting environment variables from repository settings
$ export MOCK_HOST=[secure]

@thinkingserious
Copy link
Contributor

@kernkw Correct, that is a known issue that will be fixed soon; meanwhile, you can test locally like this: #72 (comment) Thanks!

@kernkw
Copy link
Contributor Author

kernkw commented Jul 11, 2016

@thinkingserious i am getting some test errors running the prism server, followed instructions and looked at the prism json i am not sure why the error is occurring, i pryed into the code to see what api call was failing.

Basically i looks like prism is not reading integer params as integers but is receiving them as strings.
I tested in sendgrid prod account and this call passes so i assume its something due to prism endpoints.

Not sure what i am doing differently from you, i downloaded latest prism and pulled a fresh master branch from sendgrid-ruby

prism run --mock --list --spec https://api.stoplight.io/v1/versions/563a5309daad691100fb05af/export/stoplight.json

curl http://localhost:4010/v3/geo/stats\?end_date\=2016-04-01\&country\=US\&aggregated_by\=day\&start_date\=2016-01-01\&limit\=1\&offset\=1
{
    "errors": [
        {
            "context": "request.url.queryString",
            "description": "Invalid type. Expected: [array,integer], given: string",
            "field": "limit",
            "location": "limit",
            "type": "invalid_type"
        },
        {
            "context": "request.url.queryString",
            "description": "Invalid type. Expected: [array,integer], given: string",
            "field": "offset",
            "location": "offset",
            "type": "invalid_type"
        }
    ]
}

but when i remove the limit and offset param it works

curl http://localhost:4010/v3/geo/stats\?end_date\=2016-04-01\&country\=US\&aggregated_by\=day\&start_date\=2016-01-01
[
  {
    "date": "2015-10-11",
    "stats": [
      {
        "metrics": {
          "clicks": 0,
          "opens": 0,
          "unique_clicks": 0,
          "unique_opens": 0
        },
        "name": "TX",
        "type": "province"
      }
    ]
  },

@thinkingserious
Copy link
Contributor

@kernkw,

I just tested your branch on my machine and that test passed.

Can you please try these steps?

#74 (comment)

Thanks!

@kernkw
Copy link
Contributor Author

kernkw commented Jul 11, 2016

@thinkingserious ok cool it works thanks, i figured their website would download the latest version of prism but they have you download v0.1.2 so must have fixed the issue with their latest.

I am gonna add rspec tests to run in the rake task, so travis CI can pick those up, sound good?

@thinkingserious
Copy link
Contributor

Sounds good, thanks!

@thinkingserious
Copy link
Contributor

Hi @kernkw,

This one finally rose up to the top of the queue. Could you please fix the conflicts so that I may test and merge? Thanks!

@SendGridDX
Copy link

SendGridDX commented Apr 13, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@thinkingserious thinkingserious merged commit 1210708 into sendgrid:master Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants