From b1b7a4abf10956d2d2863ba2b7e08e39b1abfbc1 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Mon, 18 Nov 2024 18:45:13 +0100 Subject: [PATCH] fix(payment): Idempotent cancellation and proper creationg fail handling (#10135) RESOLVES SUP-188 **What** Two changes are happening here - In the stripe payment provider, idempotent cancellation action, if not id is provided then return the existing data unchanged - Payment module should not try to cancel a session that have failed to be created in the first place --- .changeset/selfish-poems-jog.md | 6 ++ .../services/payment-module/index.spec.ts | 81 ++++++++++++++++++- .../payment/src/services/payment-module.ts | 26 +++--- .../payment-stripe/src/core/stripe-base.ts | 5 ++ 4 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 .changeset/selfish-poems-jog.md diff --git a/.changeset/selfish-poems-jog.md b/.changeset/selfish-poems-jog.md new file mode 100644 index 0000000000..ffb43be1bd --- /dev/null +++ b/.changeset/selfish-poems-jog.md @@ -0,0 +1,6 @@ +--- +"@medusajs/payment": patch +"@medusajs/payment-stripe": patch +--- + +fix(payment): Idempotent cancellation and proper creationg fail handling diff --git a/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts b/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts index 7a09a1cfab..8ea2d35095 100644 --- a/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts +++ b/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts @@ -14,6 +14,10 @@ moduleIntegrationTestRunner({ moduleName: Modules.PAYMENT, testSuite: ({ MikroOrmWrapper, service }) => { describe("Payment Module Service", () => { + beforeEach(() => { + jest.clearAllMocks() + }) + it(`should export the appropriate linkable configuration`, () => { const linkable = Module(Modules.PAYMENT, { service: PaymentModuleService, @@ -395,7 +399,6 @@ moduleIntegrationTestRunner({ customer: {}, billing_address: {}, email: "test@test.test.com", - resource_id: "cart_test", }, }) @@ -422,6 +425,80 @@ moduleIntegrationTestRunner({ }) ) }) + + it("should gracefully handle payment session creation fails from external provider", async () => { + jest + .spyOn((service as any).paymentProviderService_, "createSession") + .mockImplementationOnce(() => { + throw new Error("Create session error") + }) + + const deleteProviderSessionMock = jest.spyOn( + (service as any).paymentProviderService_, + "deleteSession" + ) + + const deletePaymentSessionMock = jest.spyOn( + (service as any).paymentSessionService_, + "delete" + ) + + const error = await service + .createPaymentSession("pay-col-id-1", { + provider_id: "pp_system_default", + amount: 200, + currency_code: "usd", + data: {}, + context: { + extra: {}, + customer: {}, + billing_address: {}, + email: "test@test.test.com", + }, + }) + .catch((e) => e) + + expect(deleteProviderSessionMock).toHaveBeenCalledTimes(0) + expect(deletePaymentSessionMock).toHaveBeenCalledTimes(1) + expect(error.message).toEqual("Create session error") + }) + + it("should gracefully handle payment session creation fails from internal failure", async () => { + jest + .spyOn((service as any).paymentSessionService_, "update") + .mockImplementationOnce(() => { + throw new Error("Update session error") + }) + + const deleteProviderSessionMock = jest.spyOn( + (service as any).paymentProviderService_, + "deleteSession" + ) + + const deletePaymentSessionMock = jest.spyOn( + (service as any).paymentSessionService_, + "delete" + ) + + const error = await service + .createPaymentSession("pay-col-id-1", { + provider_id: "pp_system_default", + amount: 200, + currency_code: "usd", + data: {}, + context: { + extra: {}, + customer: {}, + billing_address: {}, + email: "test@test.test.com", + }, + }) + .catch((e) => e) + + expect(deleteProviderSessionMock).toHaveBeenCalledTimes(1) + expect(deletePaymentSessionMock).toHaveBeenCalledTimes(1) + expect(error.message).toEqual("Update session error") + }) }) describe("update", () => { @@ -436,7 +513,6 @@ moduleIntegrationTestRunner({ customer: {}, billing_address: {}, email: "test@test.test.com", - resource_id: "cart_test", }, }) @@ -446,7 +522,6 @@ moduleIntegrationTestRunner({ currency_code: "eur", data: {}, context: { - resource_id: "res_id", extra: {}, customer: {}, billing_address: {}, diff --git a/packages/modules/payment/src/services/payment-module.ts b/packages/modules/payment/src/services/payment-module.ts index d27487f45c..bc24d4ea7d 100644 --- a/packages/modules/payment/src/services/payment-module.ts +++ b/packages/modules/payment/src/services/payment-module.ts @@ -299,6 +299,7 @@ export default class PaymentModuleService @MedusaContext() sharedContext?: Context ): Promise { let paymentSession: PaymentSession | undefined + let providerPaymentSession: Record | undefined try { paymentSession = await this.createPaymentSession_( @@ -307,30 +308,33 @@ export default class PaymentModuleService sharedContext ) - const providerSessionSession = - await this.paymentProviderService_.createSession(input.provider_id, { + providerPaymentSession = await this.paymentProviderService_.createSession( + input.provider_id, + { context: { ...input.context, session_id: paymentSession.id }, amount: input.amount, currency_code: input.currency_code, - }) + } + ) paymentSession = ( await this.paymentSessionService_.update( { id: paymentSession.id, - data: { ...input.data, ...providerSessionSession }, + data: { ...input.data, ...providerPaymentSession }, }, sharedContext ) )[0] } catch (error) { - if (paymentSession) { - // In case the session is created, but fails to be updated in Medusa, - // we catch the error and delete the session and rethrow. + if (providerPaymentSession) { await this.paymentProviderService_.deleteSession({ provider_id: input.provider_id, data: input.data, }) + } + + if (paymentSession) { await this.paymentSessionService_.delete( paymentSession.id, sharedContext @@ -340,9 +344,7 @@ export default class PaymentModuleService throw error } - return await this.baseRepository_.serialize(paymentSession, { - populate: true, - }) + return await this.baseRepository_.serialize(paymentSession) } @InjectTransactionManager() @@ -573,9 +575,7 @@ export default class PaymentModuleService // NOTE: currently there is no update with the provider but maybe data could be updated const result = await this.paymentService_.update(data, sharedContext) - return await this.baseRepository_.serialize(result[0], { - populate: true, - }) + return await this.baseRepository_.serialize(result[0]) } @InjectManager() diff --git a/packages/modules/providers/payment-stripe/src/core/stripe-base.ts b/packages/modules/providers/payment-stripe/src/core/stripe-base.ts index 8c51410911..9c1cc73f8d 100644 --- a/packages/modules/providers/payment-stripe/src/core/stripe-base.ts +++ b/packages/modules/providers/payment-stripe/src/core/stripe-base.ts @@ -186,6 +186,11 @@ abstract class StripeBase extends AbstractPaymentProvider { ): Promise { try { const id = paymentSessionData.id as string + + if (!id) { + return paymentSessionData + } + return (await this.stripe_.paymentIntents.cancel( id )) as unknown as PaymentProviderSessionResponse["data"]