-
Notifications
You must be signed in to change notification settings - Fork 180
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
Fix manual capture issues. #211
base: canary
Are you sure you want to change the base?
Conversation
|
@Hanskrogh is attempting to deploy a commit to the Saleor Commerce Team on Vercel. A member of the Team first needs to authorize it. |
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.
Thanks @Hanskrogh for your contribution to Saleor 😄
I've left a few comments, since I wasn't sure your changes will fully address the issue with authorizations / manualCapture please let me know what you think
@@ -433,13 +433,12 @@ function stripePaymentIntentEventToPartialTransactionEventReportMutationVariable | |||
switch (stripeEvent.type) { | |||
// handling these is required | |||
case "payment_intent.succeeded": { | |||
if (manualCapture) break; |
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.
if (manualCapture) break; |
In Stripe docs: the succeeded
status for PaymentIntent is sent only when funds are in the account, i.e. captured. The capture could have used manual capture method, but this doesn't matter for this event. I think we should just remove this condition.
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 @witoszekdev :)
If you would like to omit this line, I believe we should change the response action of TransactionChargeRequested event in salor, maybe allowing CHARGE_REQUEST as result response?
Otherwise a transaction event with same pspReference and action is going to be fired, which would cause an error.
As it is working right now, the following happens:
TransactionChargeRequested -> stripe app captures the amount in stripe -> sends response back to saleor, which creates the transaction event "CHARGE_SUCCESS".
As a race-condition the "payment_intent_succeded" event form stripe is fired, which then duplicates the "CHARGE_SUCCESS" transaction event.
I believe the best approach, would be to change the logic in saleor to only allow CHARGE_FAILED or CHARGE_REQUESTED results types and then let the app handle everything through webhooks. This would streamline the process with automatic_capture.
But I am not really a python dev, so I can't really change the logic inside saleor... :/
Let me know what you think?
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.
I am not even sure that the CHARGE_REQUESTED would resolve the case, as the race condition still exists between wether the payment_intent.suceeded or saleor adds the transaction event first :)
const type = manualCapture | ||
? TransactionEventTypeEnum.AuthorizationSuccess | ||
: TransactionEventTypeEnum.AuthorizationAdjustment; |
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.
This one is a bit tricky - if this is the first Authorization, then AuthorizationSuccess
event is fine, but in case the Authorization amount was incremented we might need to use AuthorizationAdjustment
event type. This is because Saleor doesn't allow updating the Authorization any other way (AuthorizationSuccess
with different amount than previously reported event would result in an exception).
manualCapture
from my understanding, won't be a gauge whether this was a first authorzation, or if it was incremented. According to Stripe docs we should check amount_updates
on Charge
object to determine this.
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.
Let's not remove this file :)
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.
My fault :)
This pull request fixes issues with manual capture: