fix: Improve Webhook Reliability via Prefixed Subscription IDs

[Neer-Pathak]

Overview

Fixes #764

This pull request introduces a new approach to managing donation cancellations by appending a prefix ("cancel") to the stripe_subscription_id of donations that have been requested for cancellation. This method aims to improve our handling of webhook events and ensure that our database remains in sync with Stripe's subscription status.

Key Changes

• Cancel Donation API: - def cancel_donation(request, hero):

When a donation is canceled, the stripe_subscription_id is prefixed with "cancel" to indicate that a cancellation has been initiated. This allows us to easily identify donations that are in the process of being canceled but may not yet have received confirmation from Stripe.

Webhook Handling:

• Subscription Cancel method - def subscription_cancelled(self): in class WebhookHandler: The subscription_cancelled method in the webhook handler is updated to look for donations with the prefixed stripe_subscription_id. The method constructs the prefixed ID when querying the donation, ensuring that the lookup reflects the current state of the database. And finally sets it to "".

Data Integrity:

By using a prefix, we can filter donations that have initiated cancellation without losing the original subscription ID. Once the webhook processes successfully, it will clear the stripe_subscription_id, ensuring the database is accurately updated.

Concerns Addressed

Webhook Failure:

If the webhook fails to deliver the cancellation event, the prefixed IDs will allow us to identify donations that are pending cancellation. This can help with future cleanup processes or notifications to users.

Webhook Success:

If the webhook processes successfully, the prefixed IDs will be updated to reflect the cancellation (to ""), effectively maintaining data integrity and consistency within the application.

Assumptions

The webhook is reliable, but we need to account for potential delivery failures. The prefixing approach serves as a safeguard against this possibility.

Conclusion

This implementation enhances our ability to manage donation cancellations efficiently while addressing potential issues with webhook delivery. The prefixing strategy offers a straightforward way to track pending cancellations and maintain synchronization between our database and Stripe.

[bmispelon]

Hi,

It seems some of the existing tests are failing:

  ======================================================================
  FAIL: test_cancel_donation (fundraising.tests.test_views.TestCampaign.test_cancel_donation)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/unittest/mock.py", line 1395, in patched
      return func(*newargs, **newkeywargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/fundraising/tests/test_views.py", line 95, in test_cancel_donation
      self.assertEqual("", donation.stripe_subscription_id)
  AssertionError: '' != 'cancel12345'
  + cancel12345
  
  
  ======================================================================
  FAIL: test_subscription_cancelled (fundraising.tests.test_views.TestWebhooks.test_subscription_cancelled)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/unittest/mock.py", line 1395, in patched
      return func(*newargs, **newkeywargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/fundraising/tests/test_views.py", line 205, in test_subscription_cancelled
      self.assertEqual(donation.stripe_subscription_id, "")
  AssertionError: 'sub_3MXPaZGXvVZSrS' != ''
  - sub_3MXPaZGXvVZSrS

Could you fix those failing tests? Thanks!

[Neer-Pathak]

To maintain a clean Git history, I am force-pushing and rewriting the commit history to eliminate unnecessary commits, including the recent fix for the failing test that I just implemented.

[bmispelon]

To maintain a clean Git history, I am force-pushing and rewriting the commit history to eliminate unnecessary commits, including the recent fix for the failing test that I just implemented.

Excellent, thanks 👍🏻

The tests now pass, so that's good news. I will try to review this, but note that this might take some time because I'd like to get familiar with how stripe is used on the site first (this area of the codebase is unknown for me).

[Neer-Pathak]

Hi @bmispelon , just checking in to see if there are any updates or if any changes are needed on my PR. Let me know when you get a chance. Thanks!

[Neer-Pathak]

The tests now pass, so that's good news. I will try to review this, but note that this might take some time because I'd like to get familiar with how stripe is used on the site first (this area of the codebase is unknown for me).

Hi any update on this?

[Neer-Pathak]

Hi! @bmispelon @felixxm Just checking in, it’s been a year. Please let me know if there’s anything I can do to help move this forward. Thanks!

[adamzap]

@Neer-Pathak Could you squash and rebase the branch?

[Neer-Pathak]

Hi @adamzap , its done, the branch has now been squashed and rebased on top of the latest main.