-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add delete/1 to Subscription
#162
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
base: master
Are you sure you want to change the base?
Conversation
klacointe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No unit test ?
Thanks for the warning @klacointe. I added the tests for the |
| Chargebeex.HTTPClientMock, | ||
| :get, | ||
| fn url, body, headers -> | ||
| assert url == "https://test-namespace.chargebee.com/api/v2/subscriptions/1234" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to module variable @url ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are testing different endpoints in this module:
/v2/subscriptions;/v2/subscriptions/foobar;/v2/subscriptions/foobar/delete;/v2/customers/foobar/subscription_for_items;/v2/subscriptions/foobar/update_for_items.
Should we create one module attribute for each?
If we decide to use module attributes for these values I guess we should also refactor the other test modules in the project in order to have the same pattern, WDYT @klacointe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this would impact multiple test modules, IMO this refactoring is outside the scope of this PR. I think it should be done in a separate PR, WDYT @klacointe ?
| :get, | ||
| fn url, body, headers -> | ||
| assert url == "https://test-namespace.chargebee.com/api/v2/subscriptions/1234" | ||
| assert headers == [{"Authorization", "Basic dGVzdF9jaGFyZ2VlYmVlX2FwaV9rZXk6"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to module variable @headers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have [{"Authorization", "Basic dGVzdF9jaGFyZ2VlYmVlX2FwaV9rZXk6"}] headers for GET requests and
[{"Authorization", "Basic dGVzdF9jaGFyZ2VlYmVlX2FwaV9rZXk6"}, {"Content-Type", "application/x-www-form-urlencoded"}] for POST requests. Should we create a @get_headers and a @post_headers module attributes? If so IMO we should refactor the other test modules in order to have the same pattern, WDYT @klacointe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes ! 😄
|
@klacointe Any news on this PR? |
Adds support for Chargebee's delete_a_subscription endpoint;