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

Fixed the atomic transaction issue #1768

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

arpan8925
Copy link
Contributor

  1. Added @transaction.atomic decorator to the checkout_session_completed method

  2. All database operations are now wrapped in a transaction:

     - Creating/getting the DjangoHero
     - Creating the Donation
     - Creating the Payment (for one-time donations)
    

@arpan8925
Copy link
Contributor Author

Solved issue of #1764

Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

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

Hi and thanks for working on this! 🎸

Decorating checkout_sesion_completed() like this shoudl work I think, but I'm not sure it's the best place to add the atomic block. If instead we had with transaction.atomic() before calling handler() inside WebhookHandler.handle(), then all web hooks would be fixed, not just the one.

There also need to be tests added, to make sure the behavior continues to work even as we refactor the code in the future.

Let me know what you think!

fundraising/views.py Outdated Show resolved Hide resolved
@arpan8925
Copy link
Contributor Author

@bmispelon Check if it fixes the issue, I made a separate file for the webhook test, review please

@bmispelon
Copy link
Member

@bmispelon Check if it fixes the issue, I made a separate file for the webhook test, review please

@arpan8925 The test are not working, did you try to run them locally? I see two failures here in the CI, they seem to be the same error:

 ======================================================================
  ERROR: test_checkout_session_completed_atomic (fundraising.tests.test_webhook.TestWebhooks.test_checkout_session_completed_atomic)
  ----------------------------------------------------------------------
  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_webhook.py", line 109, in test_checkout_session_completed_atomic
      self.client.post(
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/test/client.py", line 1070, in post
      response = super().post(
                 ^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/test/client.py", line 490, in post
      return self.generic(
             ^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/test/client.py", line 617, in generic
      return self.request(**r)
             ^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/test/client.py", line 1013, in request
      self.check_exception(response)
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/test/client.py", line 743, in check_exception
      raise exc_value
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
      response = get_response(request)
                 ^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/core/handlers/base.py", line 197, in _get_response
      response = wrapped_callback(request, *callback_args, **callback_kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/views/decorators/http.py", line 64, in inner
      return func(request, *args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/.tox/py312-tests/lib/python3.12/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper
      return view_func(request, *args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/fundraising/views.py", line 204, in receive_webhook
      return WebhookHandler(event).handle()
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/fundraising/views.py", line 222, in handle
      return handler()
             ^^^^^^^^^
    File "/home/runner/work/djangoproject.com/djangoproject.com/fundraising/views.py", line 304, in checkout_session_completed
      session.customer, stripe_version="2020-08-27"
      ^^^^^^^^^^^^^^^^
  AttributeError: 'dict' object has no attribute 'customer'

arpan8925 and others added 2 commits November 25, 2024 22:06
Test Resulting:

Ran 6 tests in 0.084s
OK

Seems Everything Perfect !
@arpan8925
Copy link
Contributor Author

It Seems , test working now !

The test is failing because the webhook handler isn't properly handling the InvalidRequestError from Stripe.

However, Everything fixed.. Please Check & Review

@arpan8925
Copy link
Contributor Author

image

arpan8925 and others added 4 commits November 25, 2024 23:25
The test test_checkout_session_completed_atomic is actually passing:
test_checkout_session_completed_atomic (fundraising.tests.test_webhook.TestWebhooks.test_checkout_session_completed_atomic) ... Error in webhook handler: Database error
Internal Server Error: /fundraising/receive-webhook/
ok

All fundraising tests are passing:

test_empty_object (fundraising.tests.test_webhook.TestWebhooks.test_empty_object) ... ok
test_no_such_event (fundraising.tests.test_webhook.TestWebhooks.test_no_such_event) ... ok
test_payment_failed (fundraising.tests.test_webhook.TestWebhooks.test_payment_failed) ... ok
test_record_payment (fundraising.tests.test_webhook.TestWebhooks.test_record_payment) ... ok
test_subscription_cancelled (fundraising.tests.test_webhook.TestWebhooks.test_subscription_cancelled) ... ok
@arpan8925
Copy link
Contributor Author

@bmispelon Can you review ?

@django django deleted a comment from arpan8925 Nov 30, 2024
@marksweb
Copy link
Contributor

@arpan8925 I'm sure you'll get another review on here when someones free to do so. We're all volunteers here doing this in our free time.

@bmispelon
Copy link
Member

Hi,

There are several issues with your PR:

  • This version is very different and much more complicated than the one you submitted initially, and it contains unexplained code changes
  • Instead of adding a new test method to the TestWebhooks you created a whole new file with some of the old tests copy/pasted.
  • There are now conflicts with other changes that have been merged since this pull request was opened (this is related to the first point)

Could you revert the changes and go back to the version you had at the beginning with only a with atomic() block? Then I will make some proposals for how to add tests for that.

Thanks! 🎸

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.

3 participants