From 496dcf10c430b9ff9c21ee8bf8fae34ba01902de Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Tue, 19 Dec 2023 10:47:41 +0100 Subject: [PATCH] feat(medusa): Prevent cart completion conflict (#5814) --- .changeset/smooth-days-dress.md | 5 ++ .../store/carts/__tests__/create-line-item.js | 2 +- .../__tests__/create-payment-sessions.js | 2 +- .../store/carts/create-line-item/index.ts | 54 ++++++------ .../create-line-item/utils/handler-steps.ts | 2 +- .../store/carts/create-payment-sessions.ts | 87 ++++++++++--------- .../medusa/src/services/__tests__/cart.js | 21 +++-- packages/medusa/src/services/cart.ts | 79 +++++++++-------- .../strategies/__tests__/cart-completion.js | 5 +- .../medusa/src/strategies/cart-completion.ts | 47 +++++++--- 10 files changed, 179 insertions(+), 125 deletions(-) create mode 100644 .changeset/smooth-days-dress.md diff --git a/.changeset/smooth-days-dress.md b/.changeset/smooth-days-dress.md new file mode 100644 index 0000000000..bacab7a9a5 --- /dev/null +++ b/.changeset/smooth-days-dress.md @@ -0,0 +1,5 @@ +--- +"@medusajs/medusa": patch +--- + +Feat/cart completion conflict fixes diff --git a/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js b/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js index fefd5d86c3..8966a445f5 100644 --- a/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js +++ b/packages/medusa/src/api/routes/store/carts/__tests__/create-line-item.js @@ -26,7 +26,7 @@ describe("POST /store/carts/:id", () => { it("calls CartService retrieve", () => { expect(CartServiceMock.retrieve).toHaveBeenCalledTimes(1) - expect(CartServiceMock.retrieveWithTotals).toHaveBeenCalledTimes(1) + expect(CartServiceMock.retrieveWithTotals).toHaveBeenCalledTimes(2) }) it("calls LineItemService generate", () => { diff --git a/packages/medusa/src/api/routes/store/carts/__tests__/create-payment-sessions.js b/packages/medusa/src/api/routes/store/carts/__tests__/create-payment-sessions.js index 70c1973a98..b0f3762b07 100644 --- a/packages/medusa/src/api/routes/store/carts/__tests__/create-payment-sessions.js +++ b/packages/medusa/src/api/routes/store/carts/__tests__/create-payment-sessions.js @@ -22,7 +22,7 @@ describe("POST /store/carts/:id/payment-sessions", () => { }) it("calls Cart service retrieve", () => { - expect(CartServiceMock.retrieveWithTotals).toHaveBeenCalledTimes(1) + expect(CartServiceMock.retrieveWithTotals).toHaveBeenCalledTimes(2) }) it("returns 200", () => { diff --git a/packages/medusa/src/api/routes/store/carts/create-line-item/index.ts b/packages/medusa/src/api/routes/store/carts/create-line-item/index.ts index c18be0b525..acf34f7954 100644 --- a/packages/medusa/src/api/routes/store/carts/create-line-item/index.ts +++ b/packages/medusa/src/api/routes/store/carts/create-line-item/index.ts @@ -4,7 +4,7 @@ import { validator } from "../../../../../utils/validator" import { addOrUpdateLineItem, CreateLineItemSteps, - setPaymentSession, + setPaymentSessions, setVariantAvailability, } from "./utils/handler-steps" import { IdempotencyKey } from "../../../../../models" @@ -13,7 +13,6 @@ import { cleanResponseData } from "../../../../../utils/clean-response-data" import IdempotencyKeyService from "../../../../../services/idempotency-key" import { defaultStoreCartFields, defaultStoreCartRelations } from "../index" import { CartService } from "../../../../../services" -import { promiseAll } from "@medusajs/utils" /** * @oas [post] /store/carts/{id}/line-items @@ -130,37 +129,42 @@ export default async (req, res) => { case CreateLineItemSteps.SET_PAYMENT_SESSIONS: { try { const cartService: CartService = req.scope.resolve("cartService") - - const cart = await cartService - .withTransaction(manager) - .retrieveWithTotals(id, { - select: defaultStoreCartFields, - relations: [ - ...defaultStoreCartRelations, - "billing_address", - "region.payment_providers", - "payment_sessions", - "customer", - ], - }) - - const args = { - cart, - container: req.scope, - manager, + const getCart = async () => { + return await cartService + .withTransaction(manager) + .retrieveWithTotals(id, { + select: defaultStoreCartFields, + relations: [ + ...defaultStoreCartRelations, + "region.tax_rates", + "customer", + ], + }) } - await promiseAll([ - setVariantAvailability(args), - setPaymentSession(args), - ]) + const cart = await getCart() + + await manager.transaction(async (transactionManager) => { + await setPaymentSessions({ + cart, + container: req.scope, + manager: transactionManager, + }) + }) + + const freshCart = await getCart() + await setVariantAvailability({ + cart: freshCart, + container: req.scope, + manager, + }) idempotencyKey = await idempotencyKeyService .withTransaction(manager) .update(idempotencyKey.idempotency_key, { recovery_point: CreateLineItemSteps.FINISHED, response_code: 200, - response_body: { cart }, + response_body: { cart: freshCart }, }) } catch (e) { inProgress = false diff --git a/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts b/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts index 8a07863da0..d2860f3b1c 100644 --- a/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts +++ b/packages/medusa/src/api/routes/store/carts/create-line-item/utils/handler-steps.ts @@ -44,7 +44,7 @@ export async function addOrUpdateLineItem({ }) } -export async function setPaymentSession({ cart, container, manager }) { +export async function setPaymentSessions({ cart, container, manager }) { const cartService: CartService = container.resolve("cartService") const txCartService = cartService.withTransaction(manager) diff --git a/packages/medusa/src/api/routes/store/carts/create-payment-sessions.ts b/packages/medusa/src/api/routes/store/carts/create-payment-sessions.ts index a7ee8f308d..0757e938ad 100644 --- a/packages/medusa/src/api/routes/store/carts/create-payment-sessions.ts +++ b/packages/medusa/src/api/routes/store/carts/create-payment-sessions.ts @@ -1,12 +1,12 @@ -import { - CartService, - ProductVariantInventoryService, -} from "../../../../services" +import { CartService } from "../../../../services" import { defaultStoreCartFields, defaultStoreCartRelations } from "." import { EntityManager } from "typeorm" import IdempotencyKeyService from "../../../../services/idempotency-key" import { cleanResponseData } from "../../../../utils/clean-response-data" +import { setVariantAvailability } from "./create-line-item/utils/handler-steps" +import { WithRequiredProperty } from "../../../../types/common" +import { Cart } from "../../../../models" /** * @oas [post] /store/carts/{id}/payment-sessions @@ -55,14 +55,10 @@ import { cleanResponseData } from "../../../../utils/clean-response-data" export default async (req, res) => { const { id } = req.params - const cartService: CartService = req.scope.resolve("cartService") const idempotencyKeyService: IdempotencyKeyService = req.scope.resolve( "idempotencyKeyService" ) - const productVariantInventoryService: ProductVariantInventoryService = - req.scope.resolve("productVariantInventoryService") - const manager: EntityManager = req.scope.resolve("manager") const headerKey = req.get("Idempotency-Key") || "" @@ -88,40 +84,53 @@ export default async (req, res) => { while (inProgress) { switch (idempotencyKey.recovery_point) { case "started": { - await manager - .transaction("SERIALIZABLE", async (transactionManager) => { - idempotencyKey = await idempotencyKeyService - .withTransaction(transactionManager) - .workStage( - idempotencyKey.idempotency_key, - async (stageManager) => { - await cartService - .withTransaction(stageManager) - .setPaymentSessions(id) - - const cart = await cartService - .withTransaction(stageManager) - .retrieveWithTotals(id, { - select: defaultStoreCartFields, - relations: defaultStoreCartRelations, - }) - - await productVariantInventoryService.setVariantAvailability( - cart.items.map((i) => i.variant), - cart.sales_channel_id! - ) - - return { - response_code: 200, - response_body: { cart }, - } - } + try { + const cartService: CartService = req.scope.resolve("cartService") + const getCart = async () => { + return await cartService + .withTransaction(manager) + .retrieveWithTotals( + id, + { + select: defaultStoreCartFields, + relations: [ + ...defaultStoreCartRelations, + "region.tax_rates", + "customer", + ], + }, + { force_taxes: true } ) + } + + const cart = await getCart() + + await manager.transaction(async (transactionManager) => { + const txCartService = + cartService.withTransaction(transactionManager) + await txCartService.setPaymentSessions( + cart as WithRequiredProperty + ) }) - .catch((e) => { - inProgress = false - err = e + + const freshCart = await getCart() + await setVariantAvailability({ + cart: freshCart, + container: req.scope, + manager, }) + + idempotencyKey = await idempotencyKeyService + .withTransaction(manager) + .update(idempotencyKey.idempotency_key, { + recovery_point: "finished", + response_code: 200, + response_body: { cart: freshCart }, + }) + } catch (e) { + inProgress = false + err = e + } break } diff --git a/packages/medusa/src/services/__tests__/cart.js b/packages/medusa/src/services/__tests__/cart.js index bc657885c1..4d9b6011c3 100644 --- a/packages/medusa/src/services/__tests__/cart.js +++ b/packages/medusa/src/services/__tests__/cart.js @@ -1641,23 +1641,32 @@ describe("CartService", () => { const cartRepository = MockRepository({ findOneWithRelations: (rel, q) => { if (q.where.id === IdMap.getId("cart-to-filter")) { - return Promise.resolve(cart3) + return Promise.resolve({ + id: IdMap.getId("cart-to-filter"), + ...cart3, + }) } if (q.where.id === IdMap.getId("cart-with-session")) { - return Promise.resolve(cart2) + return Promise.resolve({ + id: IdMap.getId("cart-with-session"), + ...cart2, + }) } if (q.where.id === IdMap.getId("cart-remove")) { - return Promise.resolve(cart4) + return Promise.resolve({ id: IdMap.getId("cart-remove"), ...cart4 }) } if (q.where.id === IdMap.getId("cart-negative")) { - return Promise.resolve(cart4) + return Promise.resolve({ id: IdMap.getId("cart-negative"), ...cart4 }) } if ( q.where.id === IdMap.getId("cartWithMixedSelectedInitiatedSessions") ) { - return Promise.resolve(cart5) + return Promise.resolve({ + id: IdMap.getId("cartWithMixedSelectedInitiatedSessions"), + ...cart5, + }) } - return Promise.resolve(cart1) + return Promise.resolve({ id: q.where.id, ...cart1 }) }, }) diff --git a/packages/medusa/src/services/cart.ts b/packages/medusa/src/services/cart.ts index 6aadd7b98d..36afc3834f 100644 --- a/packages/medusa/src/services/cart.ts +++ b/packages/medusa/src/services/cart.ts @@ -1693,27 +1693,32 @@ class CartService extends TransactionBaseService { * a payment object, that we will use to update our cart payment with. * Additionally, if the payment does not require more or fails, we will * set the payment on the cart. - * @param cartId - the id of the cart to authorize payment for + * @param cartOrId - the id of the cart to authorize payment for * @param context - object containing whatever is relevant for * authorizing the payment with the payment provider. As an example, * this could be IP address or similar for fraud handling. * @return the resulting cart */ async authorizePayment( - cartId: string, - context: Record & { - cart_id: string - } = { cart_id: "" } + cartOrId: string | WithRequiredProperty, + context: Record = { cart_id: "" } ): Promise { + context = { + ...context, + cart_id: isString(cartOrId) ? cartOrId : cartOrId.id, + } + return await this.atomicPhase_( async (transactionManager: EntityManager) => { const cartRepository = transactionManager.withRepository( this.cartRepository_ ) - const cart = await this.retrieveWithTotals(cartId, { - relations: ["payment_sessions", "items.variant.product.profiles"], - }) + const cart = !isString(cartOrId) + ? cartOrId + : await this.retrieveWithTotals(cartOrId, { + relations: ["payment_sessions", "items.variant.product.profiles"], + }) // If cart total is 0, we don't perform anything payment related if (cart.total! <= 0) { @@ -1875,8 +1880,7 @@ class CartService extends TransactionBaseService { await this.eventBus_ .withTransaction(transactionManager) .emit(CartService.Events.UPDATED, { id: cartId }) - }, - "SERIALIZABLE" + } ) } @@ -1889,7 +1893,9 @@ class CartService extends TransactionBaseService { * @param cartOrCartId - the id of the cart to set payment session for * @return the result of the update operation. */ - async setPaymentSessions(cartOrCartId: Cart | string): Promise { + async setPaymentSessions( + cartOrCartId: WithRequiredProperty | string + ): Promise { return await this.atomicPhase_( async (transactionManager: EntityManager) => { const psRepo = transactionManager.withRepository( @@ -1899,31 +1905,30 @@ class CartService extends TransactionBaseService { const paymentProviderServiceTx = this.paymentProviderService_.withTransaction(transactionManager) - const cartId = - typeof cartOrCartId === `string` ? cartOrCartId : cartOrCartId.id - - const cart = await this.retrieveWithTotals( - cartId, - { - relations: [ - "items.variant.product.profiles", - "items.adjustments", - "discounts", - "discounts.rule", - "gift_cards", - "shipping_methods", - "shipping_methods.shipping_option", - "billing_address", - "shipping_address", - "region", - "region.tax_rates", - "region.payment_providers", - "payment_sessions", - "customer", - ], - }, - { force_taxes: true } - ) + const cart = !isString(cartOrCartId) + ? cartOrCartId + : await this.retrieveWithTotals( + cartOrCartId, + { + relations: [ + "items.variant.product.profiles", + "items.adjustments", + "discounts", + "discounts.rule", + "gift_cards", + "shipping_methods", + "shipping_methods.shipping_option", + "billing_address", + "shipping_address", + "region", + "region.tax_rates", + "region.payment_providers", + "payment_sessions", + "customer", + ], + }, + { force_taxes: true } + ) const { total, region } = cart @@ -1957,7 +1962,7 @@ class CartService extends TransactionBaseService { currency_code: cart.region.currency_code, } const partialPaymentSessionData = { - cart_id: cartId, + cart_id: cart.id, data: {}, status: PaymentSessionStatus.PENDING, amount: total, diff --git a/packages/medusa/src/strategies/__tests__/cart-completion.js b/packages/medusa/src/strategies/__tests__/cart-completion.js index ebfacc2653..a2db8ac922 100644 --- a/packages/medusa/src/strategies/__tests__/cart-completion.js +++ b/packages/medusa/src/strategies/__tests__/cart-completion.js @@ -62,9 +62,10 @@ const toTest = [ expect(cartServiceMock.authorizePayment).toHaveBeenCalledTimes(1) expect(cartServiceMock.authorizePayment).toHaveBeenCalledWith( - "test-cart", + expect.objectContaining({ + id: "test-cart", + }), { - cart_id: "test-cart", idempotency_key: { idempotency_key: "ikey", recovery_point: "tax_lines_created", diff --git a/packages/medusa/src/strategies/cart-completion.ts b/packages/medusa/src/strategies/cart-completion.ts index e7ec49ac30..48700febd8 100644 --- a/packages/medusa/src/strategies/cart-completion.ts +++ b/packages/medusa/src/strategies/cart-completion.ts @@ -2,12 +2,13 @@ import { IEventBusService, IInventoryService, ReservationItemDTO, + WithRequiredProperty, } from "@medusajs/types" import { AbstractCartCompletionStrategy, CartCompletionResponse, } from "../interfaces" -import { IdempotencyKey, Order } from "../models" +import { Cart, IdempotencyKey, Order } from "../models" import { PaymentProviderService, ProductVariantInventoryService, @@ -84,7 +85,7 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy { switch (idempotencyKey.recovery_point) { case "started": { await this.activeManager_ - .transaction("SERIALIZABLE", async (transactionManager) => { + .transaction(async (transactionManager) => { idempotencyKey = await this.idempotencyKeyService_ .withTransaction(transactionManager) .workStage( @@ -101,7 +102,7 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy { } case "tax_lines_created": { await this.activeManager_ - .transaction("SERIALIZABLE", async (transactionManager) => { + .transaction(async (transactionManager) => { idempotencyKey = await this.idempotencyKeyService_ .withTransaction(transactionManager) .workStage( @@ -122,7 +123,7 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy { case "payment_authorized": { await this.activeManager_ - .transaction("SERIALIZABLE", async (transactionManager) => { + .transaction(async (transactionManager) => { idempotencyKey = await this.idempotencyKeyService_ .withTransaction(transactionManager) .workStage( @@ -246,19 +247,39 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy { const txCartService = this.cartService_.withTransaction(manager) - let cart = await txCartService.retrieve(id, { - relations: ["payment_sessions"], - }) + let cart: Cart | WithRequiredProperty = + await txCartService.retrieveWithTotals(id, { + relations: [ + "items.variant.product.profiles", + "items.adjustments", + "discounts", + "discounts.rule", + "gift_cards", + "shipping_methods", + "shipping_methods.shipping_option", + "billing_address", + "shipping_address", + "region", + "region.tax_rates", + "region.payment_providers", + "payment_sessions", + "customer", + ], + }) if (cart.payment_sessions?.length) { - await txCartService.setPaymentSessions(id) + await txCartService.setPaymentSessions( + cart as WithRequiredProperty + ) } - cart = await txCartService.authorizePayment(id, { - ...context, - cart_id: id, - idempotency_key: idempotencyKey, - }) + cart = await txCartService.authorizePayment( + cart as WithRequiredProperty, + { + ...context, + idempotency_key: idempotencyKey, + } + ) if (cart.payment_session) { if (