fix(stripe): Fix the bug when there's failed payment we charge customer extra amount #8072

tianrunhe · 2023-04-19T17:20:10Z

Description

Fixes #8061. To reproduce the bug, on master branch:

Install Stripe CLI if you haven't
Setup Stripe webhook to forward to your local server: stripe listen --forward-to localhost:3000/stripe, and copy the webhook singing secret to replace the one in your .env
Create a new team/org and go to the org billing page
Upgrade with this Stripe payment failure test card: 4000 0000 0000 3063, expiration date any month in the future, CVC can be anything

Observe the org is upgraded
Wait a moment and then refresh the page, you should see that one invoice is in the state of PENDING (because the payment wasn't successful) but another invoice for next month with 2x of the amount

Now update the credit card number to 4242 4242 4242 4242
Wait a moment and then refresh the page, you should see a new invoice generated and paid with 2x of the amount, indicating the customer has been double charged.

Testing scenarios

With this branch pulled and server started, repeat the above-mentioned steps 3 - 5.

Wait a moment and then refresh the page, you should see one invoice in the state of FAILED:

Now update the credit card number to 4242 4242 4242 4242
Wait a moment and then refresh the page, you should see a new invoice generated and paid the correct amount, and an upcoming invoice also with the correct amount

Final checklist

I checked the code review guidelines
I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
I have performed a self-review of my code, the same way I'd do it for any other team member
I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
Whenever I took a non-obvious choice I added a comment explaining why I did it this way
I added the label One Review Required if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
PR title is human readable and could be used in changelog

…ly returned

nickoferrall

Really appreciate the testing steps with screenshots! It made my life as a reviewer really easy. It works, and LGTM!

Dschoordsch · 2023-04-21T09:35:05Z

webhook singing secret

I wonder which tune it sings

Dschoordsch

Looks good and it's the best type of fix where we just remove broken code 😎
However the changes in terminateSubscription appear wrong, are these covered by the test case?

Dschoordsch · 2023-04-21T11:20:19Z

packages/server/billing/helpers/terminateSubscription.ts

@@ -18,7 +18,7 @@ const terminateSubscription = async (orgId: string) => {
            stripeSubscriptionId: null
          },
          {returnChanges: true}
-        )('changes')(0)('old_val')
+        )('changes')(0)('old_val')('stripeSubscriptionId')


-1 this seems to contradict how we're reading the rethinkResult in line l26

I wasn't sure what you meant by contradiction, but I made a few changes to make it more clear & easy to read. Let me know if that's what you were thinking?

Thanks for the change, now I understand what you were doing.

Dschoordsch · 2023-04-21T11:20:57Z

packages/server/billing/helpers/terminateSubscription.ts

@@ -32,7 +32,7 @@ const terminateSubscription = async (orgId: string) => {
    try {
      await manager.deleteSubscription(stripeSubscriptionId)
    } catch (e) {
-      // noop
+      console.error(`cannot delete subscription ${stripeSubscriptionId}`, e)


+1 Reading the comment in l28 it appears as if we intentionally do not track errors here. Are you sure this is necessary?

I would highly suspect that it never did what it was supposed to do... the issue was hidden as we didn't have that many self upgrades in the past, let alone the even rare cases where the payment failed.

As a quick test, I commented out this part as the comment seemed to indicate, i.e., it wouldn't harm if the deletion failed or not even performed, as Stripe is configured to do so automatically. Well, not the case here. If the customer failed to perform the first payment and just did nothing, this is gonna happen:

A failed payment notification was sent

The invoice stays in PENDING state

The org still stays in the Team tier, i.e., the customer is enjoying the Team tier for free

Looking at Stripe side, after 24 hours, this is going to happen:

The invoice is void

The payment was cancelled

The subscription was incomplete
But again, none of those prevent us from giving the customers Team tier for free, from the app side.

So, all in all, we definitely need that subscription deletion to finish and succeed, in order for the app to cancel that upgrade. Therefore, we should capture the error.

Thanks for investigating this and removing the comment!

Dschoordsch

Did not test again, but looks good now!

Dschoordsch · 2023-04-25T13:09:48Z

packages/server/billing/helpers/terminateSubscription.ts

@@ -32,7 +32,7 @@ const terminateSubscription = async (orgId: string) => {
    try {
      await manager.deleteSubscription(stripeSubscriptionId)
    } catch (e) {
-      // noop
+      console.error(`cannot delete subscription ${stripeSubscriptionId}`, e)


Thanks for investigating this and removing the comment!

Dschoordsch · 2023-04-25T13:10:46Z

packages/server/billing/helpers/terminateSubscription.ts

@@ -18,7 +18,7 @@ const terminateSubscription = async (orgId: string) => {
            stripeSubscriptionId: null
          },
          {returnChanges: true}
-        )('changes')(0)('old_val')
+        )('changes')(0)('old_val')('stripeSubscriptionId')


Thanks for the change, now I understand what you were doing.

tianrunhe added 2 commits April 18, 2023 16:37

fix(stripe): fix an issue where Stripe subscription ID was not proper…

acb4785

…ly returned

Do not manually debt customer credit balance

7665d15

tianrunhe linked an issue Apr 19, 2023 that may be closed by this pull request

Multiples charges incurred on Team subscription when customer failed payments #8061

Closed

1 task

github-actions bot added the size/s label Apr 19, 2023

tianrunhe marked this pull request as ready for review April 19, 2023 17:41

tianrunhe requested a review from nickoferrall April 19, 2023 17:42

nickoferrall approved these changes Apr 20, 2023

View reviewed changes

nickoferrall requested a review from Dschoordsch April 20, 2023 14:43

Dschoordsch requested changes Apr 21, 2023

View reviewed changes

tianrunhe added 2 commits April 21, 2023 10:25

Make rethinkResult more clear

18fe9ef

Remove comment

a06453c

tianrunhe requested a review from Dschoordsch April 25, 2023 02:59

Dschoordsch approved these changes Apr 25, 2023

View reviewed changes

tianrunhe merged commit 2ba09fb into master Apr 25, 2023

tianrunhe deleted the bug/8061/duplicateChargesOnFailedPayments branch April 25, 2023 16:31

igorlesnenko mentioned this pull request May 26, 2023

fix: do not cancel stripe subscription in case of failed payment #8285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stripe): Fix the bug when there's failed payment we charge customer extra amount #8072

fix(stripe): Fix the bug when there's failed payment we charge customer extra amount #8072

tianrunhe commented Apr 19, 2023 •

edited

Loading

nickoferrall left a comment

Dschoordsch commented Apr 21, 2023

Dschoordsch left a comment

Dschoordsch Apr 21, 2023

tianrunhe Apr 25, 2023

Dschoordsch Apr 25, 2023

Dschoordsch Apr 21, 2023

tianrunhe Apr 25, 2023

Dschoordsch Apr 25, 2023

Dschoordsch left a comment

Dschoordsch Apr 25, 2023

Dschoordsch Apr 25, 2023

fix(stripe): Fix the bug when there's failed payment we charge customer extra amount #8072

fix(stripe): Fix the bug when there's failed payment we charge customer extra amount #8072

Conversation

tianrunhe commented Apr 19, 2023 • edited Loading

Description

Testing scenarios

Final checklist

nickoferrall left a comment

Choose a reason for hiding this comment

Dschoordsch commented Apr 21, 2023

Dschoordsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dschoordsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianrunhe commented Apr 19, 2023 •

edited

Loading