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

Support trialing subscriptions #139

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

adrianlungu
Copy link

Fix for #131

  • Supports trialing status for subscription
  • Still creates an invoice for a trialing subscription with 0 value

@adrianlungu
Copy link
Author

Hmm, I'm not sure what's failing 🤔

Log of the CI before this commit:
```
$ if [[ $TRAVIS_PYTHON_VERSION != nightly ]]; then flake8 .; fi
./localstripe/resources.py:594:47: E741 ambiguous variable name 'l'
The command "if [[ $TRAVIS_PYTHON_VERSION != nightly ]]; then flake8 .; fi"
exited with 1.
```
Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

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

Hmm, I'm not sure what's failing

You can see what's wrong by clicking on the Travis build. I recommend rebasing on latest master.

@@ -932,6 +932,9 @@ def __init__(self, customer=None, subscription=None, metadata=None,
else:
self.currency = 'eur' # arbitrary default

if subscription is not None and subscription_obj.status == "trialing":
self.lines = List()
Copy link
Owner

Choose a reason for hiding this comment

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

  • It's hard to understand these lines. Are you just resetting self.lines? In this case, it would be better to avoid adding items in it directly?
  • Using a List() without arguments will drop the '/v1/invoices/' + self.id + '/lines'.

H--o-l and others added 16 commits June 2, 2020 11:08
Since commit 4810d4b _coupon: use float value for percent_off_ the following
unit test is failing:
```
...
+ curl -sSf -u sk_test_12345: http://localhost:8420/v1/coupons -d id=PARRAIN
  -d percent_off=30 -d duration=once
curl: (22) The requested URL returned error: 400 Bad Request
```

It's because, in the code just after 4810d4b change, the expected type for
`percent_off` is still `int`.
In Stripe [documentation of the Charges list API], all parameters are optional.
Since commit 54831f9 _Charge: Add `customer` and `created` to the list API_,
localstripe support parameters `customer` and `created` but both were made
mandatory.
Let's fix them.

[documentation of the Charges list API]: https://stripe.com/docs/api/charges/list
Let's add the property `billing_reason` that was missing. For the
moment, it's not implemented so it's always `null`.

https://stripe.com/docs/api/invoices/object#invoice_object-billing_reason
Problem: very recently, Travis started to fail on commit that were
previously fine. Probably a change on their side?
It appears that on some Python version (e.g. 3.7 but not 3.8) they
double-escape `%5B` and `%5D` caracters, so they are passed as `%255B`
and `%255D`.

To avoid that, I propose to use `[` and `]` directly, and enable curl's
`-g` option (`--globoff`) so that it doesn't interpret them.

In summary now the commands are a bit clearer:

    curl -sSfg -u $SK: $HOST/v1/plans?expand[]=data.product
And drop support on Python 3.5 and 3.6.
The current implementation supports the Balance resource as
described in https://stripe.com/docs/api/balance
`None` is already the implicit default when using `get()` on a dict.
@H--o-l
Copy link
Collaborator

H--o-l commented Jan 28, 2021

Plop, just passing by to say that I think you re-pushed your branch after a merge with the master branch.
You should not do that, because the git history, and the GitHub UI, become un-readable.
You should "rebase" your branch on top of master, not "merge" master into your branch.

@adrianlungu
Copy link
Author

Yeah, I'm gonna redo this at some point as it's a bit jumbled up at some points and it's still not working properly. Thanks for the heads up!

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.

7 participants