From c572d08e5325bc78292ff70cfc545657739ae52f Mon Sep 17 00:00:00 2001
From: SJCoetz <s.coetzee6@my.milpark.ac.za>
Date: Wed, 29 Jul 2026 15:07:11 +0200
Subject: [PATCH] fix(orders): review modal review fixes (drop dead markup
 check, lock tests, link assertion)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Co-authored-by: Orca <help@stably.ai>
---
 src/components/orders-review-modal.test.tsx | 60 +++++++++++++++++++++
 src/components/orders-review-modal.tsx      |  6 ++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/components/orders-review-modal.test.tsx b/src/components/orders-review-modal.test.tsx
index 4f5c545..d8e0718 100644
--- a/src/components/orders-review-modal.test.tsx
+++ b/src/components/orders-review-modal.test.tsx
@@ -187,5 +187,65 @@ describe("OrdersReviewModal", () => {
       "/api/v1/orders/order-1/review-save",
       expect.objectContaining({ method: "POST" }),
     );
+
+    expect(
+      screen.getByRole("link", { name: /open existing/i }),
+    ).toHaveAttribute("href", "/orders/order-other");
+  });
+
+  it("lock pattern: editing cost recomputes selling and flips lock to 'cost'", () => {
+    render(<OrdersReviewModal {...baseProps()} />);
+
+    // baseItems: cost=25, selling=50, markup=100 → initial lock="cost".
+    // Edit cost from 25 → 40 (markup=100). Expected: selling = round2(40*2) = 80.
+    const costInputs = screen.getAllByDisplayValue("25");
+    expect(costInputs).toHaveLength(1);
+    fireEvent.change(costInputs[0], { target: { value: "40" } });
+
+    // Selling should now show 80.
+    expect(screen.getByDisplayValue("80")).toBeInTheDocument();
+    expect(screen.queryByDisplayValue("50")).not.toBeInTheDocument();
+  });
+
+  it("lock pattern: editing selling recomputes cost and flips lock to 'selling'", () => {
+    render(<OrdersReviewModal {...baseProps()} />);
+
+    // baseItems: cost=25, selling=50, markup=100 → initial lock="cost".
+    // Edit selling from 50 → 100 (markup=100). Expected: cost = round2(100/2) = 50.
+    const sellingInputs = screen.getAllByDisplayValue("50");
+    expect(sellingInputs).toHaveLength(1);
+    fireEvent.change(sellingInputs[0], { target: { value: "100" } });
+
+    // Cost should now show 50.
+    expect(screen.getByDisplayValue("50")).toBeInTheDocument();
+    expect(screen.queryByDisplayValue("25")).not.toBeInTheDocument();
+  });
+
+  it("lock pattern: editing markup with lock='selling' recomputes cost", () => {
+    render(<OrdersReviewModal {...baseProps()} />);
+
+    // baseItems: cost=25, selling=50, markup=100 → initial lock="cost".
+    // To exercise the markup-edit-costh-recompute branch, first flip
+    // the lock to "selling" by editing the selling price (which
+    // recomputes cost from markup, then sets lock="selling").
+    // selling 50 → 100, cost recomputes from 25 → 50, lock="selling".
+    fireEvent.change(screen.getByDisplayValue("50"), {
+      target: { value: "100" },
+    });
+    // After: cost=50, selling=100, markup=100, lock="selling".
+
+    // Now edit markup from 100 → 200. With lock="selling", cost
+    // recomputes: cost = round2(100 / (1 + 200/100)) = round2(100/3) = 33.33.
+    // After the previous selling edit, both the markup and selling
+    // inputs display "100" — pick the markup input (index 2 of the
+    // per-row input array: qty/cost/markup/selling) by scoping to the
+    // list item for "HSS drill 6mm".
+    const itemRow = screen.getByText("HSS drill 6mm").closest("li")!;
+    const rowInputs = itemRow.querySelectorAll("input");
+    // rowInputs: [qty, cost, markup, selling]
+    fireEvent.change(rowInputs[2], { target: { value: "200" } });
+
+    // Cost should now show 33.33.
+    expect(screen.getByDisplayValue("33.33")).toBeInTheDocument();
   });
 });
diff --git a/src/components/orders-review-modal.tsx b/src/components/orders-review-modal.tsx
index 417b118..711ffd9 100644
--- a/src/components/orders-review-modal.tsx
+++ b/src/components/orders-review-modal.tsx
@@ -122,7 +122,11 @@ export function OrdersReviewModal({
       if (d.quantity !== it.quantity) return true;
       if (d.selling_price !== it.selling_price) return true;
       if (d.cost_price !== it.cost_price) return true;
-      if ((d.markup ?? null) !== (it.markup ?? null)) return true;
+      // NOTE: markup is deliberately not compared here. The save body only
+      // carries quantity/selling_price/cost_price; the service layer
+      // recomputes markup server-side from the selling/cost pair. A
+      // markup-only edit is impossible anyway — onMarkupChange always
+      // rewrites the lock partner, which *is* part of the dirty body.
     }
     return false;
     // eslint-disable-next-line react-hooks/exhaustive-deps