From 81b8bbff0d700039b5594030ef209d91017fcfc5 Mon Sep 17 00:00:00 2001 From: julianpoyourow Date: Tue, 5 Nov 2024 23:38:13 +0000 Subject: [PATCH] feat(cart): add needs_input state Because: - We want to track a cart state where the cart waits on a user input This commit: - Adds the needs_input state Closes FXA-10542 --- .../cart/src/lib/cart.manager.in.spec.ts | 21 +++++++ libs/payments/cart/src/lib/cart.manager.ts | 18 +++++- .../cart/src/lib/cart.service.spec.ts | 32 +++------- libs/payments/cart/src/lib/cart.service.ts | 62 ++++++++----------- .../payments/cart/src/lib/checkout.service.ts | 1 - .../src/lib/actions/checkoutCartWithStripe.ts | 27 +++----- .../src/lib/nestapp/nextjs-actions.service.ts | 7 --- .../validators/SetCartProcessingActionArgs.ts | 13 ---- libs/payments/ui/src/lib/utils/get-cart.ts | 1 + .../db/mysql/account/src/lib/kysely-types.ts | 1 + .../db/mysql/account/src/test/carts.sql | 2 +- .../databases/fxa/patches/patch-157-158.sql | 5 ++ .../databases/fxa/patches/patch-158-157.sql | 5 ++ .../databases/fxa/target-patch.json | 2 +- 14 files changed, 97 insertions(+), 100 deletions(-) delete mode 100644 libs/payments/ui/src/lib/nestapp/validators/SetCartProcessingActionArgs.ts create mode 100644 packages/db-migrations/databases/fxa/patches/patch-157-158.sql create mode 100644 packages/db-migrations/databases/fxa/patches/patch-158-157.sql diff --git a/libs/payments/cart/src/lib/cart.manager.in.spec.ts b/libs/payments/cart/src/lib/cart.manager.in.spec.ts index 855486ba33..c956c491f4 100644 --- a/libs/payments/cart/src/lib/cart.manager.in.spec.ts +++ b/libs/payments/cart/src/lib/cart.manager.in.spec.ts @@ -226,6 +226,27 @@ describe('CartManager', () => { }); }); + describe('setNeedsInputcart', () => { + it('succeeds', async () => { + await directUpdate(db, { state: CartState.PROCESSING }, testCart.id); + testCart = await cartManager.fetchCartById(testCart.id); + + await cartManager.setNeedsInputCart(testCart.id); + const cart = await cartManager.fetchCartById(testCart.id); + + expect(cart.state).toEqual(CartState.NEEDS_INPUT); + }); + + it('fails - invalid state', async () => { + try { + await cartManager.setNeedsInputCart(testCart.id); + fail('Error in finishCart'); + } catch (error) { + expect(error).toBeInstanceOf(CartInvalidStateForActionError); + } + }); + }); + describe('finishErrorCart', () => { it('succeeds', async () => { const items = FinishErrorCartFactory(); diff --git a/libs/payments/cart/src/lib/cart.manager.ts b/libs/payments/cart/src/lib/cart.manager.ts index f882cac6de..d20096cd50 100644 --- a/libs/payments/cart/src/lib/cart.manager.ts +++ b/libs/payments/cart/src/lib/cart.manager.ts @@ -38,7 +38,8 @@ const ACTIONS_VALID_STATE = { finishErrorCart: [CartState.START, CartState.PROCESSING], deleteCart: [CartState.START, CartState.PROCESSING], restartCart: [CartState.START, CartState.PROCESSING, CartState.FAIL], - setProcessingCart: [CartState.START], + setProcessingCart: [CartState.START, CartState.NEEDS_INPUT], + setNeedsInputCart: [CartState.PROCESSING], }; // Type guard to check if action is valid key in ACTIONS_VALID_STATE @@ -181,6 +182,21 @@ export class CartManager { } } + public async setNeedsInputCart(cartId: string) { + const cart = await this.fetchCartById(cartId); + + this.checkActionForValidCartState(cart, 'setNeedsInputCart'); + + try { + await updateCart(this.db, Buffer.from(cartId, 'hex'), cart.version, { + state: CartState.NEEDS_INPUT, + }); + } catch (error) { + const cause = error instanceof CartNotUpdatedError ? undefined : error; + throw new CartNotUpdatedError(cartId, cause); + } + } + public async setProcessingCart(cartId: string) { const cart = await this.fetchCartById(cartId); diff --git a/libs/payments/cart/src/lib/cart.service.spec.ts b/libs/payments/cart/src/lib/cart.service.spec.ts index 266474dcc7..9e553fa8e2 100644 --- a/libs/payments/cart/src/lib/cart.service.spec.ts +++ b/libs/payments/cart/src/lib/cart.service.spec.ts @@ -38,7 +38,6 @@ import { StripeResponseFactory, MockStripeConfigProvider, AccountCustomerManager, - StripePaymentIntentFactory, StripeSubscriptionFactory, StripePaymentMethodFactory, } from '@fxa/payments/stripe'; @@ -344,16 +343,12 @@ describe('CartService', () => { it('accepts payment with stripe', async () => { const mockCart = ResultCartFactory(); const mockPaymentMethodId = faker.string.uuid(); - const mockPaymentIntent = StripePaymentIntentFactory({ - payment_method: mockPaymentMethodId, - status: 'succeeded', - }); - jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); jest - .spyOn(checkoutService, 'payWithStripe') - .mockResolvedValue(mockPaymentIntent.status); - jest.spyOn(cartManager, 'finishCart').mockResolvedValue(); + .spyOn(cartManager, 'fetchAndValidateCartVersion') + .mockResolvedValue(mockCart); + jest.spyOn(cartManager, 'setProcessingCart').mockResolvedValue(); + jest.spyOn(checkoutService, 'payWithStripe').mockResolvedValue(); jest.spyOn(cartManager, 'finishErrorCart').mockResolvedValue(); await cartService.checkoutCartWithStripe( @@ -368,27 +363,20 @@ describe('CartService', () => { mockPaymentMethodId, mockCustomerData ); - expect(cartManager.finishCart).toHaveBeenCalledWith( - mockCart.id, - mockCart.version, - {} - ); expect(cartManager.finishErrorCart).not.toHaveBeenCalled(); }); it('calls cartManager.finishErrorCart when error occurs during checkout', async () => { const mockCart = ResultCartFactory(); const mockPaymentMethodId = faker.string.uuid(); - const mockPaymentIntent = StripePaymentIntentFactory({ - payment_method: mockPaymentMethodId, - status: 'succeeded', - }); - jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); + jest + .spyOn(cartManager, 'fetchAndValidateCartVersion') + .mockResolvedValue(mockCart); + jest.spyOn(cartManager, 'setProcessingCart').mockResolvedValue(); jest .spyOn(checkoutService, 'payWithStripe') - .mockResolvedValue(mockPaymentIntent.status); - jest.spyOn(cartManager, 'finishCart').mockRejectedValue(undefined); + .mockRejectedValue(new Error('test')); jest.spyOn(cartManager, 'finishErrorCart').mockResolvedValue(); await cartService.checkoutCartWithStripe( @@ -426,7 +414,7 @@ describe('CartService', () => { ); expect(checkoutService.payWithPaypal).toHaveBeenCalledWith( - { ...mockCart, version: mockCart.version + 1 }, + mockCart, mockCustomerData, mockToken ); diff --git a/libs/payments/cart/src/lib/cart.service.ts b/libs/payments/cart/src/lib/cart.service.ts index ca273cef70..20d09a60b5 100644 --- a/libs/payments/cart/src/lib/cart.service.ts +++ b/libs/payments/cart/src/lib/cart.service.ts @@ -212,32 +212,32 @@ export class CartService { confirmationTokenId: string, customerData: CheckoutCustomerData ) { + let updatedCart: ResultCart | null = null; try { //Ensure that the cart version matches the value passed in from FE - const cart = await this.cartManager.fetchAndValidateCartVersion( + await this.cartManager.fetchAndValidateCartVersion(cartId, version); + + await this.cartManager.setProcessingCart(cartId); + + // Ensure we have a positive lock on the processing cart + updatedCart = await this.cartManager.fetchAndValidateCartVersion( cartId, - version + version + 1 ); - - const status = await this.checkoutService.payWithStripe( - cart, - confirmationTokenId, - customerData - ); - - const updatedCart = await this.cartManager.fetchCartById(cartId); - - if (status === 'succeeded') { - // multiple threads is causing this to be called on an already-successful cart - // this then throws an error, and has us trying to finish the cart while its in an error state - await this.cartManager.finishCart(cartId, updatedCart.version, {}); - } } catch (e) { - // TODO: Handle errors and provide an associated reason for failure - await this.cartManager.finishErrorCart(cartId, { - errorReasonId: CartErrorReasonId.Unknown, - }); + throw new CartStateProcessingError(cartId, e); } + + // Intentionally left out of try/catch block to so that the rest of the logic + // is non-blocking and can be handled asynchronously. + this.checkoutService + .payWithStripe(updatedCart, confirmationTokenId, customerData) + .catch(async () => { + // TODO: Handle errors and provide an associated reason for failure + await this.cartManager.finishErrorCart(cartId, { + errorReasonId: CartErrorReasonId.Unknown, + }); + }); } async checkoutCartWithPaypal( @@ -249,16 +249,15 @@ export class CartService { let updatedCart: ResultCart | null = null; try { //Ensure that the cart version matches the value passed in from FE - const cart = await this.cartManager.fetchAndValidateCartVersion( - cartId, - version - ); + await this.cartManager.fetchAndValidateCartVersion(cartId, version); await this.cartManager.setProcessingCart(cartId); - updatedCart = { - ...cart, - version: cart.version + 1, - }; + + // Ensure we have a positive lock on the processing cart + updatedCart = await this.cartManager.fetchAndValidateCartVersion( + cartId, + version + 1 + ); } catch (e) { throw new CartStateProcessingError(cartId, e); } @@ -364,13 +363,6 @@ export class CartService { } } - /** - * Update a cart to be in the processing state - */ - async setCartProcessing(cartId: string): Promise { - await this.cartManager.setProcessingCart(cartId); - } - /** * Update a cart in the database by ID or with an existing cart reference */ diff --git a/libs/payments/cart/src/lib/checkout.service.ts b/libs/payments/cart/src/lib/checkout.service.ts index 56339c3c1e..d05d200f09 100644 --- a/libs/payments/cart/src/lib/checkout.service.ts +++ b/libs/payments/cart/src/lib/checkout.service.ts @@ -306,7 +306,6 @@ export class CheckoutService { } await this.postPaySteps(cart, updatedVersion, subscription, uid); } - return status; } async payWithPaypal( diff --git a/libs/payments/ui/src/lib/actions/checkoutCartWithStripe.ts b/libs/payments/ui/src/lib/actions/checkoutCartWithStripe.ts index 5ee4719835..8d5290d675 100644 --- a/libs/payments/ui/src/lib/actions/checkoutCartWithStripe.ts +++ b/libs/payments/ui/src/lib/actions/checkoutCartWithStripe.ts @@ -10,7 +10,6 @@ import { CheckoutCartWithStripeActionArgs, CheckoutCartWithStripeActionCustomerData, } from '../nestapp/validators/CheckoutCartWithStripeActionArgs'; -import { SetCartProcessingActionArgs } from '../nestapp/validators/SetCartProcessingActionArgs'; export const checkoutCartWithStripe = async ( cartId: string, @@ -18,22 +17,12 @@ export const checkoutCartWithStripe = async ( confirmationTokenId: string, customerData: CheckoutCartWithStripeActionCustomerData ) => { - await getApp() - .getActionsService() - .setCartProcessing( - plainToClass(SetCartProcessingActionArgs, { cartId, version }) - ); - - const updatedVersion = version + 1; - - getApp() - .getActionsService() - .checkoutCartWithStripe( - plainToClass(CheckoutCartWithStripeActionArgs, { - cartId, - version: updatedVersion, - customerData, - confirmationTokenId, - }) - ); + getApp().getActionsService().checkoutCartWithStripe( + plainToClass(CheckoutCartWithStripeActionArgs, { + cartId, + version, + customerData, + confirmationTokenId, + }) + ); }; diff --git a/libs/payments/ui/src/lib/nestapp/nextjs-actions.service.ts b/libs/payments/ui/src/lib/nestapp/nextjs-actions.service.ts index f4ad88f945..c9f52732c9 100644 --- a/libs/payments/ui/src/lib/nestapp/nextjs-actions.service.ts +++ b/libs/payments/ui/src/lib/nestapp/nextjs-actions.service.ts @@ -20,7 +20,6 @@ import { SetupCartActionArgs } from './validators/SetupCartActionArgs'; import { UpdateCartActionArgs } from './validators/UpdateCartActionArgs'; import { RecordEmitterEventArgs } from './validators/RecordEmitterEvent'; import { PaymentsEmitterService } from '../emitter/emitter.service'; -import { SetCartProcessingActionArgs } from './validators/SetCartProcessingActionArgs'; import { FinalizeProcessingCartActionArgs } from './validators/finalizeProcessingCartActionArgs'; import { PollCartActionArgs } from './validators/pollCartActionArgs'; @@ -105,12 +104,6 @@ export class NextJSActionsService { await this.cartService.finalizeProcessingCart(args.cartId); } - async setCartProcessing(args: SetCartProcessingActionArgs) { - await new Validator().validateOrReject(args); - - await this.cartService.setCartProcessing(args.cartId); - } - async getPayPalCheckoutToken(args: GetPayPalCheckoutTokenArgs) { await new Validator().validateOrReject(args); diff --git a/libs/payments/ui/src/lib/nestapp/validators/SetCartProcessingActionArgs.ts b/libs/payments/ui/src/lib/nestapp/validators/SetCartProcessingActionArgs.ts deleted file mode 100644 index 5465876a89..0000000000 --- a/libs/payments/ui/src/lib/nestapp/validators/SetCartProcessingActionArgs.ts +++ /dev/null @@ -1,13 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -import { IsNumber, IsString } from 'class-validator'; - -export class SetCartProcessingActionArgs { - @IsString() - cartId!: string; - - @IsNumber() - version!: number; -} diff --git a/libs/payments/ui/src/lib/utils/get-cart.ts b/libs/payments/ui/src/lib/utils/get-cart.ts index e86a10909d..a02813fac1 100644 --- a/libs/payments/ui/src/lib/utils/get-cart.ts +++ b/libs/payments/ui/src/lib/utils/get-cart.ts @@ -7,6 +7,7 @@ import { SupportedPages } from './types'; export const cartStateToPageMap = { [CartState.START]: SupportedPages.START, [CartState.PROCESSING]: SupportedPages.PROCESSING, + [CartState.NEEDS_INPUT]: SupportedPages.PROCESSING, [CartState.SUCCESS]: SupportedPages.SUCCESS, [CartState.FAIL]: SupportedPages.ERROR, }; diff --git a/libs/shared/db/mysql/account/src/lib/kysely-types.ts b/libs/shared/db/mysql/account/src/lib/kysely-types.ts index 5473e4d444..972f136882 100644 --- a/libs/shared/db/mysql/account/src/lib/kysely-types.ts +++ b/libs/shared/db/mysql/account/src/lib/kysely-types.ts @@ -26,6 +26,7 @@ export enum CartState { START = 'start', PROCESSING = 'processing', SUCCESS = 'success', + NEEDS_INPUT = 'needs_input', FAIL = 'fail', } diff --git a/libs/shared/db/mysql/account/src/test/carts.sql b/libs/shared/db/mysql/account/src/test/carts.sql index 063ca2cb5e..7a1268e7d9 100644 --- a/libs/shared/db/mysql/account/src/test/carts.sql +++ b/libs/shared/db/mysql/account/src/test/carts.sql @@ -1,7 +1,7 @@ CREATE TABLE `carts` ( `id` binary(16) NOT NULL, `uid` binary(16) DEFAULT NULL, - `state` enum('start','processing','success','fail') COLLATE utf8mb4_bin NOT NULL, + `state` enum('start','processing','success','fail','needs_input') COLLATE utf8mb4_bin NOT NULL, `errorReasonId` varchar(255) COLLATE utf8mb4_bin DEFAULT NULL, `offeringConfigId` varchar(255) COLLATE utf8mb4_bin NOT NULL, `interval` varchar(255) COLLATE utf8mb4_bin NOT NULL, diff --git a/packages/db-migrations/databases/fxa/patches/patch-157-158.sql b/packages/db-migrations/databases/fxa/patches/patch-157-158.sql new file mode 100644 index 0000000000..8af1c06123 --- /dev/null +++ b/packages/db-migrations/databases/fxa/patches/patch-157-158.sql @@ -0,0 +1,5 @@ +-- Add `needs_input` to the `carts` `state` enum. +ALTER TABLE carts +MODIFY state ENUM('start', 'processing', 'success', 'fail', 'needs_input'); + +UPDATE dbMetadata SET value = '158' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa/patches/patch-158-157.sql b/packages/db-migrations/databases/fxa/patches/patch-158-157.sql new file mode 100644 index 0000000000..c636192f96 --- /dev/null +++ b/packages/db-migrations/databases/fxa/patches/patch-158-157.sql @@ -0,0 +1,5 @@ +-- Add `needs_input` to the `carts` `state` enum. +-- ALTER TABLE carts +-- MODIFY state ENUM('start', 'processing', 'success', 'fail'); + +-- UPDATE dbMetadata SET value = '157' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa/target-patch.json b/packages/db-migrations/databases/fxa/target-patch.json index 95af065385..090b1b55f0 100644 --- a/packages/db-migrations/databases/fxa/target-patch.json +++ b/packages/db-migrations/databases/fxa/target-patch.json @@ -1,3 +1,3 @@ { - "level": 157 + "level": 158 }