Stock movement is doubled

[chladog]

Describe the bug
We started tracking inventory and found out that each completed order lowered stock by double the size of actual order contents. I believe this piece of code is to blame:
in order-state-machine.ts

    private async onTransitionEnd(fromState: OrderState, toState: OrderState, data: OrderTransitionData) {
        if (toState === 'PaymentAuthorized' || toState === 'PaymentSettled') {
            data.order.active = false;
            data.order.orderPlacedAt = new Date();
            await this.stockMovementService.createSalesForOrder(data.ctx, data.order);
            await this.promotionService.addPromotionsToOrder(data.ctx, data.order);
        }

To Reproduce
Steps to reproduce the behavior:

• Have an order flow that uses both PaymentAuthroized and PaymentSettled states.
• Track inventory
• Make an order and transition it through these both states
• notice that inventory of those products were lowered by double the amount

Expected behavior
Should lower the stock exactly to the order.
We should be able to define at which point of an order flow should the inventory movement take place.
Also this code should take in account the direction of the transition - if we revert state transition (e.g. "Processing" to "PaymentSettled" it should not do stock movement neither.)

Environment (please complete the following information):

• @vendure/core version: .16.3
• Database (mysql/postgres etc): postgres

[chladog]

additionally export StockMovementService from vendure/core would be good

[michaelbromley]

Thanks for the report!

We should be able to define at which point of an order flow should the inventory movement take place.

In 0.17.0, there is a new concept of "allocated" (part of the new back-order system #319), which occurs where the current Sales stock adjustment takes place. Allocations occur at the same point (and suffer from the same bug currently).

Sales now get created (and therefore stock levels adjusted) only when a Fulfillment is created.

Do you think there is still need for being able to define where these 2 stock movement types get created? If so, could you describe a use-case?

additionally export StockMovementService from vendure/core would be good
👍

[chladog]

We should be able to define at which point of an order flow should the inventory movement take place.

In 0.17.0, there is a new concept of "allocated" (part of the new back-order system #319), which occurs where the current Sales stock adjustment takes place. Allocations occur at the same point (and suffer from the same bug currently).

Sales now get created (and therefore stock levels adjusted) only when a Fulfillment is created.

Do you think there is still need for being able to define where these 2 stock movement types get created? If so, could you describe a use-case?

additionally export StockMovementService from vendure/core would be good
👍

After reading new feature form 0.17.0 I cannot think of any use case where we would want to change point where sale takes place other than Fulfillment.
However with allocation I believe the point where it allocates should be configurable. Our team had discussed this with contradictory opinions - in one opinion would "allocate" (make a reservation) of products in the moment of placing the order before settling payment, another opinion was to have this point only after payment settled.
I believe it depends whether authors want to prioritize first-comers or first-payers.
Maybe even possibility to "allocate" product differently depending on the order (payment) would be handy - as for onsite payment/pickup you need to reserve products right-away because you can't afford to not have the product when the customer comes to pick it up, but for online credit card payment which can be abandoned very easily making the reservation prior paying might be burden.

[michaelbromley]

Maybe even possibility to "allocate" product differently depending on the order (payment) would be handy

We could then add a new property to the config of a PaymentMethodHandler, allocateStockOn: 'PaymentAuthorized' | 'PaymentSettled' | 'auto';. So you can specify where to allocate, or leaving as 'auto' (the default) will allocate stock on either of the 2 states, so long as the fromState was not such that double allocations are possible.

[chladog]

Yes, sounds great, though IMHO for full flexibility also to be possible to set any other (custom) order state.
And exactly, it should be handled not to have multiple allocations of the same order (e.g. also when we revert state)

[michaelbromley]

set any other (custom) order state

I did consider that, e.g. allocateStockOn: OrderState[];, but then it does not make any sense at all for any state that comes after Shipped, so I thought it both simpler and less error-prone to explicitly constrain it. Suggestions welcome for better ways to handle this, of course.

[chladog]

True, I also meant custom states that might be between payment and shipped, or even before arranging payment (e.g. product reservation without payment/choosing payment) - in that case it should not be property at PaymentHandler, but Order

[michaelbromley]

Good point on the custom states prior to payment. Maybe we need a StockAllocationStrategy in OrderOptions, which would essentially make this code block replaceable with custom implementations:

       if (toState === 'PaymentAuthorized' || toState === 'PaymentSettled') {
            data.order.active = false;
            data.order.orderPlacedAt = new Date();
            await this.stockMovementService.createAllocationsForOrder(data.ctx, data.order);
        }

[chladog]

Great! This way we'll have good default behavior, but also will have option to implement advanced one.

BTW the property I see here orderPlacedAt is on Order entity, but is missing in graphql Object type. Could we make it accessible on both admin-api and shop-api? It's handy property for data analytics (e.g. we want to know how many orders been placed in between some dates.). And on shop-api handy for making "Order history" page where will be date of placing the order, not creation. Should I make new feature request for this?

[michaelbromley]

orderPlacedAt is added to the GraphQL type in my local next branch already, and will be in 0.17.0