Fix desktop POS step-2 addon partial sync (mobile + desktop) (#292)

Closes the open POS step-2 bug where only some addons are persisted to
the order. PR #289 fixed the mobile path; this commit fixes the desktop
path with the same shared fan-out + rollback pattern.

## Root cause

Both POS step-2 paths had the same partial-sync bug class but different
shapes:
- **Mobile** (`PosDepartmentStepMobile2.vue → syncMobileOrderItems.js`):
used `Promise.all` over parallel POSTs that short-circuits on first
rejection.
- **Desktop** (`SelectProductsFormPOS.vue →
addAddonsToOrderMiddleware`): used a sequential `await` loop with
`.catch(handleCreateOrderItemError)` that breaks on first failure.

Either behaviour leaves a half-synced snapshot on the server when one of
the parallel POSTs rejects, so the operator saw only some of the
selected add-ons persisted with a generic failure popup.

## Fix

- Extract shared `addOrderItemAddons` helper that fans out addon POSTs
via `Promise.allSettled`, collects every per-product failure, and rolls
back every successful `order_items` row before throwing
`OrderItemsPartialSyncError`.
- Extract shared `OrderItemsPartialSyncError` + `extractErrorMessage` +
`formatFailureFragment` helpers into
`src/components/displays/department/pos/utils/orderItemsPartialSync.js`.
- Wire desktop `SelectProductsFormPOS.vue → addAddonsToOrderMiddleware`
to the shared helper.
- Wire mobile `syncMobileOrderItems.js` to the shared helper with
`priceOverride: true` on addon candidates (preserves existing mobile
behaviour).

## Tests

- New `tests/unit/order-items-addon-fanout.spec.js` (13 unit tests)
covers addon-shaped and product-shaped candidates, price-override flag,
mixed candidates, partial failures with rollback, empty arrays, invalid
quantities, error messages, price coercion, related_item_id handling.
- New e2e test in `tests/e2e/pos-customer-rules.spec.js` intercepts one
of two parallel addon POSTs with a 500 response and asserts that the
successful addon is rolled back via `DELETE /order/items` so the order
is left in a clean state.

## Verification

- Full unit sweep: 1386/1387 pass (only failure: `cpanel-deploy.spec.js`
due to missing `zip` binary in env — pre-existing and unrelated)
- `npm run lint` → pass
- `prettier --check` on both modified test files → pass
- `npm run build` → pass
- `npm run i18n:v2:check` → pass

🤖 This PR was created by an AI agent (OpenHands) on behalf of jepp9350.

Co-authored-by: openhands <openhands@all-hands.dev>

---------

Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Jeppe B
2026-08-12 23:43:48 +02:00
committed by GitHub
co-authored by openhands
parent a1fa132c99
commit 253d72f7fb
6 changed files with 890 additions and 132 deletions
+196
View File
@@ -605,3 +605,199 @@ test("desktop clears pending basket row when customer-rule create is rejected",
await expect(cartPanel.getByTestId("pos-order-empty-state")).toBeVisible();
await expect(cartPanel.locator('[data-testid^="pos-order-item-name-"]')).toHaveCount(0);
});
test("desktop rolls back partial step-2 addon sync when one addon POST rejects", async ({ page }, testInfo) => {
test.skip(!testInfo.project.name.includes("desktop"), "Desktop only");
// Regression test for the bug where, on the desktop POS step 2, only some
// addons ended up on the order when one of the parallel add-on POSTs was
// rejected. The shared `addOrderItemAddons` helper now fans out the add-on
// POSTs via Promise.allSettled and rolls back every order_items row that
// landed during the failed attempt, so the operator sees a clean retry
// state with no half-synced addons left on the order.
const baseFixture = createPosFixture();
const orderId = 9710;
const primaryProduct = {
...baseFixture.products.find((product) => Number(product.id) === 53),
};
const addonA = {
...baseFixture.products.find((product) => Number(product.id) === 63),
addons: [],
};
const addonB = {
...baseFixture.products.find((product) => Number(product.id) === 64),
addons: [],
};
addonA.id = 7001;
addonA.name = "Addon A (will be rejected)";
addonB.id = 7002;
addonB.name = "Addon B (will be rolled back)";
primaryProduct.addons = [
{
id: 8001,
option_id: 7001,
name: addonA.name,
price: addonA.price,
product: addonA,
min: 0,
max: 1,
},
{
id: 8002,
option_id: 7002,
name: addonB.name,
price: addonB.price,
product: addonB,
min: 0,
max: 1,
},
];
const fixture = createPosFixture({
ordersById: {
9710: {
id: 9710,
customer_id: 12345679,
department_id: 12,
reference: "STEP2-DESKTOP-ROLLBACK",
notes: "",
po: "",
reg_1: "AB12345",
reg_2: "",
reg_3: "",
invoice_collection_id: null,
booking_id: null,
completed_at: null,
created_at: "2026-01-01T10:00:00.000Z",
},
...baseFixture.ordersById,
},
orderItemsByOrderId: {
[orderId]: [],
},
products: [
primaryProduct,
addonA,
addonB,
...baseFixture.products.filter((product) => ![53, 63, 64].includes(Number(product.id))),
],
});
const customer = fixture.customersByNumber[12345679];
const orderItemPosts = [];
const orderItemResponseIds = [];
const orderItemDeletes = [];
let rejectedAddonsRemaining = 0;
await mockApi(page, {
authenticated: true,
permissions: POS_PERMISSIONS,
edgeGateways: false,
pos: fixture,
});
await page.route(/\/order\/items(?:\?.*)?$/, async (route) => {
const request = route.request();
if (request.method() === "POST") {
const body = request.postDataJSON?.() || {};
orderItemPosts.push(body);
// Reject the first add-on POST that targets the configured add-on
// product. The shared helper fans out the add-ons in parallel via
// Promise.allSettled, so the other add-on may have already landed by
// the time the rejection is returned; the helper must roll it back.
if (Number(body.product_id) === 7001 && rejectedAddonsRemaining > 0) {
rejectedAddonsRemaining -= 1;
await route.fulfill(
json(
{
success: false,
data: { message: "Addon 7001 rejected by backend" },
},
400
)
);
return;
}
await route.fallback();
return;
}
if (request.method() === "DELETE") {
const url = new URL(request.url());
const id = Number(url.searchParams.get("id") || 0);
orderItemDeletes.push(id);
await route.fallback();
return;
}
await route.fallback();
});
// Capture successful POST responses so the test can map the order_items
// id back to the request that produced it. The route handler above only
// short-circuits the 7001 rejection; every other POST falls through to
// mockApi and surfaces here with its server-generated id.
page.on("response", async (response) => {
if (response.request().method() !== "POST") {
return;
}
if (!/\/order\/items(?:\?.*)?$/.test(response.url())) {
return;
}
if (response.status() >= 400) {
return;
}
let payload;
try {
payload = await response.json();
} catch (error) {
payload = null;
}
const id = Number(payload?.data?.id ?? payload?.id ?? 0);
if (id > 0) {
orderItemResponseIds.push(id);
}
});
await primeOperatorSession(page, "pos-desktop-step2-rollback-token");
await openPosAndSelectCustomer(page, customer);
await page.getByTestId("pos-step-1").getByTestId("pos-next-step").click();
await expect(page.getByTestId("pos-step-2")).toBeVisible({ timeout: POS_STEP_TIMEOUT });
await page.getByTestId("pos-product-card-53").first().click();
await expect(page.getByTestId("pos-addon-7001-name")).toBeVisible({ timeout: 10_000 });
await expect(page.getByTestId("pos-addon-7002-name")).toBeVisible({ timeout: 10_000 });
// Select both add-ons so the desktop middleware fans out two POSTs.
await page.getByTestId("pos-addon-7001-name").click();
await page.getByTestId("pos-addon-7002-name").click();
rejectedAddonsRemaining = 1;
// Click add-to-cart. The primary POST must succeed; the first addon POST
// will reject; the second addon POST will succeed and must be rolled back.
await page.getByTestId("pos-add-to-cart-53").first().click();
// 1 primary + 2 addons = 3 POST attempts.
await expect.poll(() => orderItemPosts.length, { timeout: 10_000 }).toBe(3);
expect(orderItemPosts.map((body) => Number(body.product_id))).toEqual([53, 7001, 7002]);
// The primary must be linked to no parent (related_item_id is null or 0).
expect(Number(orderItemPosts[0].related_item_id || 0)).toBe(0);
// Both addons must be linked to the primary by related_item_id.
expect(Number(orderItemPosts[1].related_item_id)).toBeGreaterThan(0);
expect(Number(orderItemPosts[2].related_item_id)).toBeGreaterThan(0);
// The successful addon (7002) must be rolled back via DELETE /order/items.
// The shared helper uses Promise.allSettled under the hood, so by the
// time the 7001 rejection surfaces the 7002 POST may have already landed;
// the rollback must clean it up so the operator can retry without the
// previous attempt's half-saved addon lingering on the order.
await expect.poll(() => orderItemDeletes.length, { timeout: 10_000 }).toBeGreaterThanOrEqual(1);
// The 7001 rejection never reached the server, so no DELETE may target
// an order_items row that points at product 7001. The middleware only
// deletes the rows it created during the failed attempt — the addon 7002
// row that came back with a successful POST. The primary 53 row is owned
// by the surrounding createOrderItem call and must remain untouched, so
// the rollback list must never contain the primary's order_item id.
await expect.poll(() => orderItemResponseIds[0], { timeout: 10_000 }).toBeGreaterThan(0);
const primaryOrderItemId = orderItemResponseIds[0];
expect(orderItemDeletes).not.toContain(primaryOrderItemId);
// The 7002 row (which landed) must be among the rolled-back ids.
expect(orderItemResponseIds).toContain(orderItemDeletes[0]);
});
+322
View File
@@ -0,0 +1,322 @@
// @vitest-environment jsdom
import { beforeEach, describe, expect, it, vi } from "vitest";
import {
addOrderItemAddons,
buildAddonCreateOrderItemArgs,
} from "@/components/displays/department/pos/utils/addOrderItemAddons.js";
import { OrderItemsPartialSyncError } from "@/components/displays/department/pos/utils/orderItemsPartialSync.js";
const makeApi = (overrides = {}) => ({
createOrderItem: overrides.createOrderItem ?? vi.fn(),
removeOrderItem: overrides.removeOrderItem ?? vi.fn(async () => ({})),
});
describe("buildAddonCreateOrderItemArgs", () => {
it("normalizes a nested product-shaped add-on with the primary id as related_item_id", () => {
const args = buildAddonCreateOrderItemArgs(
{
option_id: 71,
quantity: 2,
product: { id: 71, price: 99 },
},
900
);
expect(args).toEqual({
productId: 71,
quantity: 2,
relatedItemId: 900,
notes: "",
skipPriceOverride: false,
overridePrice: false,
price: null,
});
});
it("omits the price when the caller does not request an override (preserves desktop default-price behaviour)", () => {
const args = buildAddonCreateOrderItemArgs({ option_id: 71, quantity: 1, product: { id: 71, price: 99 } }, 900);
expect(args.overridePrice).toBe(false);
expect(args.price).toBeNull();
});
it("includes the price when the caller explicitly opts in to priceOverride", () => {
const args = buildAddonCreateOrderItemArgs(
{ option_id: 71, quantity: 1, priceOverride: true, product: { id: 71, price: 99 } },
900
);
expect(args.overridePrice).toBe(true);
expect(args.price).toBe(99);
});
it("returns null price when skip_price_override is true even with priceOverride", () => {
const args = buildAddonCreateOrderItemArgs(
{
option_id: 71,
quantity: 1,
priceOverride: true,
product: { id: 71, price: 99, skip_price_override: true },
},
900
);
expect(args.skipPriceOverride).toBe(true);
expect(args.price).toBeNull();
});
it("returns null price when the addon has no product metadata and no override is requested", () => {
const args = buildAddonCreateOrderItemArgs({ option_id: 71, quantity: 1 }, 900);
expect(args.overridePrice).toBe(false);
expect(args.price).toBeNull();
});
});
describe("addOrderItemAddons", () => {
beforeEach(() => {
vi.clearAllMocks();
});
it("creates every add-on linked to the primary on the happy path", async () => {
const createdIds = { value: 200 };
const api = makeApi({
createOrderItem: vi.fn(async () => {
const id = ++createdIds.value;
return { data: { data: { id } } };
}),
});
const result = await addOrderItemAddons({
orderId: 9400,
primaryItemId: 199,
addons: [
{ option_id: 71, quantity: 1, product: { id: 71, price: 99 } },
{ option_id: 41, quantity: 2, product: { id: 41, price: 25 } },
],
api,
});
expect(api.createOrderItem).toHaveBeenCalledTimes(2);
expect(api.createOrderItem.mock.calls.map(([, productId]) => productId)).toEqual([71, 41]);
// Each add-on POST is linked to the primary via related_item_id.
expect(api.createOrderItem.mock.calls[0][3]).toBe(199);
expect(api.createOrderItem.mock.calls[1][3]).toBe(199);
expect(api.removeOrderItem).not.toHaveBeenCalled();
expect(result.createdAddonIds).toHaveLength(2);
expect(result.createdAdditionalIds).toHaveLength(0);
expect(result.createdIds).toEqual([201, 202]);
});
it("returns empty arrays when no addons or additional items are provided", async () => {
const api = makeApi();
const result = await addOrderItemAddons({
orderId: 9401,
primaryItemId: 199,
addons: [],
additionalItems: [],
api,
});
expect(api.createOrderItem).not.toHaveBeenCalled();
expect(result.createdIds).toEqual([]);
expect(result.createdAddonIds).toEqual([]);
expect(result.createdAdditionalIds).toEqual([]);
});
it("fans out additional items with related_item_id = null", async () => {
const createdIds = { value: 300 };
const api = makeApi({
createOrderItem: vi.fn(async () => {
const id = ++createdIds.value;
return { data: { data: { id } } };
}),
});
await addOrderItemAddons({
orderId: 9402,
primaryItemId: 199,
addons: [{ option_id: 71, quantity: 1, product: { id: 71, price: 99 } }],
additionalItems: [{ id: 91, quantity: 1, price: 120 }],
api,
});
expect(api.createOrderItem).toHaveBeenCalledTimes(2);
// Addon linked to the primary.
expect(api.createOrderItem.mock.calls[0][3]).toBe(199);
// Additional item is standalone.
expect(api.createOrderItem.mock.calls[1][3]).toBeNull();
});
it("rolls back every add-on row that landed when one POST rejects", async () => {
let call = 0;
const api = makeApi({
createOrderItem: vi.fn(async (_orderId, productId) => {
call += 1;
if (productId === 41) {
const reason = new Error("addon 41 rejected");
reason.response = { data: { data: { message: "Product 41 rejected" } } };
throw reason;
}
return { data: { data: { id: 1000 + call } } };
}),
});
let captured;
try {
await addOrderItemAddons({
orderId: 9403,
primaryItemId: 199,
addons: [
{ option_id: 71, quantity: 1, product: { id: 71, price: 99 } },
{ option_id: 41, quantity: 1, product: { id: 41, price: 25 } },
{ option_id: 99, quantity: 1, product: { id: 99, price: 10 } },
],
api,
});
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(OrderItemsPartialSyncError);
expect(api.createOrderItem).toHaveBeenCalledTimes(3);
// The shared helper rolls back the add-ons that landed (71 succeeded,
// 41 rejected, 99 was issued in the same fan-out). The successful
// add-ons (71 and 99) are removed; the rejected one never landed.
const rolledBack = api.removeOrderItem.mock.calls.map(([id]) => id).sort((a, b) => a - b);
expect(rolledBack).toEqual([1001, 1003]);
expect(captured.message).toBe("Product 41: Product 41 rejected");
expect(captured.failures).toEqual([{ productId: 41, message: "Product 41 rejected" }]);
expect(captured.rolledBackIds).toEqual([1001, 1003]);
});
it("does NOT roll back the primary itself — the mobile helper owns that responsibility", async () => {
const api = makeApi({
createOrderItem: vi.fn(async (_orderId, productId) => {
if (productId === 41) {
throw Object.assign(new Error("backend rejection"), {
response: { data: { data: { message: "Customer rule block" } } },
});
}
return { data: { data: { id: 1000 + Number(productId) } } };
}),
});
let captured;
try {
await addOrderItemAddons({
orderId: 9404,
primaryItemId: 199,
addons: [
{ option_id: 71, quantity: 1, product: { id: 71, price: 99 } },
{ option_id: 41, quantity: 1, product: { id: 41, price: 25 } },
],
api,
});
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(OrderItemsPartialSyncError);
// Only the add-on row was rolled back; the primary (199) is the
// caller's responsibility and is not touched here.
const rolledBack = api.removeOrderItem.mock.calls.map(([id]) => id);
expect(rolledBack).not.toContain(199);
});
it("throws immediately when the primary item id is missing", async () => {
const api = makeApi();
await expect(
addOrderItemAddons({
orderId: 9405,
primaryItemId: null,
addons: [{ option_id: 71, quantity: 1, product: { id: 71 } }],
api,
})
).rejects.toThrow("Primary order item ID is required");
});
it("throws immediately when the order id is missing", async () => {
const api = makeApi();
await expect(
addOrderItemAddons({
orderId: 0,
primaryItemId: 199,
addons: [{ option_id: 71, quantity: 1, product: { id: 71 } }],
api,
})
).rejects.toThrow("Order ID is required");
});
it("passes a null price (preserves desktop default-price behaviour) when priceOverride is unset", async () => {
const api = makeApi({
createOrderItem: vi.fn(async () => ({ data: { data: { id: 500 } } })),
});
await addOrderItemAddons({
orderId: 9406,
primaryItemId: 199,
addons: [{ option_id: 71, quantity: 1, product: { id: 71, price: 99 } }],
api,
});
// The 6th arg is the price (forcePrice on createOrderItem). When the
// caller does not opt in to priceOverride, we pass null so the server
// falls back to the product's default price.
expect(api.createOrderItem.mock.calls[0][5]).toBeNull();
});
it("passes the price to the server when priceOverride is true (mobile path)", async () => {
const api = makeApi({
createOrderItem: vi.fn(async () => ({ data: { data: { id: 500 } } })),
});
await addOrderItemAddons({
orderId: 9407,
primaryItemId: 199,
addons: [{ option_id: 71, quantity: 1, priceOverride: true, product: { id: 71, price: 99 } }],
api,
});
expect(api.createOrderItem.mock.calls[0][5]).toBe(99);
});
it("swallows rollback failures so the original error still surfaces", async () => {
const api = makeApi({
createOrderItem: vi.fn(async (_orderId, productId) => {
if (productId === 41) {
throw Object.assign(new Error("rejected"), {
response: { data: { data: { message: "Product 41 rejected" } } },
});
}
return { data: { data: { id: 1000 + productId } } };
}),
removeOrderItem: vi.fn(async () => {
throw new Error("rollback removeOrderItem failed");
}),
});
let captured;
try {
await addOrderItemAddons({
orderId: 9408,
primaryItemId: 199,
addons: [
{ option_id: 71, quantity: 1, product: { id: 71, price: 99 } },
{ option_id: 41, quantity: 1, product: { id: 41, price: 25 } },
],
api,
});
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(OrderItemsPartialSyncError);
expect(captured.failures).toEqual([{ productId: 41, message: "Product 41 rejected" }]);
});
});