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

@@ -0,0 +1,6 @@
+---
+"@medusajs/payment": patch
+"@medusajs/payment-stripe": patch
+---
+
+fix(payment): Idempotent cancellation and proper creationg fail handling

packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts

@@ -14,6 +14,10 @@ moduleIntegrationTestRunner<IPaymentModuleService>({
   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<IPaymentModuleService>({
                 customer: {},
                 billing_address: {},
                 email: "test@test.test.com",
-                resource_id: "cart_test",
               },
             })
 
@@ -422,6 +425,80 @@ moduleIntegrationTestRunner<IPaymentModuleService>({
               })
             )
           })
+
+          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<IPaymentModuleService>({
                 customer: {},
                 billing_address: {},
                 email: "test@test.test.com",
-                resource_id: "cart_test",
               },
             })
 
@@ -446,7 +522,6 @@ moduleIntegrationTestRunner<IPaymentModuleService>({
               currency_code: "eur",
               data: {},
               context: {
-                resource_id: "res_id",
                 extra: {},
                 customer: {},
                 billing_address: {},

packages/modules/payment/src/services/payment-module.ts

@@ -299,6 +299,7 @@ export default class PaymentModuleService
     @MedusaContext() sharedContext?: Context
   ): Promise<PaymentSessionDTO> {
     let paymentSession: PaymentSession | undefined
+    let providerPaymentSession: Record<string, unknown> | undefined
 
     try {
       paymentSession = await this.createPaymentSession_(
@@ -307,30 +308,33 @@ export default class PaymentModuleService
         sharedContext
       )
 
-      const providerSessionSession =
+      providerPaymentSession = await this.paymentProviderService_.createSession(
-        await this.paymentProviderService_.createSession(input.provider_id, {
+        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) {
+      if (providerPaymentSession) {
-        // In case the session is created, but fails to be updated in Medusa,
-        // we catch the error and delete the session and rethrow.
         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, {
+    return await this.baseRepository_.serialize(paymentSession)
-      populate: true,
-    })
   }
 
   @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<PaymentDTO>(result[0], {
+    return await this.baseRepository_.serialize<PaymentDTO>(result[0])
-      populate: true,
-    })
   }
 
   @InjectManager()

packages/modules/providers/payment-stripe/src/core/stripe-base.ts

@@ -186,6 +186,11 @@ abstract class StripeBase extends AbstractPaymentProvider<StripeOptions> {
   ): Promise<PaymentProviderError | PaymentProviderSessionResponse["data"]> {
     try {
       const id = paymentSessionData.id as string
+
+      if (!id) {
+        return paymentSessionData
+      }
+
       return (await this.stripe_.paymentIntents.cancel(
         id
       )) as unknown as PaymentProviderSessionResponse["data"]