From df2d7aa0c58c1c7b35b7dacdeecd8233b14ee749 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 21:55:14 +0000 Subject: [PATCH] fix: move inventory when an order ships or is cancelled quantity_reserved went up in TryPlaceAsync and came down in exactly one place, MarkPaymentFailedAsync. Nothing released it along the fulfilment path, so against the real transition table: Paid -> Cancelled kept the reservation, and the stock became permanently unsellable with no route back but an admin edit. Paid -> Shipped decremented nothing, so quantity_on_hand kept counting goods that had left the warehouse and quantity_reserved grew without bound. The shipped case hid itself. Availability is on_hand - reserved, and both stayed wrong by the same amount, so the storefront number looked right while the warehouse number drifted - visible only at a stock count. UpdateStatusAsync now takes the transitioned order and writes the status together with the movement it implies, in one transaction: shipping turns the reservation into a real decrement of both columns, cancelling releases the hold, delivery moves nothing because shipping already did. Splitting the two writes would let a crash between them leave a shipped order still holding its stock, which is the drift being fixed. The guards keep both columns off negative, and MarkPaymentFailedAsync now shares the release statement instead of repeating it. The in-memory fake mirrors the same movement, skipping widgets absent from its store so existing lifecycle tests are unaffected. Five tests cover it: ship, cancel and deliver against real Postgres, and the handler path for ship and cancel. Not verified locally - this container has no .NET SDK and no Postgres, so CI is the first execution of any of it. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Abstractions/IOrderRepository.cs | 8 ++- .../UpdateStatus/UpdateOrderStatusHandler.cs | 2 +- .../Persistence/OrderRepository.cs | 62 +++++++++++++++++-- .../OrderRepositoryTests.cs | 60 +++++++++++++++++- tests/WidgetWorks.UnitTests/Fakes.cs | 31 ++++++++-- .../OrderLifecycleTests.cs | 56 +++++++++++++++++ 6 files changed, 205 insertions(+), 14 deletions(-) diff --git a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs index 372f362..927ec5d 100644 --- a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs +++ b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs @@ -15,7 +15,13 @@ public interface IOrderRepository /// Marks the order failed and releases its inventory reservations. Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct); - Task UpdateStatusAsync(Guid orderId, string status, string? trackingNumber, DateTimeOffset now, CancellationToken ct); + /// + /// Persists a fulfilment transition together with the inventory movement it implies, in one + /// transaction: shipping converts the reservation into a real stock decrement, cancelling + /// releases it back. Pass the order after has run - the new + /// status on it decides the movement. + /// + Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct); Task GetByIdAsync(Guid id, CancellationToken ct); diff --git a/src/WidgetWorks.Application/Orders/UpdateStatus/UpdateOrderStatusHandler.cs b/src/WidgetWorks.Application/Orders/UpdateStatus/UpdateOrderStatusHandler.cs index 79985b8..c956c7a 100644 --- a/src/WidgetWorks.Application/Orders/UpdateStatus/UpdateOrderStatusHandler.cs +++ b/src/WidgetWorks.Application/Orders/UpdateStatus/UpdateOrderStatusHandler.cs @@ -36,7 +36,7 @@ public async Task> Handle(UpdateOrderStatusCommand command, Ca var now = clock.GetUtcNow(); order.TransitionTo(target, command.TrackingNumber, now); - await orders.UpdateStatusAsync(order.Id, order.Status, order.TrackingNumber, now, ct); + await orders.UpdateStatusAsync(order, now, ct); try { diff --git a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs index 55b3620..6abbd5a 100644 --- a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs +++ b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs @@ -24,6 +24,22 @@ public sealed class OrderRepository(IDbConnectionFactory factory) : IOrderReposi @"update widgets set quantity_reserved = quantity_reserved + @Quantity, updated_at = @Now where id = @WidgetId and (quantity_on_hand - quantity_reserved) >= @Quantity"; + /// Hands a reservation back: the goods never left, so only the hold is undone. + private const string ReleaseSql = + @"update widgets set quantity_reserved = quantity_reserved - @Quantity, updated_at = @Now + where id = @WidgetId and quantity_reserved >= @Quantity"; + + /// + /// Turns a reservation into a real decrement when the parcel leaves. Both columns fall by the + /// same amount, so availability (on_hand - reserved) is unchanged and the on-hand figure starts + /// telling the truth about what is on the shelf. The guards keep either column off negative. + /// + private const string ShipSql = + @"update widgets set quantity_on_hand = quantity_on_hand - @Quantity, + quantity_reserved = quantity_reserved - @Quantity, + updated_at = @Now + where id = @WidgetId and quantity_reserved >= @Quantity and quantity_on_hand >= @Quantity"; + public async Task TryPlaceAsync(Order order, CancellationToken ct) { using var db = await factory.OpenAsync(ct); @@ -88,8 +104,7 @@ await db.ExecuteAsync(new CommandDefinition( foreach (var item in order.Items) { await db.ExecuteAsync(new CommandDefinition( - "update widgets set quantity_reserved = quantity_reserved - @Quantity, updated_at = @Now where id = @WidgetId and quantity_reserved >= @Quantity", - new { item.WidgetId, item.Quantity, Now = now }, tx, cancellationToken: ct)); + ReleaseSql, new { item.WidgetId, item.Quantity, Now = now }, tx, cancellationToken: ct)); } tx.Commit(); @@ -101,12 +116,47 @@ await db.ExecuteAsync(new CommandDefinition( } } - public async Task UpdateStatusAsync(Guid orderId, string status, string? trackingNumber, DateTimeOffset now, CancellationToken ct) + public async Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct) { + // Status and stock move together or not at all. Splitting them would let a crash between + // the two leave a shipped order whose goods are still reserved, which is exactly the drift + // this method exists to stop. using var db = await factory.OpenAsync(ct); - await db.ExecuteAsync( - "update orders set status = @Status, tracking_number = @Tracking, updated_at = @Now where id = @Id", - new { Id = orderId, Status = status, Tracking = trackingNumber, Now = now }); + using var tx = db.BeginTransaction(); + try + { + await db.ExecuteAsync(new CommandDefinition( + "update orders set status = @Status, tracking_number = @Tracking, updated_at = @Now where id = @Id", + new { Id = order.Id, Status = order.Status, Tracking = order.TrackingNumber, Now = now }, + tx, cancellationToken: ct)); + + // Shipping turns a reservation into a real decrement; cancelling hands it back. + // Delivered moves no stock - shipping already did. + // Explicit string? rather than var: a switch expression mixing string arms with a + // null arm has no best common type to infer. + string? sql = order.Status switch + { + OrderStatus.Shipped => ShipSql, + OrderStatus.Cancelled => ReleaseSql, + _ => null, + }; + + if (sql is not null) + { + foreach (var item in order.Items) + { + await db.ExecuteAsync(new CommandDefinition( + sql, new { item.WidgetId, item.Quantity, Now = now }, tx, cancellationToken: ct)); + } + } + + tx.Commit(); + } + catch + { + tx.Rollback(); + throw; + } } public async Task GetByIdAsync(Guid id, CancellationToken ct) diff --git a/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs b/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs index 1017609..c558462 100644 --- a/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs +++ b/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs @@ -235,13 +235,71 @@ public async Task Updating_status_stores_the_tracking_number() await Orders.TryPlaceAsync(order, CancellationToken.None); await Orders.MarkPaidAsync(order.Id, "Mock", "r", Now, CancellationToken.None); - await Orders.UpdateStatusAsync(order.Id, OrderStatus.Shipped, "1Z-TRACK", Now.AddHours(1), CancellationToken.None); + // Set directly rather than via TransitionTo: MarkPaidAsync moved the row, not this + // in-memory instance, so the entity would refuse Pending -> Shipped. + order.Status = OrderStatus.Shipped; + order.TrackingNumber = "1Z-TRACK"; + await Orders.UpdateStatusAsync(order, Now.AddHours(1), CancellationToken.None); var stored = await Orders.GetByIdAsync(order.Id, CancellationToken.None); Assert.Equal(OrderStatus.Shipped, stored!.Status); Assert.Equal("1Z-TRACK", stored.TrackingNumber); } + [Fact] + public async Task Shipping_turns_the_reservation_into_a_real_decrement() + { + var widget = await GivenWidget(onHand: 5); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + + var reserved = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(5, reserved!.QuantityOnHand); + Assert.Equal(2, reserved.QuantityReserved); + + order.Status = OrderStatus.Shipped; + await Orders.UpdateStatusAsync(order, Now.AddHours(1), CancellationToken.None); + + // The goods left the shelf: on-hand falls and the hold is gone, so availability + // (on_hand - reserved) is unchanged at 3 while on-hand now tells the truth. + var shipped = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(3, shipped!.QuantityOnHand); + Assert.Equal(0, shipped.QuantityReserved); + } + + [Fact] + public async Task Cancelling_hands_the_reservation_back() + { + var widget = await GivenWidget(onHand: 5); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + + order.Status = OrderStatus.Cancelled; + await Orders.UpdateStatusAsync(order, Now.AddHours(1), CancellationToken.None); + + // Nothing shipped, so the stock returns to sale in full. + var cancelled = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(5, cancelled!.QuantityOnHand); + Assert.Equal(0, cancelled.QuantityReserved); + } + + [Fact] + public async Task Delivery_moves_no_stock_because_shipping_already_did() + { + var widget = await GivenWidget(onHand: 5); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + + order.Status = OrderStatus.Shipped; + await Orders.UpdateStatusAsync(order, Now.AddHours(1), CancellationToken.None); + order.Status = OrderStatus.Delivered; + await Orders.UpdateStatusAsync(order, Now.AddHours(2), CancellationToken.None); + + var delivered = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(3, delivered!.QuantityOnHand); + Assert.Equal(0, delivered.QuantityReserved); + } + [Fact] public async Task A_users_orders_come_back_newest_first_with_their_lines() { diff --git a/tests/WidgetWorks.UnitTests/Fakes.cs b/tests/WidgetWorks.UnitTests/Fakes.cs index cca37d7..d9520f1 100644 --- a/tests/WidgetWorks.UnitTests/Fakes.cs +++ b/tests/WidgetWorks.UnitTests/Fakes.cs @@ -235,12 +235,33 @@ public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset no return Task.CompletedTask; } - public Task UpdateStatusAsync(Guid orderId, string status, string? trackingNumber, DateTimeOffset now, CancellationToken ct) + public Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct) { - var order = Orders.First(o => o.Id == orderId); - order.Status = status; - order.TrackingNumber = trackingNumber; - order.UpdatedAt = now; + var stored = Orders.First(o => o.Id == order.Id); + stored.Status = order.Status; + stored.TrackingNumber = order.TrackingNumber; + stored.UpdatedAt = now; + + // Mirrors OrderRepository: shipping converts the reservation into a real + // decrement, cancelling hands it back, delivery moves nothing. + foreach (var item in order.Items) + { + if (!widgets.Store.TryGetValue(item.WidgetId, out var w)) + { + continue; + } + + if (order.Status == OrderStatus.Shipped) + { + w.QuantityOnHand -= item.Quantity; + w.QuantityReserved -= item.Quantity; + } + else if (order.Status == OrderStatus.Cancelled) + { + w.QuantityReserved -= item.Quantity; + } + } + return Task.CompletedTask; } diff --git a/tests/WidgetWorks.UnitTests/OrderLifecycleTests.cs b/tests/WidgetWorks.UnitTests/OrderLifecycleTests.cs index 4d7b864..a69e831 100644 --- a/tests/WidgetWorks.UnitTests/OrderLifecycleTests.cs +++ b/tests/WidgetWorks.UnitTests/OrderLifecycleTests.cs @@ -2,6 +2,7 @@ using Microsoft.Extensions.Time.Testing; using WidgetWorks.Application.Abstractions; using WidgetWorks.Application.Orders.UpdateStatus; +using WidgetWorks.Domain.Catalog; using WidgetWorks.Domain.Orders; using WidgetWorks.UnitTests.Fakes; using Xunit; @@ -22,6 +23,61 @@ private static (InMemoryOrderRepository Orders, FakeEmailSender Email, Order Ord return (orders, new FakeEmailSender(), order); } + /// + /// As , but the widget is really in stock with the order's units held, + /// so a transition's effect on inventory is observable. + /// + private static (InMemoryOrderRepository Orders, InMemoryWidgetRepository Widgets, Order Order, Guid WidgetId) StockedSetup(int onHand = 5, int quantity = 2) + { + var widgets = new InMemoryWidgetRepository(); + var orders = new InMemoryOrderRepository(widgets); + var widgetId = Guid.NewGuid(); + widgets.Store[widgetId] = new Widget + { + Id = widgetId, + Sku = "WW-1", + Name = "Gizmo", + Price = 10m, + IsActive = true, + QuantityOnHand = onHand, + QuantityReserved = quantity, + }; + + var order = new Order { Id = Guid.NewGuid(), OrderNumber = "WW-1", Email = "jane@example.com", Status = OrderStatus.Paid, Total = 10m }; + order.Items.Add(new OrderItem { Id = Guid.NewGuid(), WidgetId = widgetId, Sku = "WW-1", Name = "Gizmo", UnitPrice = 10m, Quantity = quantity, LineSubtotal = 10m }); + orders.Orders.Add(order); + return (orders, widgets, order, widgetId); + } + + [Fact] + public async Task Shipping_converts_the_reservation_into_a_stock_decrement() + { + var (orders, widgets, order, widgetId) = StockedSetup(); + var handler = new UpdateOrderStatusHandler(orders, new FakeEmailSender(), Clock(), NullLogger.Instance); + + var result = await handler.Handle(new UpdateOrderStatusCommand(order.Id, OrderStatus.Shipped, "1Z999"), CancellationToken.None); + + Assert.True(result.IsSuccess); + // Both columns fall together, so availability is untouched while on-hand stops + // overstating what is physically on the shelf. + Assert.Equal(3, widgets.Store[widgetId].QuantityOnHand); + Assert.Equal(0, widgets.Store[widgetId].QuantityReserved); + } + + [Fact] + public async Task Cancelling_returns_the_reserved_stock_to_sale() + { + var (orders, widgets, order, widgetId) = StockedSetup(); + var handler = new UpdateOrderStatusHandler(orders, new FakeEmailSender(), Clock(), NullLogger.Instance); + + var result = await handler.Handle(new UpdateOrderStatusCommand(order.Id, OrderStatus.Cancelled, null), CancellationToken.None); + + Assert.True(result.IsSuccess); + // Nothing shipped: the hold is released and every unit is sellable again. + Assert.Equal(5, widgets.Store[widgetId].QuantityOnHand); + Assert.Equal(0, widgets.Store[widgetId].QuantityReserved); + } + [Fact] public async Task Paid_to_shipped_sets_tracking_and_emails() {