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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: canary
Choose a base branch
from
Open

Fix manual capture issues. #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e6bf7ae
fix: manual capture
Hanskrogh Feb 18, 2024
6a68d4c
fix: payment_intent.succeeded webhook handler
Hanskrogh Feb 18, 2024
f80ccae
update stripe integration
Hanskrogh Feb 18, 2024
5ad89ec
feat: payment intent charge sucess
Hanskrogh Feb 18, 2024
2bd2583
feat: ignore payment_intent.succeeded
Hanskrogh Feb 18, 2024
4e6ccf2
test
Hanskrogh Feb 18, 2024
633c955
test
Hanskrogh Feb 18, 2024
8f1e591
fix: remove buttons
Hanskrogh Feb 18, 2024
81be6ca
test
Hanskrogh Feb 18, 2024
5bbe17d
Based on result og transaction charge request, set relevant actions
Hanskrogh Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .env.example
View file
Open in desktop
Copy link
Member

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

My fault :)

This file was deleted.

9 changes: 5 additions & 4 deletions src/modules/webhooks/stripe-webhook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,12 @@ function stripePaymentIntentEventToPartialTransactionEventReportMutationVariable
switch (stripeEvent.type) {
// handling these is required
case "payment_intent.succeeded": {
if (manualCapture) break;
Copy link
Member

@witoszekdev witoszekdev Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
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.

CleanShot 2024-02-19 at 16 31 42@2x

Copy link
Author

@Hanskrogh Hanskrogh Feb 19, 2024 •
edited
Loading

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?

Copy link
Author

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 amount = getSaleorAmountFromStripeAmount({
amount: manualCapture ? paymentIntent.amount_capturable : paymentIntent.amount_received,
currency: paymentIntent.currency,
});
const type = manualCapture
? TransactionEventTypeEnum.AuthorizationSuccess
: TransactionEventTypeEnum.ChargeSuccess;
const type = TransactionEventTypeEnum.ChargeSuccess;

return { amount, type, externalUrl, pspReference, message };
}
Expand Down Expand Up @@ -503,7 +502,9 @@ function stripePaymentIntentEventToPartialTransactionEventReportMutationVariable
amount: paymentIntent.amount,
currency: paymentIntent.currency,
});
const type = TransactionEventTypeEnum.AuthorizationAdjustment;
const type = manualCapture
? TransactionEventTypeEnum.AuthorizationSuccess
: TransactionEventTypeEnum.AuthorizationAdjustment;
Comment on lines +505 to +507
Copy link
Member

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.

return { amount, type, externalUrl, pspReference, message };
}

Expand Down
1 change: 1 addition & 0 deletions src/modules/webhooks/transaction-charge-requested.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export const TransactionChargeRequestedWebhookHandler = async (
pspReference,
amount,
externalUrl,
actions: result === "CHARGE_SUCCESS" ? ["REFUND"] : ["CANCEL", "CHARGE"],
};
return transactionChargeRequestedResponse;
} else {
Expand Down
5 changes: 4 additions & 1 deletion src/schemas/TransactionChargeRequested/TransactionChargeRequestedResponse.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
"amount": { "$ref": "definitions.json#/definitions/PositiveDecimal" },
"time": { "$ref": "definitions.json#/definitions/DateTime" },
"externalUrl": { "type": "string" },
"message": { "type": "string" }
"message": { "type": "string" },
"actions": {
"$ref": "definitions.json#/definitions/TransactionActions"
}
},
"additionalProperties": false,
"required": ["pspReference"]
Expand Down