Add manual order completion; refactor Stripe event handling - #3855
Open
jkachel wants to merge 10 commits into
Open
Add manual order completion; refactor Stripe event handling#3855jkachel wants to merge 10 commits into
jkachel wants to merge 10 commits into
Conversation
… work with >1 gateway
…eanup stuff and tests
…ight; finish out the new stripe event/checkout session processing stuff; update its tests too There were a couple of places where the default gateway setting was coming out of main.settings, which means the mocked one doesn't work.. and a few places where it would have been bette r to specify the payment gateway explicitly, so did all that. Still have some more Stripe mock object testing stuff to finish out.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36131093 | Triggered | Generic High Entropy Secret | 5405629 | ecommerce/fixtures.py | View secret |
| 36131093 | Triggered | Generic High Entropy Secret | a85cfc1 | ecommerce/fixtures.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
OpenAPI ChangesShow/hide changesUnexpected changes? Ensure your branch is up-to-date with |
jkachel
marked this pull request as ready for review
August 17, 2026 17:29
…sure the flags do what we expect
dsubak
self-requested a review
August 18, 2026 13:25
dsubak
reviewed
Aug 18, 2026
dsubak
reviewed
Aug 18, 2026
dsubak
reviewed
Aug 18, 2026
| checkout_session_id = session_transaction.data.get("id") | ||
|
|
||
| if ( | ||
| session_transaction.data.get("object") != "checkout.session" |
Contributor
There was a problem hiding this comment.
Nit: Should checkout.session be a constant?
dsubak
reviewed
Aug 18, 2026
Comment on lines
+1404
to
+1407
| "checkout.session.completed", | ||
| "checkout.session.expired", | ||
| "checkout.session.async_payment_failed", | ||
| "checkout.session.async_payment_succeeded", |
Contributor
There was a problem hiding this comment.
Might be good to pull this into the constants file too
dsubak
reviewed
Aug 18, 2026
| } | ||
|
|
||
| if self.order.total_price_paid == 0: | ||
| if self.order.total_price_paid == 0 or payment_data.get("is_administrative"): |
Contributor
There was a problem hiding this comment.
This may be an issue on my end so my apologies if that is the case, but when trying to go through a normal checkout with stripe as my payment gateway this line explodes because payment_data is a CheckoutSession StripeObject and is_administrative isn't in that data (and I guess .get throws an error since they stopped inheriting from dict quite recently)
Does this seem like a real issue or is everything working as expected on your setup?
Contributor
Author
There was a problem hiding this comment.
The problem was that generate_checkout_payload saves the checkout session now (so we can maybe look it up later if we need, for out-of-band fulfillment) - it needed to be converted to dict first though and it didn't do that. So, real issue that is now fixed. The other places where the session data gets store were doing this properly so it was just the new one that was broken.
Contributor
There was a problem hiding this comment.
Awesome; that's mostly in line with what I pieced together and makes total sense. I'll put it back through it's paces first thing tomorrow and as long as everything looks good, I think this'll be good to go!
Co-authored-by: Dan Subak <dsubak@users.noreply.github.com>
Co-authored-by: Dan Subak <dsubak@users.noreply.github.com>
…re we save it in generate_checkout_payload; add more constants
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
Closes mitodl/hq#12694
Description (What does it do?)
Adds the ability to administratively fulfill an order using the existing management command. This also fixes some issues with the command itself, including adding support for Stripe transactions (which isn't something it supported).
In addition, this refactors some of the Stripe implementation - there were some things that were problematic about the existing implementation:
How can this be tested?
Payment collected, but order status not updated:
Create an order, and then complete the payment process but do not allow the app to be aware of that. The easiest way to do this is:
Then, run
resolve_pending_orderfor the order in question. (For Stripe, do not re-enable your webhooks - you don't want it to retry sending events.) The app should reach out to the payment processor, determine that the order is complete, and then fulfill the order (or cancel it, depending on what you did).Forcing fulfillment of a PendingOrder, regardless of payment state:
Create an order, and then return to the app once you get to the payment processor. For ease of testing, manually type your local instance's URL into the address bar. This should leave the order in the Pending state.
Then, run
resolve_pending_order --no-checkagainst the order in question. It should be set to Fulfilled and the order should be fulfilled.Disabling fulfillment:
For both of these scenarios, test the skip fulfillment option by specifying
--no-fulfillmentwhen running the management command. This should still mark the order as Fulfilled but it should not make any enrollment changes.Additional Context
A lot of the Stripe refactoring ended up in a bunch of constants and some helper functions that don't really belong in the app. Once these are shaken out a bit, they should move into the
ol_django PaymentGatewayapp.