-
-
Notifications
You must be signed in to change notification settings - Fork 965
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
base: main
Are you sure you want to change the base?
Conversation
…e Solved - django#1739 Added verbose_name_plural = _("Local Django Communities") in the Meta class
Fixed incorrect pluralization of LocalDjangoCommunity in admin - Issu…
Issue 1764
Arpan8925 issue1764
for more information, see https://pre-commit.ci
Solved issue of #1764 |
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.
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!
for more information, see https://pre-commit.ci
@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:
|
Test Resulting: Ran 6 tests in 0.084s OK Seems Everything Perfect !
for more information, see https://pre-commit.ci
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 |
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
…angoproject.com into arpan8925-issue1764
for more information, see https://pre-commit.ci
@bmispelon Can you review ? |
@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. |
Hi, There are several issues with your PR:
Could you revert the changes and go back to the version you had at the beginning with only a Thanks! 🎸 |
Added @transaction.atomic decorator to the checkout_session_completed method
All database operations are now wrapped in a transaction: