search

dj-stripe / dj-stripe

dj-stripe automatically syncs your Stripe Data to your local database as pre-implemented Django Models allowing you to use the Django ORM, in your code, to work with the data making it easier and faster.
https://dj-stripe.dev
MIT License
1.56k stars 476 forks source link

Support Multiplan Subscriptions #417

Closed kavdev closed 4 years ago

kavdev commented 7 years ago

https://stripe.com/blog/multiplan-subscriptions

twidi commented 7 years ago

I was in the process of creating this issue... then I checked and found this one.

As stated on the stripe documentation a subscription that can handle more than one plans has a big advantage: only one billing cycle, ie less fees.

See https://stripe.com/docs/subscriptions/multiplan (multiple plans on one subscription) And https://stripe.com/docs/subscriptions/multiple (multiple subscriptions)

danryu commented 5 years ago

Just realised that having multiplan is probably a dealbreaker for me. With 1.1.0 now released, is there any likely movement on this coming up? Hoping it's gonna be a fairly easy change ...

jleclanche commented 5 years ago

It's not easy for me to test. If you want to work on it, I can give some guidance.

danryu commented 5 years ago

Ok, I'm pretty sure I need it, because my service wants to offer multiple instances of the same product plan, each of them metered per-unit time and then billed once monthly. (Much like cloud server provisioning eg Digital Ocean etc). I think this scenario necessitates multi-plan support, if it's to be done cleanly. Was just taking a look at the models.Subscription definition now. Feel free to go ahead with the advice, I'll see what I can do.

jleclanche commented 5 years ago

Without having looked at it, I'm guessing that:

Let me know if you have questions on any of this. I'm also on Gitter.

danryu commented 5 years ago

Ok. This might take me a while. Both because of limited time and also because of my still-not-great understanding of the code-base in general. The migration and test bits especially. I'll take another look and join you on Gitter.

danryu commented 5 years ago

I've taken another little look. Re the plans m2m - I assume we want to mirror the Stripe API and have an items attribute? As at https://stripe.com/docs/api#subscription_object-items. RE removing the plan foreign key - as the Stripe subscription object contains both an items[plan1,plan2] list, and a singular plan element, I'm not sure how to handle this.

danryu commented 5 years ago

I have committed a couple of changes towards getting multiplan working to my fork here: https://github.com/dj-stripe/dj-stripe/compare/master...danryu:master

No PR yet. If @jleclanche or @kavdev or somebody can take a look and tell me if I'm doing anything insane, that would be helpful. I haven't tested yet (or got any tests or migrations going) - I just want to get an idea if I'm doing the right thing or not. I'm basically following the InvoiceItem model for SubscriptionItem. I've replaced the plan foreignkey as advised.

jleclanche commented 5 years ago

Ok yeah sounds like SubscriptionItem needs to be implemented.

You don't need to reimplement the fields that are already defined in StripeObject, which includes metadata, created etc.

The rest looks good I think. Try it out, if it works just PR it, ping me and ill review.

jleclanche commented 5 years ago

I also don't think you should remove the plan foreign key after all, also. Does that key still exist if you have multiple plans?

danryu commented 5 years ago

Yeah I was gonna ask about the plan FK too - I'm guessing that it can still exist with multiple plans. But I haven't tried that and don't have a use case for it. For my part, I just want to add/remove multiple items (of the same Plan in fact) to the same subscription.

jleclanche commented 5 years ago

Let's leave it in for now.

danryu commented 5 years ago

Ha, turns out that Stripe doesn't actually allow multiple SubscriptionItems of the same Plan ... wow.

"message": "Cannot add multiple subscription items with the same plan: plan_foo",

Nothing in their docs suggested that. Shame, that was my specific use case - still got an open support call with them. So I'm gonna have to do my own usage-metering and then use the invoice.created webhook to return an InvoiceItem which I calculate my side. Didn't expect that. Anyway, it can still be useful to people I guess. I'll still submit the PR once I have a confirmed test fail that somebody else can put their eyes on.

m-vdb commented 5 years ago

Arf, almost a deal breaker for me too :( Do you guys need help on this issue? Are we far from a patch?

danryu commented 5 years ago

Hi @m-vdb - sorry, been away on holiday. Answer is, I don't think the patch is far away. @jleclanche previously suggested I do a PR to get some more eyes on it (even though I haven't updated tests), and because I can't use multiplan anyway now :( - I had to de-prioritise my multiplan attempt. Please take a look at the PR and maybe have a chat with @jleclanche on Discord.

m-vdb commented 5 years ago

Ok thanks, I will try to take a look!

jleclanche commented 5 years ago

Folks I added SubscriptionItem in master. Can someone try it out?

danryu commented 5 years ago

@m-vdb I suggest you go ahead and pull master and see if it works for you. It may well work out of the box ;-)

therefromhere commented 5 years ago

@jleclanche I've just had a go at using this (master at 301923de2085abd8960590551076765c8ff882f1), webhooks fail in Subscription._attach_objects_hook(), because Subscription._stripe_object_to_plan() doesn't support multi-item - data["plan"] is null. So I think we'll need to make Subscription.plan nullable.

def _attach_objects_hook(self, cls, data):
    self.customer = cls._stripe_object_to_customer(target_cls=Customer, data=data)
    self.plan = cls._stripe_object_to_plan(target_cls=Plan, data=data)

Example invoice_created events with multi and single plan: https://gist.github.com/therefromhere/60b295798ed1a5ef35bbf268210cb526

account is on latest stripe API version - 2018-11-08

therefromhere commented 5 years ago

In case anyone else wants to try this out, I've enabled sync of SubscriptionItem in #784, which should close this issue.

One thing to note: this will mean that SubscriptionItem is created even for regular single-plan Subscriptions, but that makes sense I think, and matches Stripe's API structure.

acamposAccelize commented 5 years ago

hi @therefromhere , again me ... I take the last version of master and this feature ( multiplan for subscriptions) is not working and generate webhook errors in case multiplan present on stripe.

Model is not with null allowed and the of the flow is not working either. I have done those modifications on my side ( manly based on the previous works share here).

If I'm not wrong, I should :

Agree?

therefromhere commented 5 years ago

@acamposAccelize Yes, if you're hitting an error with this with the latest version of master ( 46594993ebdf5d23141e96f84041ffc1f8a27f49 ) please add a pull request with what you think needs to change with a test.

Please note that I merged #784 into master 2 days ago - are you running the latest version?

acamposAccelize commented 5 years ago

Good point, I have missed the last version ( clone just before the commit :-( . Everything works as expected with the last commit. Sorry to have disturbed you. Alex

therefromhere commented 4 years ago

Closing this since SubscriptionItem sync was released in 2.0.0.