-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: Handle webhooks for ACH microdeposits lifecycle #1116
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: billing/views.py
Did you find this useful? React with a 👍 or 👎 |
@@ -17,6 +17,8 @@ class StripeWebhookEvents: | |||
"customer.updated", | |||
"invoice.payment_failed", | |||
"invoice.payment_succeeded", | |||
"payment_intent.succeeded", | |||
"setup_intent.succeeded", |
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.
TODO - these need to be turned on in Stripe dashboard
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1116 +/- ##
==========================================
- Coverage 96.08% 96.06% -0.02%
==========================================
Files 837 837
Lines 19687 19756 +69
==========================================
+ Hits 18916 18979 +63
- Misses 771 777 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
) | ||
log.info( | ||
f"Stripe Checkout Session created successfully for owner {owner.ownerid} by user #{self.requesting_user.ownerid}" | ||
) | ||
return session["id"] | ||
|
||
def _is_unverified_payment_method(self, payment_method_id: str) -> bool: |
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.
nit: could we move this to a separate helpers file
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.
I tried it out but ending up leaving it here for now as I was thinking spinning off a helpers file just because this file is unmanageable seems not great imo. Ideally we can split this mega file out instead.. I think creating a new kitchen sink could make it worse so left it here status quo
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.
Ah okay fair enough, originally I thought this was the webhooks file but its not. I actually meant to add this comment to the new functions added in views.py
billing/views.py
Outdated
@@ -105,9 +130,9 @@ def invoice_payment_failed(self, invoice: stripe.Invoice) -> None: | |||
invoice["payment_intent"], expand=["payment_method"] | |||
) | |||
card = ( |
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.
Really not a fan of this, how about
Try:
card = payment_intent.payment_method.card
except NotFound:
card = None
billing/views.py
Outdated
if invoice.payment_intent: | ||
payment_intent = stripe.PaymentIntent.retrieve(invoice.payment_intent) | ||
if ( | ||
payment_intent is not None |
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.
nit: don't need the is not None
billing/views.py
Outdated
latest_invoice.payment_intent | ||
) | ||
return ( | ||
payment_intent is not None |
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.
not: don't need the is not None
billing/views.py
Outdated
return ( | ||
payment_intent is not None | ||
and payment_intent.status == "requires_action" | ||
and payment_intent.next_action is not None |
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.
same comment here, and I saw in the other reference we used .get() if we want to do the same here
owner = Owner.objects.get(stripe_customer_id=customer_id) | ||
payment_method = stripe.PaymentMethod.retrieve(payment_method_id) | ||
|
||
is_us_bank_account = payment_method.type == "us_bank_account" and hasattr( |
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.
jw, why do we need both of these conditions?
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.
I thought it'd be safer to check both as I haven't read through all scenarios where Stripe may populate one but not the other. When we reach this point in the code we expect both of these to be in this state in order to proceed
) | ||
if is_us_bank_account: | ||
setup_intents = stripe.SetupIntent.list( | ||
payment_method=payment_method_id, limit=1 |
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.
does this default to the most recent?
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.
We should only ever have 1 setup intent to 1 paymentMethodId but the only available api to fetch this from stripe is this list with filter, it seemed. I don't think it's possible on their end for it to be more than 1. I'd guess if it is, it'd take the most recent
services/billing.py
Outdated
setup_intents = stripe.SetupIntent.list( | ||
payment_method=payment_method_id, limit=1 | ||
) | ||
if ( |
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.
I'd try and see if we can get away with an eager try catch here
Maybe something like
Try
latest_intent = setup_intents.data[0]
if latest_intent.status blah blah
except NotFound:
return False
services/billing.py
Outdated
def _cleanup_incomplete_subscription( | ||
self, subscription: stripe.Subscription, owner: Owner | ||
): | ||
latest_invoice = subscription.get("latest_invoice") |
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.
Similar thought here with the Try
try:
payment_intent_id = subscription["latest_invoice"]["payment_intent"]
except NotFound:
return None
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
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Handles ACH microdeposits delayed payment verification flow.
This PR handles the below new logical flows:
User async verifies the ACH microdeposits for the initial checkout session
payment_intent.succeeded
User async verifies the ACH microdeposits for subsequent paymentMethodUpdate
setup_intent.succeeded
The initial checkout session by Stripe has a "charges_automatically" that issues an invoice that fails when trying to use the ach microdeposits payment method.
If the user abandons their initial checkout session that has a pending ACH, we delete that abandoned one before offering a way to make a new one
customer.subscription.deleted
listener be the source of truth of then removing it from the owner object in our postgresOther things of note:
Tested end-to-end flows:
More about the end to end testing here: https://www.notion.so/sentry/Suejung-s-notes-1838b10e4b5d8075b7a9cca013de502c?pvs=4#18c8b10e4b5d80a29634f0702e743a90
Epic: codecov/engineering-team#2622