From 253d72f7fbbea95164143800578e634c2f7eec57 Mon Sep 17 00:00:00 2001 From: Jeppe B <2jepp9350@gmail.com> Date: Wed, 12 Aug 2026 23:43:48 +0200 Subject: [PATCH] Fix desktop POS step-2 addon partial sync (mobile + desktop) (#292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --------- Co-authored-by: openhands --- .../mobile/utils/syncMobileOrderItems.js | 165 +++------ .../pos/utils/addOrderItemAddons.js | 204 +++++++++++ .../pos/utils/orderItemsPartialSync.js | 56 +++ .../department/pos/SelectProductsFormPOS.vue | 79 +++-- tests/e2e/pos-customer-rules.spec.js | 196 +++++++++++ tests/unit/order-items-addon-fanout.spec.js | 322 ++++++++++++++++++ 6 files changed, 890 insertions(+), 132 deletions(-) create mode 100644 src/components/displays/department/pos/utils/addOrderItemAddons.js create mode 100644 src/components/displays/department/pos/utils/orderItemsPartialSync.js create mode 100644 tests/unit/order-items-addon-fanout.spec.js diff --git a/src/components/displays/department/pos/steps/mobile/utils/syncMobileOrderItems.js b/src/components/displays/department/pos/steps/mobile/utils/syncMobileOrderItems.js index e3f078b9..867f3d75 100644 --- a/src/components/displays/department/pos/steps/mobile/utils/syncMobileOrderItems.js +++ b/src/components/displays/department/pos/steps/mobile/utils/syncMobileOrderItems.js @@ -12,11 +12,27 @@ * - The old "Promise.all over parallel POSTs" fan-out silently dropped * rows on a single rejection: the rows that already landed stayed on * the server while the operator saw only a generic failure popup. - * This helper uses Promise.allSettled, collects every per-product - * failure, and rolls back every order_items row created during this - * attempt before throwing, so a retry starts from a clean state. + * This helper delegates the add-on / additional-item fan-out to the + * shared `addOrderItemAddons` helper, which uses Promise.allSettled, + * collects every per-product failure, and rolls back every + * order_items row created during this attempt before throwing, so a + * retry starts from a clean state. + * + * The shared error class + helpers live in + * `src/components/displays/department/pos/utils/orderItemsPartialSync.js` + * so the desktop `addAddonsToOrderMiddleware` can throw the same shape. */ +import { + OrderItemsPartialSyncError, + extractErrorMessage, + formatFailureFragment, +} from "@/components/displays/department/pos/utils/orderItemsPartialSync.js"; +import { addOrderItemAddons } from "@/components/displays/department/pos/utils/addOrderItemAddons.js"; + +// Re-export for back-compat with existing tests / call sites. +export { OrderItemsPartialSyncError, extractErrorMessage, formatFailureFragment }; + const PLACEHOLDER_PRIMARY_RELATED_ITEM_ID = "__PRIMARY__"; const toInteger = (value) => { @@ -191,40 +207,6 @@ const arraysEqual = (left, right) => { return true; }; -/** - * Map an axios-like rejection to the human-readable string the operator - * should see. Falls back to the error message itself. - */ -export const extractErrorMessage = (reason) => { - const message = - reason?.response?.data?.data?.message ?? reason?.response?.data?.message ?? reason?.message ?? "Unknown error"; - return String(message); -}; - -/** - * Format a per-product failure into a human-readable fragment. - */ -export const formatFailureFragment = ({ productId, message }) => { - if (!Number.isFinite(Number(productId)) || Number(productId) <= 0) { - return message; - } - return `Product ${productId}: ${message}`; -}; - -/** - * Custom error thrown when one or more order_items POSTs fail. Carries the - * aggregated message and the per-product failure list so the UI layer can - * either display it directly or surface structured details. - */ -export class OrderItemsPartialSyncError extends Error { - constructor(message, failures, rolledBackIds) { - super(message); - this.name = "OrderItemsPartialSyncError"; - this.failures = failures; - this.rolledBackIds = rolledBackIds; - } -} - /** * @typedef {Object} OrderItemApi * @property {(orderId, productId, quantity, relatedItemId, notes, price) => Promise<{data:{data:{id:number}}}>} createOrderItem @@ -310,81 +292,44 @@ export const syncMobileOrderItems = async ({ } const createdPrimaryItemId = toPositiveInteger(primaryCreate?.data?.data?.id); - const addonCandidates = (Array.isArray(primaryItem.addons) ? primaryItem.addons : []).filter( - (addon) => Number(addon?.quantity ?? 0) > 0 && !isAddonRestricted(addon) - ); - const additionalCandidates = (Array.isArray(additionalItems) ? additionalItems : []).filter( - (item) => Number(item?.quantity ?? 0) > 0 && !isStandaloneRestricted(item) - ); + const addonCandidates = (Array.isArray(primaryItem.addons) ? primaryItem.addons : []) + .filter((addon) => Number(addon?.quantity ?? 0) > 0 && !isAddonRestricted(addon)) + .map((addon) => ({ ...addon, priceOverride: true })); + const additionalCandidates = (Array.isArray(additionalItems) ? additionalItems : []) + .filter((item) => Number(item?.quantity ?? 0) > 0 && !isStandaloneRestricted(item)) + .map((item) => ({ ...item, priceOverride: true })); - const addonPromises = addonCandidates.map((addon) => { - const addonProduct = addon?.product ?? addon; - return api - .createOrderItem( - normalizedOrderId, - Number(addonProduct.id ?? addon.id ?? 0), - Number(addon.quantity ?? 0), - createdPrimaryItemId, - String(addonProduct?.notes ?? addon?.notes ?? ""), - addonProduct?.skip_price_override === true || addon?.skip_price_override === true - ? null - : Number(addonProduct.price ?? addon.price ?? 0) - ) - .then((response) => ({ - productId: Number(addonProduct.id ?? addon.id ?? 0), - response, - })); - }); - - const additionalPromises = additionalCandidates.map((item) => - api - .createOrderItem( - normalizedOrderId, - Number(item.id ?? 0), - Number(item.quantity ?? 0), - null, - String(item.notes ?? ""), - item.skip_price_override === true ? null : Number(item.price ?? 0) - ) - .then((response) => ({ - productId: Number(item.id ?? 0), - response, - })) - ); - - const results = await Promise.allSettled([...addonPromises, ...additionalPromises]); - - const createdIds = []; - if (createdPrimaryItemId) { - createdIds.push(createdPrimaryItemId); - } - - const failures = []; - results.forEach((result, index) => { - const productId = - index < addonPromises.length - ? Number(addonCandidates[index]?.product?.id ?? addonCandidates[index]?.id ?? 0) - : Number(additionalCandidates[index - addonPromises.length]?.id ?? 0); - if (result.status === "fulfilled") { - const id = toPositiveInteger(result.value?.response?.data?.data?.id); - if (id) { - createdIds.push(id); - } - return; - } - failures.push({ - productId, - message: extractErrorMessage(result.reason), + try { + const { createdIds: childIds } = await addOrderItemAddons({ + orderId: normalizedOrderId, + primaryItemId: createdPrimaryItemId, + addons: addonCandidates, + additionalItems: additionalCandidates, + api, }); - }); - if (failures.length === 0) { - return { createdIds, createdPrimaryItemId }; + return { + createdIds: [createdPrimaryItemId, ...childIds].filter(Boolean), + createdPrimaryItemId, + }; + } catch (error) { + // The shared helper has already rolled back every add-on / additional + // row that landed during this attempt. Roll back the primary too so a + // retry starts from a clean state, then re-throw with the same error + // shape (`OrderItemsPartialSyncError`) the rest of the UI expects. + // Preserve the helper's aggregated failures and grow the + // rolledBackIds list to include the primary for callers / tests that + // inspect the error. + if (createdPrimaryItemId) { + await Promise.allSettled([api.removeOrderItem(createdPrimaryItemId)]); + } + if (error && typeof error === "object" && error.name === "OrderItemsPartialSyncError") { + const rolledBackIds = Array.isArray(error.rolledBackIds) ? error.rolledBackIds.slice() : []; + if (createdPrimaryItemId && !rolledBackIds.includes(createdPrimaryItemId)) { + rolledBackIds.push(createdPrimaryItemId); + } + throw new OrderItemsPartialSyncError(error.message, error.failures, rolledBackIds); + } + throw error; } - - // Roll back every row we created in this attempt so a retry starts clean. - await Promise.allSettled(createdIds.map((id) => api.removeOrderItem(id))); - - const aggregated = failures.map(formatFailureFragment).join("; "); - throw new OrderItemsPartialSyncError(aggregated, failures, createdIds.slice()); }; diff --git a/src/components/displays/department/pos/utils/addOrderItemAddons.js b/src/components/displays/department/pos/utils/addOrderItemAddons.js new file mode 100644 index 00000000..bc0d22aa --- /dev/null +++ b/src/components/displays/department/pos/utils/addOrderItemAddons.js @@ -0,0 +1,204 @@ +/** + * Pure, side-effect-free (apart from the injected API calls) helper that + * fans out a batch of `createOrderItem` POSTs for primary-product add-ons + * (and, optionally, additional standalone items) and rolls back every + * successful row when any one of the POSTs rejects. + * + * The same fan-out + rollback pattern is shared between the desktop + * (`SelectProductsFormPOS.vue → addAddonsToOrderMiddleware`) and the mobile + * (`PosDepartmentStepMobile2.vue → syncMobileOrderItems`) reconciliation + * paths. The desktop path passes a `primaryItemId` of the row it has just + * created; the mobile helper uses the same helper for its add-on / + * additional-item fan-out after the primary POST resolves. + * + * Why Promise.allSettled: a single rejection inside Promise.all would + * short-circuit the rest of the batch while the rows that already landed + * stayed on the server. On retry the operator saw only "some" of the + * selected add-ons persisted and a generic failure popup. allSettled + * collects every outcome, and the rollback restores a clean server state + * for the next attempt. + */ + +import { + OrderItemsPartialSyncError, + extractErrorMessage, + formatFailureFragment, +} from "./orderItemsPartialSync.js"; + +const toPositiveInteger = (value) => { + const parsed = Number.parseInt(String(value ?? ""), 10); + return Number.isInteger(parsed) && parsed > 0 ? parsed : null; +}; + +const toNonNegativeNumber = (value) => { + const parsed = Number(value ?? 0); + return Number.isFinite(parsed) && parsed >= 0 ? parsed : 0; +}; + +/** + * Build the createOrderItem arguments for one add-on row. The caller passes + * either an addon-shaped object (with `option_id` / `addon_id` / nested + * `product`) or a plain object with `product_id` + `quantity`; this helper + * normalizes both shapes. + * + * The `price` field is only included when the caller has explicitly set + * `priceOverride: true` on the addon. The mobile path always sets the + * override (it computes the customer-discounted price locally and wants + * the server to record it); the desktop path does not (it lets the server + * fall back to the product's default price). This matches the original + * desktop behaviour pre-PR-289 fix, where the existing `createOrderItem` + * call was made with five arguments and the server-side price was left + * untouched. + * + * @param {object} addon + * @param {number} primaryItemId The order_items.id of the freshly-created + * primary row that the add-on should be linked to via related_item_id. + */ +export const buildAddonCreateOrderItemArgs = (addon, primaryItemId) => { + const addonProduct = addon?.product ?? addon ?? {}; + const productId = Number(addonProduct?.id ?? addon?.option_id ?? addon?.addon_id ?? addon?.id ?? 0); + const quantity = toNonNegativeNumber(addon?.quantity ?? addonProduct?.quantity); + const relatedItemId = toPositiveInteger(primaryItemId); + const notes = String(addonProduct?.notes ?? addon?.notes ?? ""); + const skipPriceOverride = + addonProduct?.skip_price_override === true || addon?.skip_price_override === true; + const shouldOverridePrice = addon?.priceOverride === true; + const price = shouldOverridePrice + ? skipPriceOverride + ? null + : Number(addonProduct?.price ?? addon?.price ?? 0) + : null; + + return { + productId, + quantity, + relatedItemId, + notes, + skipPriceOverride, + overridePrice: shouldOverridePrice, + price, + }; +}; + +/** + * @typedef {Object} AddOrderItemAddonsApi + * @property {(orderId, productId, quantity, relatedItemId, notes, price) => Promise<{data:{data:{id:number}}}>} createOrderItem + * @property {(orderItemId) => Promise} removeOrderItem + */ + +/** + * Fan out `createOrderItem` for every add-on (and additional item) and + * rollback any rows that landed if any POST rejects. + * + * @param {object} args + * @param {number} args.orderId + * @param {number} args.primaryItemId The order_items.id of the freshly- + * created primary row. Every add-on POST links to it via + * related_item_id. + * @param {Array} [args.addons] Primary-product add-ons to POST. + * @param {Array} [args.additionalItems] Standalone additional + * items to POST (related_item_id is null). + * @param {AddOrderItemAddonsApi} args.api + * @returns {Promise<{createdIds: number[], createdAddonIds: number[], createdAdditionalIds: number[]}>} + * @throws {OrderItemsPartialSyncError} when any add-on or additional POST + * fails, after the partial rollback has already run. + */ +export const addOrderItemAddons = async ({ orderId, primaryItemId, addons = [], additionalItems = [], api }) => { + const normalizedOrderId = toPositiveInteger(orderId); + if (!normalizedOrderId) { + throw new Error("Order ID is required"); + } + const normalizedPrimaryItemId = toPositiveInteger(primaryItemId); + if (!normalizedPrimaryItemId) { + throw new Error("Primary order item ID is required"); + } + if (!api || typeof api.createOrderItem !== "function" || typeof api.removeOrderItem !== "function") { + throw new Error("createOrderItem and removeOrderItem are required"); + } + + const addonCandidates = Array.isArray(addons) ? addons.filter((addon) => toNonNegativeNumber(addon?.quantity) > 0) : []; + const additionalCandidates = Array.isArray(additionalItems) + ? additionalItems.filter((item) => toNonNegativeNumber(item?.quantity) > 0) + : []; + + const addonPromises = addonCandidates.map((addon) => { + const args = buildAddonCreateOrderItemArgs(addon, normalizedPrimaryItemId); + return Promise.resolve( + api.createOrderItem( + normalizedOrderId, + args.productId, + args.quantity, + args.relatedItemId, + args.notes, + args.price + ) + ).then((response) => ({ + productId: args.productId, + relatedItemId: args.relatedItemId, + response, + })); + }); + + const additionalPromises = additionalCandidates.map((item) => { + const productId = Number(item?.id ?? item?.product_id ?? 0); + const quantity = toNonNegativeNumber(item?.quantity); + const notes = String(item?.notes ?? ""); + const skipPriceOverride = item?.skip_price_override === true; + const shouldOverridePrice = item?.priceOverride === true; + const price = shouldOverridePrice ? (skipPriceOverride ? null : Number(item?.price ?? 0)) : null; + return Promise.resolve( + api.createOrderItem(normalizedOrderId, productId, quantity, null, notes, price) + ).then((response) => ({ + productId, + relatedItemId: null, + response, + })); + }); + + const results = await Promise.allSettled([...addonPromises, ...additionalPromises]); + + const createdAddonIds = []; + const createdAdditionalIds = []; + const failures = []; + + results.forEach((result, index) => { + const productId = + index < addonPromises.length + ? Number(addonCandidates[index]?.product?.id ?? addonCandidates[index]?.id ?? 0) + : Number(additionalCandidates[index - addonPromises.length]?.id ?? 0); + if (result.status === "fulfilled") { + const id = toPositiveInteger(result.value?.response?.data?.data?.id); + if (id) { + if (index < addonPromises.length) { + createdAddonIds.push(id); + } else { + createdAdditionalIds.push(id); + } + } + return; + } + failures.push({ + productId, + message: extractErrorMessage(result.reason), + }); + }); + + if (failures.length === 0) { + return { + createdIds: [...createdAddonIds, ...createdAdditionalIds], + createdAddonIds, + createdAdditionalIds, + }; + } + + // Roll back every row we created in this attempt so the next attempt + // starts from a clean server state. Use allSettled so a rollback + // rejection doesn't mask the original failure. + const createdIdsToRollBack = [...createdAddonIds, ...createdAdditionalIds]; + await Promise.allSettled(createdIdsToRollBack.map((id) => Promise.resolve(api.removeOrderItem(id)))); + + const aggregated = failures.map(formatFailureFragment).join("; "); + throw new OrderItemsPartialSyncError(aggregated, failures, createdIdsToRollBack); +}; + +export { OrderItemsPartialSyncError }; \ No newline at end of file diff --git a/src/components/displays/department/pos/utils/orderItemsPartialSync.js b/src/components/displays/department/pos/utils/orderItemsPartialSync.js new file mode 100644 index 00000000..ee9d88bc --- /dev/null +++ b/src/components/displays/department/pos/utils/orderItemsPartialSync.js @@ -0,0 +1,56 @@ +/** + * Shared error class + helpers for the POS order_items partial-sync pattern. + * + * Both the desktop (SelectProductsFormPOS.vue → addAddonsToOrderMiddleware) + * and the mobile (PosDepartmentStepMobile2.vue → syncMobileOrderItems) + * reconciliation paths use the same fan-out strategy: + * + * 1. Fan out every pending createOrderItem POST via Promise.allSettled so + * no single rejection short-circuits the batch. + * 2. If any POST rejects, roll back the rows that did land via + * Promise.allSettled(removeOrderItem) so a retry starts from a clean + * server state instead of the half-synced snapshot that caused the + * "only some of the selected primary product add-ons were persisted" + * bug originally filed for the mobile path. + * 3. Throw OrderItemsPartialSyncError so the UI can surface an aggregated + * error message that names every failed product. + * + * Keeping this contract in one place lets both call sites produce the same + * error shape and rollback guarantees. + */ + +/** + * Map an axios-like rejection to the human-readable string the operator + * should see. Falls back to the error message itself. + */ +export const extractErrorMessage = (reason) => { + const message = + reason?.response?.data?.data?.message ?? reason?.response?.data?.message ?? reason?.message ?? "Unknown error"; + return String(message); +}; + +/** + * Format a per-product failure into a human-readable fragment. Used to + * aggregate multiple add-on / additional-item rejections into one operator + * message. + */ +export const formatFailureFragment = ({ productId, message }) => { + if (!Number.isFinite(Number(productId)) || Number(productId) <= 0) { + return message; + } + return `Product ${productId}: ${message}`; +}; + +/** + * Thrown when one or more order_items POSTs fail during a fan-out. Carries + * the aggregated message and the per-product failure list so the UI layer + * can either display it directly or surface structured details. + */ +export class OrderItemsPartialSyncError extends Error { + constructor(message, failures, rolledBackIds) { + super(message); + this.name = "OrderItemsPartialSyncError"; + this.failures = failures; + this.rolledBackIds = rolledBackIds; + } +} \ No newline at end of file diff --git a/src/components/forms/department/pos/SelectProductsFormPOS.vue b/src/components/forms/department/pos/SelectProductsFormPOS.vue index 780cf3a7..596112ac 100644 --- a/src/components/forms/department/pos/SelectProductsFormPOS.vue +++ b/src/components/forms/department/pos/SelectProductsFormPOS.vue @@ -20,7 +20,7 @@ import { loadCustomerAttributes } from "@/components/shop/POSDepartmentProcess.vue"; import {getProductCategory, getProducts} from "@/components/shop/Products.vue"; -import { AUDITED_ORDER_ITEM_PRODUCT_IDS, createOrderItem } from "@/components/shop/OrdersItems.vue"; +import { AUDITED_ORDER_ITEM_PRODUCT_IDS, createOrderItem, removeOrderItem } from "@/components/shop/OrdersItems.vue"; import {SessionUser} from "@/components/session/token/SessionUser.vue"; import { useRoute } from 'vue-router'; import { useI18n } from "vue-i18n"; @@ -28,6 +28,7 @@ import Swal from "sweetalert2"; import ConfigurationCategory from "@/components/displays/superuser/configuration/ConfigurationCategory.vue"; import WhiteBox from "@/components/displays/boxes/WhiteBox.vue"; import ProductBox from "@/components/displays/boxes/ProductBox.vue"; +import { addOrderItemAddons } from "@/components/displays/department/pos/utils/addOrderItemAddons.js"; import { getPicture } from "@/components/displays/department/pos/displays/Piktogrammer.vue"; const emits = defineEmits(['onAddToCartProduct', 'onAddProduct', 'onSelectProduct', 'onSelectionInvalidated']); @@ -576,28 +577,62 @@ const addAddonsToOrderMiddleware = async (product_id, quantity = 1, related_item throw new Error("Created parent order item ID is missing"); } - for (let i = 0; i < product_addons.length; i++) { - // Get the addons quantity (If the addon quantity is 0, use the product quantity, otherwise multiply the addon quantity with the product quantity) - let addon_quantity = product_addons[i].quantity === 0 ? quantity : product_addons[i].quantity * quantity; - // Get the products addon, and show the fake create order item - showPendingCreateOrderItem( - getPendingProductFromAddon(product_id, product_addons[i].addon_id), - addon_quantity, - getAddonPrice(product_id, product_addons[i].addon_id), - normalizedRelatedItemId, - product_addons[i].notes === undefined ? null : product_addons[i].notes - ); + // Show the pending rows for every add-on so the cart panel reflects the + // optimistic state immediately. The real POST fan-out happens below and + // the pending rows are cleared by `loadOrderItems` on success or by + // `clearPendingOrderItems` on failure. + const addonDefinitions = product_addons.map((selection) => { + const addon_quantity = selection.quantity === 0 ? quantity : selection.quantity * quantity; + const addonAddon = getProductAddonDefinition(product_id, selection.addon_id); + return { + addon_id: selection.addon_id, + quantity: addon_quantity, + notes: selection.notes === undefined ? null : selection.notes, + product: getPendingProductFromAddon(product_id, selection.addon_id), + price: getAddonPrice(product_id, selection.addon_id), + _addon_definition: addonAddon, + }; + }); + + for (const addon of addonDefinitions) { + showPendingCreateOrderItem(addon.product, addon.quantity, addon.price, normalizedRelatedItemId, addon.notes); } - // Add the addons to the order - for (let i = 0; i < product_addons.length; i++) { - let addon_quantity = product_addons[i].quantity === 0 ? quantity : product_addons[i].quantity * quantity; - await createOrderItem( - targetOrderId, - product_addons[i].addon_id, - addon_quantity, - normalizedRelatedItemId, - product_addons[i].notes === undefined ? null : product_addons[i].notes - ).catch((error) => handleCreateOrderItemError(error)); + + // Build the add-on payloads the shared fan-out helper expects. The + // helper accepts either an addon-shaped object (`option_id` / nested + // `product`) or a plain object with `product_id` + `quantity`; we pass + // the nested `product` form so the helper can read both `product.id` + // and `product.price` for the POST without us flattening it twice. + const helperAddons = addonDefinitions.map((addon) => ({ + option_id: addon.addon_id, + quantity: addon.quantity, + notes: addon.notes, + skip_price_override: addon._addon_definition?.product?.skip_price_override === true, + product: { + id: addon.product?.id, + price: addon.price, + notes: addon.notes ?? "", + }, + })); + + try { + await addOrderItemAddons({ + orderId: targetOrderId, + primaryItemId: normalizedRelatedItemId, + addons: helperAddons, + api: { + createOrderItem: (orderId, productId, addonQuantity, relatedItemId, notes, price) => + createOrderItem(orderId, productId, addonQuantity, relatedItemId, notes, price), + removeOrderItem: (orderItemId) => removeOrderItem(orderItemId), + }, + }); + } catch (error) { + // The shared helper has already rolled back every row that landed + // during this attempt. Surface the error through the same handler + // the rest of the desktop flow uses so the operator sees a friendly + // restriction warning for backend customer-rule blocks and the + // original error otherwise. + await handleCreateOrderItemError(error); } }; diff --git a/tests/e2e/pos-customer-rules.spec.js b/tests/e2e/pos-customer-rules.spec.js index abd90f92..711c7b02 100644 --- a/tests/e2e/pos-customer-rules.spec.js +++ b/tests/e2e/pos-customer-rules.spec.js @@ -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]); +}); diff --git a/tests/unit/order-items-addon-fanout.spec.js b/tests/unit/order-items-addon-fanout.spec.js new file mode 100644 index 00000000..b9717d9c --- /dev/null +++ b/tests/unit/order-items-addon-fanout.spec.js @@ -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" }]); + }); +});