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

Added support for subaccounts #129

Merged
merged 5 commits into from
Apr 1, 2016
Merged

Conversation

coldacid
Copy link
Contributor

Adds a subaccounts library roughly matching the functionality and design of the templates library.

Closes #128

@aydrian aydrian added this to the 1.3.0 Release milestone Mar 31, 2016
@aydrian
Copy link
Contributor

aydrian commented Mar 31, 2016

Thank you for the PR. Just in time for the 1.3 release slated for tomorrow. We'll get this reviewed ASAP.

@aydrian
Copy link
Contributor

aydrian commented Mar 31, 2016

So it looks good, but we are changes the way we are doing some things. Just put everything you need to for create and update in options, no need to create options.subaccount. We'll be changing this for the other libraries in 2.0.0. Might as well start now for this resource. You can see the same in Relay Webhooks. Also, you can remove options for find and have it take just the subaccount_id.
We are also working to remove the toApiFormat function. See how we are doing it with Relay Webhooks. We have found that one function does not rule them all.

@coldacid
Copy link
Contributor Author

Alright, I was able to get this done today after all. The subaccount library now matches the new API style as presented by the Relay Webhooks library.

JSHint complains and dies because the validation for options brings the
cyclomatic complexity of subaccounts.create to 6, where the settings say
the maximum should be 5. Refactored the validation of options into its
own function, but if there ever should be more required properties, then
we'll be right back here again.
@@ -0,0 +1,165 @@
var chai = require('chai')

Choose a reason for hiding this comment

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

This file is misspelled. Must be subaccounts.spec.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn, nice catch. I didn't even notice that. I blame our product. Will fix in a couple minutes.

* `options.keyLabel` - user-friendly identifier for subaccount API key **required**
* `options.keyGrants` - list of grants to give the subaccount API key **required**
* `options.keyValidIps` - list of IPs the subaccount may be used from
* `options.ipPool` - id of the default IP pool assigned to subaccount's transmissions

Choose a reason for hiding this comment

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

this can be set but has no effect yet. This feature is coming soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still documented for the endpoint, therefore included.

@bizob2828
Copy link

Thanks again for the contribution.

@bizob2828 bizob2828 merged commit 10f6bbb into SparkPost:master Apr 1, 2016
@coldacid coldacid deleted the ISSUE-128 branch April 1, 2016 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants