-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix(subscriptions): on upgrade use correct invoice line #12702
Conversation
7fc3913
to
d88969f
Compare
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.
Thanks Reino for digging into this. Your solution will definitely work, though I think we may want to throw an error if we don't find a type
of subscription
on any of the invoice.line_item
objects?
As written, the current fallback is to use the period.end
for an invoiceitem
, which is the bug in this case.
Because: * The incorrect next invoice date is shown in the subsequent invoice email * For a subscription upgrade, the Stripe invoice can have a line item for the previous and current subscription. The existing logic assumes there is only one invoice line item. This commit: * Use the invoice line item with type "subscription" to determine the correct next invoice date. Closes #12690
d88969f
to
44c8823
Compare
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.
LGTM Reino thanks; one non-blocking suggestion for you based on other tests where we make assertions about errors.
@StaberindeZA Ouch, this is going to be a problem elsewhere in our code most likely, because we have code in various places that still assume a single line item per subscription. I didn't know that they're both end up on a single subscription during an upgrade, we had assumed this would only occur when having multiple unrelated plans on a single subscription for billing alignment purposes. |
@bbangert thank you for calling it out. I was worried that might be the case. I'll take quick look through the code and will create a follow up ticket to fix the other occurrences. |
Because
email
for the previous and current subscription. The existing logic assumes
there is only one invoice line item.
This pull request
correct next invoice date.
Issue that this pull request solves
Closes: #12690
Checklist
Put an
x
in the boxes that apply