Conversation
astex
changed the title
feliixx
left a comment
feliixx
left a comment
There was a problem hiding this comment.
Hi @astex,
thank you for the PR, it does look like a useful addition !
Before diving into the code, can you please:
- make sure that the PR doesn't break the existing tests. (Currently the PR breaks some subscription related tests. To make sure that the tests pass, start a Localstripe instance and run
./test.sh, and all tests should pass) - add some new tests to cover this code, so we don't break it by accident in the future (you can mimic a few of the existing ones that are already presents for
Plansif you want) - fix the minor issues noticed by the linter (ie,
flake8 .should not raise any error)
(sorry for the extra work 😅)
Thanks for the response. I wasn't sure how to run tests locally. I'm more used to setups like Ditto flake8. I've used it extensively, but wasn't sure if your repo did so. How would you feel about a PR to make it run automatically pre-commit? |
Hello @astex, good idea to set this locally on your computer, but no need to commit that (anyone can run linters in their preferred way). Let's keep the repo minimalist and easily understandable. |
|
Alright. Tests ran locally. This should be ready for review. |
Prices is a replacement for the legacy Plans API from Stripe. From their documentation: > You can now model subscriptions more flexibly using the Prices API. It > replaces the Plans API and is backwards compatible to simplify your > migration. This PR implements a partial version of the Prices API containing only the fields currently required.
feliixx
left a comment
feliixx
left a comment
There was a problem hiding this comment.
Thanks for fixing the tests and adding new ones !
I'm not familiar with the price API (we don't use it yet), so I only have a few minor comments.
Once they are fixed, can you also please squash all commits into a single one ?
(@adrienverge @H--o-l : I ran our company internal test suit with this PR and it works fine)
| assert type(plan) is str | ||
| if plan is None: | ||
| assert isinstance(price, str) | ||
| assert plan is None |
There was a problem hiding this comment.
I guess you meant
| assert plan is None | |
| assert price is None |
?
and why not adding a similar assert in the elif price is None branch ?
| customer_obj = Customer._api_retrieve(customer) | ||
| if subscription_items: | ||
| for si in subscription_items: | ||
| Plan._api_retrieve(si['plan']) # to return 404 if not existant |
There was a problem hiding this comment.
Please keep the existing comments, here and in the rest of the file (there are two other place where this comment was removed in the rest of the file)
Also, can you add this # to return 404 if not existant to the Price._api_retrieve(si['price']) lines you added ? it looks like you added the comment in some places but not always
| assert isinstance(item['price'], str) | ||
| assert 'plan' not in item | ||
| elif 'plan' in item: | ||
| assert isinstance(item['plan'], str) |
There was a problem hiding this comment.
Why not adding a similar assert 'price' not in item here ?
|
@astex hello! Could you fix PR comments please? Really need your changes |
4 participants
Prices is a replacement for the legacy Plans API from Stripe. From their documentation:
This resolves #160.