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

feat: Handle webhooks for ACH microdeposits lifecycle #1116

Merged
merged 14 commits into from
Feb 4, 2025

Conversation

suejung-sentry
Copy link
Contributor

@suejung-sentry suejung-sentry commented Jan 30, 2025

Handles ACH microdeposits delayed payment verification flow.

This PR handles the below new logical flows:

  1. User async verifies the ACH microdeposits for the initial checkout session

    • added webhook listener for payment_intent.succeeded
    • once verified, we set the account as the default payment method on customer, subscription, invoice_settings
    • we also upgrade the plan at this point because stripe fulfills the initial checkout session's subscription from incomplete to active. That updated status is picked up by the customer.subscription.updated webhook handler that saves/updates/upgrades plan data in our database.
    • on Stripe's end the "charges_automatically" initital invoice that was pending gets successfully fulfilled
  2. User async verifies the ACH microdeposits for subsequent paymentMethodUpdate

    • added webhook listener for setup_intent.succeeded
    • once verified, we set the account as the default payment method on customer, subscription, invoice_settings
  3. 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.

    • ignore this case in the webhook listener for stripe invoice.payment_failed that does other stuff like marking the account delinquent
  4. 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

    • adjusted the upgrade_plan logic to handle when we have this incomplete subscription. Deletes the abandoned subscription in stripe but let's the customer.subscription.deleted listener be the source of truth of then removing it from the owner object in our postgres

Other things of note:

  • Stripe's list payment methods api does not included unverified paymentIntent / setupIntent
  • There isn't a combined Stripe endpoint to get a list of both payment intents and setup intents or otherwise a list of all unverified payment methods.

Tested end-to-end flows:

  1. Setup initial checkout session
    • Card
    • ACH instant
    • ACH microdeposits
  2. Change payment method
    • Card
    • ACH instant
    • ACH microdeposits
  3. Abandon ACH microdeposits
    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

Copy link

sentry-io bot commented Jan 30, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: billing/views.py

Function Unhandled Issue
invoice_payment_failed AttributeError: card /webhooks/stripe
Event Count: 96
customer_subscription_created Owner.DoesNotExist: Owner matching query does not exist. /webhook...
Event Count: 3

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",
Copy link
Contributor Author

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-notifications
Copy link

codecov-notifications bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 91.89189% with 6 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
services/billing.py 89.18% 4 Missing ⚠️
billing/views.py 94.59% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@codecov-qa
Copy link

codecov-qa bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 91.89189% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.97%. Comparing base (d13cceb) to head (3809147).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Copy link

codecov-public-qa bot commented Jan 30, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2733 1 2732 6
View the top 1 failed tests by shortest run time
billing/tests/test_views.py::StripeWebhookHandlerTests::test_has_unverified_initial_payment_method_payment_intent_succeeded
Stack Traces | 0.008s run time
self = <billing.tests.test_views.StripeWebhookHandlerTests testMethod=test_has_unverified_initial_payment_method_payment_intent_succeeded>
invoice_retrieve_mock = <MagicMock name='retrieve' id='140195832143344'>
payment_intent_retrieve_mock = <MagicMock name='retrieve' id='140195849034576'>

    @patch("services.billing.stripe.PaymentIntent.retrieve")
    @patch("services.billing.stripe.Invoice.retrieve")
    def test_has_unverified_initial_payment_method_payment_intent_succeeded(
        self, invoice_retrieve_mock, payment_intent_retrieve_mock
    ):
        subscription = Mock()
        subscription.latest_invoice = "inv_123"
    
        class MockPaymentIntent:
            status = "succeeded"
    
        invoice_retrieve_mock.return_value = Mock(payment_intent="pi_123")
        payment_intent_retrieve_mock.return_value = MockPaymentIntent()
    
        handler = StripeWebhookHandler()
>       result = handler._has_unverified_initial_payment_method(subscription)

billing/tests/test_views.py:1597: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <billing.views.StripeWebhookHandler object at 0x7f81e2f10440>
subscription = <Mock id='140195834960672'>

    def _has_unverified_initial_payment_method(
        self, subscription: stripe.Subscription
    ) -> bool:
        """
        Helper method to check if a subscription's latest invoice has a payment intent
        that requires verification (e.g. ACH microdeposits). This indicates that
        there is an unverified payment method from the initial CheckoutSession.
        """
        latest_invoice = stripe.Invoice.retrieve(subscription.latest_invoice)
        if latest_invoice and latest_invoice.payment_intent:
            payment_intent = stripe.PaymentIntent.retrieve(
                latest_invoice.payment_intent
            )
            return (
                payment_intent
>               and payment_intent.get("status") == "requires_action"
                and payment_intent.get("next_action")
                and payment_intent.get("next_action", {}).get("type")
                == "verify_with_microdeposits"
            )
E           AttributeError: 'MockPaymentIntent' object has no attribute 'get'

billing/views.py:378: AttributeError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Copy link
Contributor

github-actions bot commented Jan 30, 2025

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

@suejung-sentry suejung-sentry marked this pull request as ready for review January 30, 2025 18:58
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 91.89189% with 6 lines in your changes missing coverage. Please review.

Project coverage is 96.06%. Comparing base (d13cceb) to head (3809147).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
services/billing.py 89.18% 4 Missing ⚠️
billing/views.py 94.59% 2 Missing ⚠️
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     
Flag Coverage Δ
unit 95.97% <91.89%> (-0.02%) ⬇️
unit-latest-uploader 95.97% <91.89%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

)
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:
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

services/billing.py Outdated Show resolved Hide resolved
billing/views.py Outdated
@@ -105,9 +130,9 @@ def invoice_payment_failed(self, invoice: stripe.Invoice) -> None:
invoice["payment_intent"], expand=["payment_method"]
)
card = (
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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(
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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

setup_intents = stripe.SetupIntent.list(
payment_method=payment_method_id, limit=1
)
if (
Copy link
Contributor

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

def _cleanup_incomplete_subscription(
self, subscription: stripe.Subscription, owner: Owner
):
latest_invoice = subscription.get("latest_invoice")
Copy link
Contributor

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

Copy link
Contributor

@ajay-sentry ajay-sentry left a comment

Choose a reason for hiding this comment

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

lgtm

@suejung-sentry suejung-sentry added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit 714ba37 Feb 4, 2025
14 of 19 checks passed
@suejung-sentry suejung-sentry deleted the sshin/microdeposits-2 branch February 4, 2025 01:02
Copy link

sentry-io bot commented Feb 5, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Owner.MultipleObjectsReturned: get() returned more than one Owner -- it returned 2! /billing/stripe/webhooks View Issue
  • ‼️ Owner.DoesNotExist: Owner matching query does not exist. /billing/stripe/webhooks View Issue

Did you find this useful? React with a 👍 or 👎

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.

2 participants