From 005a5535754a3179d16c8b63bd602a3468a15da0 Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Thu, 23 Jan 2020 16:30:30 +0100 Subject: [PATCH] fix(core): Publish state transition events after persisting entities Relates to #245. Previously, the events would be published *prior* to the associated Order etc. entities being persisted to the DB. This meant that any event subscribers which then attempted to update that Order entity would have their changes clobbered by the subsequent call to `.save()`, resulting in vexing bugs. --- .../order-state-machine.ts | 4 ---- .../payment-state-machine.ts | 8 +------ .../refund-state-machine.ts | 8 +------ .../src/service/services/order.service.ts | 24 ++++++++++++++++--- .../services/payment-method.service.ts | 16 ++++++++++--- 5 files changed, 36 insertions(+), 24 deletions(-) diff --git a/packages/core/src/service/helpers/order-state-machine/order-state-machine.ts b/packages/core/src/service/helpers/order-state-machine/order-state-machine.ts index edca1bf690..4dee6b80b8 100644 --- a/packages/core/src/service/helpers/order-state-machine/order-state-machine.ts +++ b/packages/core/src/service/helpers/order-state-machine/order-state-machine.ts @@ -6,8 +6,6 @@ import { IllegalOperationError } from '../../../common/error/errors'; import { FSM, StateMachineConfig, Transitions } from '../../../common/finite-state-machine'; import { ConfigService } from '../../../config/config.service'; import { Order } from '../../../entity/order/order.entity'; -import { EventBus } from '../../../event-bus/event-bus'; -import { OrderStateTransitionEvent } from '../../../event-bus/events/order-state-transition-event'; import { HistoryService } from '../../services/history.service'; import { PromotionService } from '../../services/promotion.service'; import { StockMovementService } from '../../services/stock-movement.service'; @@ -24,7 +22,6 @@ export class OrderStateMachine { private stockMovementService: StockMovementService, private historyService: HistoryService, private promotionService: PromotionService, - private eventBus: EventBus, ) { this.config = this.initConfig(); } @@ -75,7 +72,6 @@ export class OrderStateMachine { if (toState === 'Cancelled') { data.order.active = false; } - this.eventBus.publish(new OrderStateTransitionEvent(fromState, toState, data.ctx, data.order)); await this.historyService.createHistoryEntryForOrder({ orderId: data.order.id, type: HistoryEntryType.ORDER_STATE_TRANSITION, diff --git a/packages/core/src/service/helpers/payment-state-machine/payment-state-machine.ts b/packages/core/src/service/helpers/payment-state-machine/payment-state-machine.ts index 3ebbfb570d..3250848f60 100644 --- a/packages/core/src/service/helpers/payment-state-machine/payment-state-machine.ts +++ b/packages/core/src/service/helpers/payment-state-machine/payment-state-machine.ts @@ -7,22 +7,18 @@ import { FSM, StateMachineConfig } from '../../../common/finite-state-machine'; import { ConfigService } from '../../../config/config.service'; import { Order } from '../../../entity/order/order.entity'; import { Payment } from '../../../entity/payment/payment.entity'; -import { EventBus } from '../../../event-bus/event-bus'; -import { PaymentStateTransitionEvent } from '../../../event-bus/events/payment-state-transition-event'; import { HistoryService } from '../../services/history.service'; import { PaymentState, paymentStateTransitions, PaymentTransitionData } from './payment-state'; @Injectable() export class PaymentStateMachine { - private readonly config: StateMachineConfig = { transitions: paymentStateTransitions, onTransitionStart: async (fromState, toState, data) => { return true; }, onTransitionEnd: async (fromState, toState, data) => { - this.eventBus.publish(new PaymentStateTransitionEvent(fromState, toState, data.ctx, data.payment, data.order)); await this.historyService.createHistoryEntryForOrder({ ctx: data.ctx, orderId: data.order.id, @@ -42,9 +38,7 @@ export class PaymentStateMachine { }, }; - constructor(private configService: ConfigService, - private historyService: HistoryService, - private eventBus: EventBus) {} + constructor(private configService: ConfigService, private historyService: HistoryService) {} getNextStates(payment: Payment): PaymentState[] { const fsm = new FSM(this.config, payment.state); diff --git a/packages/core/src/service/helpers/refund-state-machine/refund-state-machine.ts b/packages/core/src/service/helpers/refund-state-machine/refund-state-machine.ts index b74af35120..e513be9a69 100644 --- a/packages/core/src/service/helpers/refund-state-machine/refund-state-machine.ts +++ b/packages/core/src/service/helpers/refund-state-machine/refund-state-machine.ts @@ -7,22 +7,18 @@ import { FSM, StateMachineConfig } from '../../../common/finite-state-machine'; import { ConfigService } from '../../../config/config.service'; import { Order } from '../../../entity/order/order.entity'; import { Refund } from '../../../entity/refund/refund.entity'; -import { EventBus } from '../../../event-bus/event-bus'; -import { RefundStateTransitionEvent } from '../../../event-bus/events/refund-state-transition-event'; import { HistoryService } from '../../services/history.service'; import { RefundState, refundStateTransitions, RefundTransitionData } from './refund-state'; @Injectable() export class RefundStateMachine { - private readonly config: StateMachineConfig = { transitions: refundStateTransitions, onTransitionStart: async (fromState, toState, data) => { return true; }, onTransitionEnd: async (fromState, toState, data) => { - this.eventBus.publish(new RefundStateTransitionEvent(fromState, toState, data.ctx, data.refund, data.order)); await this.historyService.createHistoryEntryForOrder({ ctx: data.ctx, orderId: data.order.id, @@ -43,9 +39,7 @@ export class RefundStateMachine { }, }; - constructor(private configService: ConfigService, - private historyService: HistoryService, - private eventBus: EventBus) {} + constructor(private configService: ConfigService, private historyService: HistoryService) {} getNextStates(refund: Refund): RefundState[] { const fsm = new FSM(this.config, refund.state); diff --git a/packages/core/src/service/services/order.service.ts b/packages/core/src/service/services/order.service.ts index 46f8c786c0..b4a9e535c0 100644 --- a/packages/core/src/service/services/order.service.ts +++ b/packages/core/src/service/services/order.service.ts @@ -38,6 +38,10 @@ import { ProductVariant } from '../../entity/product-variant/product-variant.ent import { Promotion } from '../../entity/promotion/promotion.entity'; import { Refund } from '../../entity/refund/refund.entity'; import { User } from '../../entity/user/user.entity'; +import { EventBus } from '../../event-bus/event-bus'; +import { OrderStateTransitionEvent } from '../../event-bus/events/order-state-transition-event'; +import { PaymentStateTransitionEvent } from '../../event-bus/events/payment-state-transition-event'; +import { RefundStateTransitionEvent } from '../../event-bus/events/refund-state-transition-event'; import { ListQueryBuilder } from '../helpers/list-query-builder/list-query-builder'; import { OrderCalculator } from '../helpers/order-calculator/order-calculator'; import { OrderMerger } from '../helpers/order-merger/order-merger'; @@ -76,6 +80,7 @@ export class OrderService { private refundStateMachine: RefundStateMachine, private historyService: HistoryService, private promotionService: PromotionService, + private eventBus: EventBus, ) {} findAll(ctx: RequestContext, options?: ListQueryOptions): Promise> { @@ -378,8 +383,10 @@ export class OrderService { async transitionToState(ctx: RequestContext, orderId: ID, state: OrderState): Promise { const order = await this.getOrderOrThrow(ctx, orderId); + const fromState = order.state; await this.orderStateMachine.transition(ctx, order, state); await this.connection.getRepository(Order).save(order, { reload: false }); + this.eventBus.publish(new OrderStateTransitionEvent(fromState, state, ctx, order)); return order; } @@ -423,9 +430,14 @@ export class OrderService { const payment = await getEntityOrThrow(this.connection, Payment, paymentId, { relations: ['order'] }); const settlePaymentResult = await this.paymentMethodService.settlePayment(payment, payment.order); if (settlePaymentResult.success) { - await this.paymentStateMachine.transition(ctx, payment.order, payment, 'Settled'); + const fromState = payment.state; + const toState = 'Settled'; + await this.paymentStateMachine.transition(ctx, payment.order, payment, toState); payment.metadata = { ...payment.metadata, ...settlePaymentResult.metadata }; await this.connection.getRepository(Payment).save(payment, { reload: false }); + this.eventBus.publish( + new PaymentStateTransitionEvent(fromState, toState, ctx, payment, payment.order), + ); if (payment.amount === payment.order.total) { await this.transitionToState(ctx, payment.order.id, 'PaymentSettled'); } @@ -642,8 +654,14 @@ export class OrderService { relations: ['payment', 'payment.order'], }); refund.transactionId = input.transactionId; - await this.refundStateMachine.transition(ctx, refund.payment.order, refund, 'Settled'); - return this.connection.getRepository(Refund).save(refund); + const fromState = refund.state; + const toState = 'Settled'; + await this.refundStateMachine.transition(ctx, refund.payment.order, refund, toState); + await this.connection.getRepository(Refund).save(refund); + this.eventBus.publish( + new RefundStateTransitionEvent(fromState, toState, ctx, refund, refund.payment.order), + ); + return refund; } async addCustomerToOrder(ctx: RequestContext, orderId: ID, customer: Customer): Promise { diff --git a/packages/core/src/service/services/payment-method.service.ts b/packages/core/src/service/services/payment-method.service.ts index 08f8ec7c6f..3dc88a99e6 100644 --- a/packages/core/src/service/services/payment-method.service.ts +++ b/packages/core/src/service/services/payment-method.service.ts @@ -9,7 +9,6 @@ import { Connection } from 'typeorm'; import { RequestContext } from '../../api/common/request-context'; import { UserInputError } from '../../common/error/errors'; import { ListQueryOptions } from '../../common/types/common-types'; -import { getConfig } from '../../config/config-helpers'; import { ConfigService } from '../../config/config.service'; import { PaymentMethodArgs, @@ -17,11 +16,13 @@ import { PaymentMethodHandler, } from '../../config/payment-method/payment-method-handler'; import { OrderItem } from '../../entity/order-item/order-item.entity'; -import { OrderLine } from '../../entity/order-line/order-line.entity'; import { Order } from '../../entity/order/order.entity'; import { PaymentMethod } from '../../entity/payment-method/payment-method.entity'; import { Payment, PaymentMetadata } from '../../entity/payment/payment.entity'; import { Refund } from '../../entity/refund/refund.entity'; +import { EventBus } from '../../event-bus/event-bus'; +import { PaymentStateTransitionEvent } from '../../event-bus/events/payment-state-transition-event'; +import { RefundStateTransitionEvent } from '../../event-bus/events/refund-state-transition-event'; import { ListQueryBuilder } from '../helpers/list-query-builder/list-query-builder'; import { PaymentStateMachine } from '../helpers/payment-state-machine/payment-state-machine'; import { RefundStateMachine } from '../helpers/refund-state-machine/refund-state-machine'; @@ -36,6 +37,7 @@ export class PaymentMethodService { private listQueryBuilder: ListQueryBuilder, private paymentStateMachine: PaymentStateMachine, private refundStateMachine: RefundStateMachine, + private eventBus: EventBus, ) {} async initPaymentMethods() { @@ -78,11 +80,15 @@ export class PaymentMethodService { ): Promise { const { paymentMethod, handler } = await this.getMethodAndHandler(method); const result = await handler.createPayment(order, paymentMethod.configArgs, metadata || {}); + const initialState = 'Created'; const payment = await this.connection .getRepository(Payment) - .save(new Payment({ ...result, state: 'Created' })); + .save(new Payment({ ...result, state: initialState })); await this.paymentStateMachine.transition(ctx, order, payment, result.state); await this.connection.getRepository(Payment).save(payment, { reload: false }); + this.eventBus.publish( + new PaymentStateTransitionEvent(initialState, result.state, ctx, payment, payment.order), + ); return payment; } @@ -126,8 +132,12 @@ export class PaymentMethodService { } refund = await this.connection.getRepository(Refund).save(refund); if (createRefundResult) { + const fromState = refund.state; await this.refundStateMachine.transition(ctx, order, refund, createRefundResult.state); await this.connection.getRepository(Refund).save(refund, { reload: false }); + this.eventBus.publish( + new RefundStateTransitionEvent(fromState, createRefundResult.state, ctx, refund, order), + ); } return refund; }