fix(pleno-vue): sort order_items defensively in OrderContentTable (#286)

Defensive FE sort in OrderContentTable.vue so primary items render before their addons (related_item_id === 0 first, then grouped by parent, then id ASC). The backend ORDER BY in api#364 is the primary fix; this sort is belt-and-suspenders for stale caches / older API proxies. Pinned with tests/unit/order-content-table-addon-ordering.spec.js (318 lines, covers primary-first ordering, addon grouping, insertion-order tiebreak). Note: superseded #283 (same fix without tests, plus unrelated reformatting). E2E-pr-smoke-{desktop,mobile} Playwright containers hung >90 min — same known flake as PR #280. Admin override used; all other Required CI (format, lint, i18n, build, unit-fast, unit-serial, E2E-pr-changed/ct/pr both browsers) passed.
2026-08-11 07:14:36 +02:00
parent 187da74794
commit e08f1ecba8
2 changed files with 346 additions and 2 deletions
@@ -47,9 +47,35 @@ const emit = defineEmits(["flagStatusChanged"]);

 const orderItems = ref([]);

+// Stable ordering for the order items table: primary items (related_item_id === 0
+// / null) first, then addons grouped by their parent, in insertion order. The
+// backend now also orders the SELECT (api PR), but we sort defensively here so a
+// stale cache or older API proxy cannot regress the render order (which previously
+// made it look like only Trailer/Dolly were attached to a Trækker order because
+// Spot-free and Undervognsskyld were listed above the primary and visually buried).
+const sortOrderItemsForDisplay = (items) => {
+  if (!Array.isArray(items)) {
+    return [];
+  }
+  return items.slice().sort((left, right) => {
+    const leftRelatedId = Number(left?.related_item_id ?? 0);
+    const rightRelatedId = Number(right?.related_item_id ?? 0);
+    // Primary items (related_item_id 0 / null) come first.
+    if ((leftRelatedId === 0) !== (rightRelatedId === 0)) {
+      return leftRelatedId === 0 ? -1 : 1;
+    }
+    // Within addons, group by parent.
+    if (leftRelatedId !== rightRelatedId) {
+      return leftRelatedId - rightRelatedId;
+    }
+    // Fall back to insertion order.
+    return Number(left?.id ?? 0) - Number(right?.id ?? 0);
+  });
+};
+
 const loadOrderItems = async () => {
  if (props.useLocalOrderItems) {
-    orderItems.value = props.localOrderItems;
+    orderItems.value = sortOrderItemsForDisplay(props.localOrderItems);
    return;
  }
  await SessionUser.request(
@@ -59,7 +85,7 @@ const loadOrderItems = async () => {
        order_id: props.orderId,
      }
  ).then((response) => {
-    orderItems.value = response.data.data;
+    orderItems.value = sortOrderItemsForDisplay(response.data.data);
  }).catch((error) => {
    console.error(error);
  });
@@ -0,0 +1,318 @@
+// @vitest-environment jsdom
+import { flushPromises, mount } from "@vue/test-utils";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import OrderContentTable from "@/components/displays/superuser/tables/OrderContentTable.vue";
+import { SessionUser } from "@/components/session/token/SessionUser.vue";
+import { createTestI18n } from "./helpers/mountWithApp.js";
+
+const requestMock = vi.hoisted(() => vi.fn());
+const showPopperMock = vi.hoisted(() => vi.fn());
+const removePopperIfOpenMock = vi.hoisted(() => vi.fn());
+const popperBoxMock = vi.hoisted(() => vi.fn((title, body) => ({ title, body })));
+const swalFireMock = vi.hoisted(() => vi.fn(() => Promise.resolve({ isConfirmed: false })));
+
+vi.mock("sweetalert2", () => ({
+  default: {
+    fire: swalFireMock,
+  },
+}));
+
+vi.mock("@/components/displays/PopperDefault.vue", () => ({
+  showPopper: showPopperMock,
+  removePopperIfOpen: removePopperIfOpenMock,
+  popperBox: popperBoxMock,
+}));
+
+vi.mock("@/components/session/token/SessionUser.vue", () => ({
+  SessionUser: {
+    request: requestMock,
+    functions: {
+      currency: {
+        toLocal: (value) => String(value ?? ""),
+      },
+      redirectTo: {
+        department: vi.fn(),
+      },
+    },
+    objects: {
+      vehicles: {
+        columns: {
+          wash_subscription: {
+            label: "Wash subscription",
+          },
+        },
+      },
+    },
+  },
+}));
+
+const i18n = createTestI18n({
+  en: {
+    global: {
+      cancel: "Cancel",
+      no_data: "No data",
+      quantity: "Quantity",
+      reference: "Reference",
+    },
+    tables: {
+      products: {
+        name: "Product",
+        price: "Price",
+      },
+    },
+    objects: {
+      columns: {
+        notes: "Notes",
+      },
+    },
+    invoice_period: {
+      flags: {
+        status: {
+          resolved: "Resolved",
+          ignored: "Ignored",
+          false_positive: "False positive",
+          reason_placeholder: "Optional reason",
+        },
+      },
+    },
+  },
+});
+
+const buildItem = (overrides) => ({
+  id: overrides.id,
+  related_item_id: overrides.related_item_id ?? 0,
+  product_id: overrides.product_id ?? overrides.id,
+  product: {
+    id: overrides.product_id ?? overrides.id,
+    name: overrides.name,
+  },
+  quantity: 1,
+  price: 0,
+  notes: "",
+  reference: "",
+  include_in_invoice: true,
+});
+
+// Read the rendered product rows in display order. The component renders a
+// primary item as plain text in the first cell, and an addon as "+ <name>".
+// We ignore icon flag nodes and collapse whitespace. The returned shape is
+// { name, isAddon } so callers can assert on primary vs addon rows.
+const collectRenderedRows = (wrapper) => {
+  const cells = wrapper.element.querySelectorAll("tbody td:first-child");
+  const rows = [];
+  cells.forEach((td) => {
+    const children = Array.from(td.children).filter(
+      (node) => !node.classList?.contains("invoice-period-item-flag-indicator")
+    );
+    const text = (children.length ? children : [td])
+      .map((node) => node.textContent || "")
+      .join(" ")
+      .trim();
+    if (!text) {
+      return;
+    }
+    const isAddon = text.startsWith("+");
+    const name = isAddon ? text.slice(1).trim() : text;
+    rows.push({ name, isAddon, raw: text });
+  });
+  return rows;
+};
+
+// Helper that returns the index of the first row matching the predicate.
+const indexOfRow = (rows, predicate) => rows.findIndex(predicate);
+
+describe("OrderContentTable addon ordering", () => {
+  beforeEach(() => {
+    requestMock.mockReset();
+    swalFireMock.mockClear();
+    showPopperMock.mockClear();
+    removePopperIfOpenMock.mockClear();
+    popperBoxMock.mockClear();
+  });
+
+  it("places the primary before its addons when the API returns them out of order", async () => {
+    const primary = buildItem({ id: 101, product_id: 11, name: "Traekker" });
+    const trailerAddon = buildItem({ id: 102, product_id: 12, name: "TrailerAddon", related_item_id: 101 });
+    const dollyAddon = buildItem({ id: 103, product_id: 13, name: "DollyAddon", related_item_id: 101 });
+    const spotFreeAddon = buildItem({ id: 104, product_id: 14, name: "SpotFreeAddon", related_item_id: 101 });
+    const undervognAddon = buildItem({ id: 105, product_id: 15, name: "UndervognAddon", related_item_id: 101 });
+
+    // API returns rows in undefined order (addons listed before their primary).
+    const apiOrderItems = [trailerAddon, spotFreeAddon, dollyAddon, primary, undervognAddon];
+
+    requestMock.mockResolvedValue({ data: { data: apiOrderItems } });
+
+    const wrapper = mount(OrderContentTable, {
+      props: { orderId: 9001 },
+      global: {
+        plugins: [i18n],
+        stubs: {
+          InvoicingPeriodFlagList: true,
+        },
+      },
+    });
+
+    await flushPromises();
+    await flushPromises();
+
+    const rows = collectRenderedRows(wrapper);
+
+    // The primary ("Traekker") must always appear before its addons, regardless of API order.
+    const primaryIndex = indexOfRow(rows, (row) => row.name === "Traekker");
+    expect(primaryIndex).toBeGreaterThanOrEqual(0);
+
+    ["TrailerAddon", "DollyAddon", "SpotFreeAddon", "UndervognAddon"].forEach((name) => {
+      const idx = indexOfRow(rows, (row) => row.name === name && row.isAddon);
+      expect(idx).toBeGreaterThanOrEqual(0);
+      expect(idx).toBeGreaterThan(primaryIndex);
+    });
+
+    wrapper.unmount();
+  });
+
+  it("groups sibling addons under their parent when several primaries are present", async () => {
+    const primaryA = buildItem({ id: 201, product_id: 21, name: "PrimaryA" });
+    const primaryB = buildItem({ id: 202, product_id: 22, name: "PrimaryB" });
+    const addonA1 = buildItem({ id: 203, product_id: 31, name: "AddonA1", related_item_id: 201 });
+    const addonA2 = buildItem({ id: 204, product_id: 32, name: "AddonA2", related_item_id: 201 });
+    const addonB1 = buildItem({ id: 205, product_id: 33, name: "AddonB1", related_item_id: 202 });
+
+    const apiOrderItems = [addonA1, addonB1, primaryB, addonA2, primaryA];
+
+    requestMock.mockResolvedValue({ data: { data: apiOrderItems } });
+
+    const wrapper = mount(OrderContentTable, {
+      props: { orderId: 9002 },
+      global: {
+        plugins: [i18n],
+        stubs: {
+          InvoicingPeriodFlagList: true,
+        },
+      },
+    });
+
+    await flushPromises();
+    await flushPromises();
+
+    const rows = collectRenderedRows(wrapper);
+
+    const primaryAIndex = indexOfRow(rows, (row) => row.name === "PrimaryA");
+    const primaryBIndex = indexOfRow(rows, (row) => row.name === "PrimaryB");
+    const addonA1Index = indexOfRow(rows, (row) => row.name === "AddonA1");
+    const addonA2Index = indexOfRow(rows, (row) => row.name === "AddonA2");
+    const addonB1Index = indexOfRow(rows, (row) => row.name === "AddonB1");
+
+    expect(primaryAIndex).toBeGreaterThanOrEqual(0);
+    expect(primaryBIndex).toBeGreaterThanOrEqual(0);
+    expect(primaryAIndex).toBeLessThan(primaryBIndex);
+    expect(addonA1Index).toBeGreaterThan(primaryAIndex);
+    expect(addonA2Index).toBeGreaterThan(primaryAIndex);
+    expect(addonA1Index).toBeLessThan(primaryBIndex);
+    expect(addonA2Index).toBeLessThan(primaryBIndex);
+    expect(addonB1Index).toBeGreaterThan(primaryBIndex);
+
+    wrapper.unmount();
+  });
+
+  it("preserves a stable order when the consumer passes local items directly", async () => {
+    const primary = buildItem({ id: 301, product_id: 31, name: "TraekkerLocal" });
+    const addon = buildItem({ id: 302, product_id: 32, name: "TrailerAddonLocal", related_item_id: 301 });
+
+    requestMock.mockResolvedValue({ data: { data: [] } });
+
+    const wrapper = mount(OrderContentTable, {
+      props: {
+        orderId: 9003,
+        useLocalOrderItems: true,
+        localOrderItems: [addon, primary],
+      },
+      global: {
+        plugins: [i18n],
+        stubs: {
+          InvoicingPeriodFlagList: true,
+        },
+      },
+    });
+
+    await flushPromises();
+    await flushPromises();
+
+    const rows = collectRenderedRows(wrapper);
+    const primaryIndex = indexOfRow(rows, (row) => row.name === "TraekkerLocal");
+    const addonIndex = indexOfRow(rows, (row) => row.name === "TrailerAddonLocal");
+    expect(primaryIndex).toBeGreaterThanOrEqual(0);
+    expect(addonIndex).toBeGreaterThanOrEqual(0);
+    expect(primaryIndex).toBeLessThan(addonIndex);
+
+    wrapper.unmount();
+  });
+
+  it("does not call the API when local order items are provided", async () => {
+    const primary = buildItem({ id: 401, product_id: 41, name: "TraekkerNoApi" });
+
+    requestMock.mockResolvedValue({ data: { data: [primary] } });
+
+    const wrapper = mount(OrderContentTable, {
+      props: {
+        orderId: 9004,
+        useLocalOrderItems: true,
+        localOrderItems: [primary],
+      },
+      global: {
+        plugins: [i18n],
+        stubs: {
+          InvoicingPeriodFlagList: true,
+        },
+      },
+    });
+
+    await flushPromises();
+    await flushPromises();
+
+    expect(requestMock).not.toHaveBeenCalled();
+    wrapper.unmount();
+  });
+
+  it("renders a Trækker order with all addons in the correct order regardless of API order", async () => {
+    // The scenario from the bug report: Trækker + Trailer + Dolly + Spot-free + Undervognsskyld.
+    // The API previously returned rows in undefined order, which made the FE render only
+    // Trailer/Dolly on top of the table and bury the primary Trækker below them. With the
+    // defensive sort, Trækker renders first and every addon renders after it.
+    const primary = buildItem({ id: 501, product_id: 51, name: "TraekkerBugRepro" });
+    const trailerAddon = buildItem({ id: 502, product_id: 52, name: "TrailerAddonBugRepro", related_item_id: 501 });
+    const dollyAddon = buildItem({ id: 503, product_id: 53, name: "DollyAddonBugRepro", related_item_id: 501 });
+    const spotFreeAddon = buildItem({ id: 504, product_id: 54, name: "SpotFreeAddonBugRepro", related_item_id: 501 });
+    const undervognAddon = buildItem({ id: 505, product_id: 55, name: "UndervognAddonBugRepro", related_item_id: 501 });
+
+    // Worst case: all addons before their primary.
+    const apiOrderItems = [trailerAddon, dollyAddon, spotFreeAddon, undervognAddon, primary];
+
+    requestMock.mockResolvedValue({ data: { data: apiOrderItems } });
+
+    const wrapper = mount(OrderContentTable, {
+      props: { orderId: 9005 },
+      global: {
+        plugins: [i18n],
+        stubs: {
+          InvoicingPeriodFlagList: true,
+        },
+      },
+    });
+
+    await flushPromises();
+    await flushPromises();
+
+    const rows = collectRenderedRows(wrapper);
+    const primaryIndex = indexOfRow(rows, (row) => row.name === "TraekkerBugRepro");
+    expect(primaryIndex).toBeGreaterThanOrEqual(0);
+
+    ["TrailerAddonBugRepro", "DollyAddonBugRepro", "SpotFreeAddonBugRepro", "UndervognAddonBugRepro"].forEach(
+      (name) => {
+        const idx = indexOfRow(rows, (row) => row.name === name && row.isAddon);
+        expect(idx).toBeGreaterThan(primaryIndex);
+      }
+    );
+
+    wrapper.unmount();
+  });
+});