fix(orders): review-marker review fixes (collection helper, empty patch, markup_percent)

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

Co-authored-by: Orca <help@stably.ai>

src/lib/server/orders-service-review.test.ts

@@ -294,4 +294,104 @@ describe("updateOrderItemsPrices", () => {
     expect(result.updated).toBe(0);
     expect(result.failed).toEqual([]);
   });
+
+  it("skips pb.update when the resolved patch is empty (no field changes)", async () => {
+    const order = await pbMock.collection("orders").create({
+      order_number: "ORD-2026-07-001",
+    });
+    await pbMock.collection("order_items").create({
+      id: "item-noop",
+      order: order.id,
+      description: "Drill",
+      quantity: 2,
+      selling_price: 50,
+      cost_price: 25,
+      markup: 100,
+    });
+    await pbMock.collection("order_items").create({
+      id: "item-does",
+      order: order.id,
+      description: "Bit",
+      quantity: 1,
+      selling_price: 10,
+      cost_price: 5,
+      markup: 100,
+    });
+
+    // Spy on update to confirm the no-op row doesn't trigger a write.
+    // `pbMock.collection(name)` returns a fresh object each call, so we
+    // wrap the `collection` factory itself and intercept at every call.
+    const originalCollection = pbMock.collection;
+    const updateCalls: Array<{ id: string; data: Record<string, unknown> }> = [];
+    pbMock.collection = (name: string) => {
+      const col = originalCollection(name);
+      if (name === "order_items") {
+        const originalUpdate = col.update;
+        col.update = vi.fn(async (id: string, data: Record<string, unknown>) => {
+          updateCalls.push({ id, data });
+          return originalUpdate(id, data);
+        });
+      }
+      return col;
+    };
+
+    try {
+      const result = await updateOrderItemsPrices(order.id, [
+        // No field changes — caller's patch has no quantity/price/markup.
+        { id: "item-noop" },
+        // Real change — should issue an update.
+        { id: "item-does", quantity: 7 },
+      ]);
+
+      // Only the row with an actual change counts as updated.
+      expect(result.updated).toBe(1);
+      expect(result.failed).toEqual([]);
+
+      // The no-op row did NOT receive a pb.update call.
+      const noopCalls = updateCalls.filter((c) => c.id === "item-noop");
+      expect(noopCalls).toEqual([]);
+
+      // The real row did.
+      const doesCalls = updateCalls.filter((c) => c.id === "item-does");
+      expect(doesCalls).toHaveLength(1);
+      expect(doesCalls[0].data).toEqual({ quantity: 7 });
+
+      // The row's quantity was actually written.
+      const item = await pbMock.collection("order_items").getOne("item-does");
+      expect(item.quantity).toBe(7);
+    } finally {
+      pbMock.collection = originalCollection;
+    }
+  });
+
+  it("writes markup_percent alongside markup when selling_price is supplied", async () => {
+    const order = await pbMock.collection("orders").create({
+      order_number: "ORD-2026-07-001",
+    });
+    await pbMock.collection("order_items").create({
+      id: "item-mp",
+      order: order.id,
+      description: "Drill",
+      quantity: 2,
+      selling_price: 50,
+      cost_price: 25,
+      // Start with stale markup_percent to confirm it gets overwritten.
+      markup: 100,
+      markup_percent: 0,
+    });
+
+    const result = await updateOrderItemsPrices(order.id, [
+      // selling_price 60, cost_price unchanged at 25 → markup = (60-25)/25*100 = 140
+      { id: "item-mp", selling_price: 60 },
+    ]);
+
+    expect(result.updated).toBe(1);
+    expect(result.failed).toEqual([]);
+
+    const item = await pbMock.collection("order_items").getOne("item-mp");
+    expect(item.selling_price).toBe(60);
+    expect(item.markup).toBe(140);
+    // markup_percent must mirror markup — the two fields stay in lock-step.
+    expect(item.markup_percent).toBe(140);
+  });
 });

src/lib/server/orders-service.ts

@@ -1,7 +1,6 @@
 import { getAdminPocketBase } from "./pocketbase-admin";
 
 const ORDER_COLLECTION = "orders";
-const ORDER_ITEMS_COLLECTION = "order_items";
 const QUOTE_COLLECTION = "quotes";
 const QUOTE_ITEMS_COLLECTION = "quote_items";
 const ORDER_COUNTERS_COLLECTION = "order_counters";
@@ -363,6 +362,7 @@ export async function updateOrderItemsPrices(
   failed: Array<{ id: string; error: string }>;
 }> {
   const pb = await getAdminPocketBase();
+  const orderItemsCollection = getOrderItemsCollectionName();
 
   if (items.length === 0) return { updated: 0, failed: [] };
 
@@ -373,7 +373,7 @@ export async function updateOrderItemsPrices(
     try {
       // Read the current row so we can compute markup when only
       // one of selling_price/cost_price is supplied.
-      const current = await pb.collection(ORDER_ITEMS_COLLECTION).getOne(item.id);
+      const current = await pb.collection(orderItemsCollection).getOne(item.id);
 
       const patch: Record<string, unknown> = {};
       if (item.quantity !== undefined) patch.quantity = item.quantity;
@@ -386,23 +386,34 @@ export async function updateOrderItemsPrices(
       //  - cost_price supplied, selling_price not → recompute from current-selling/cost
       //  - both supplied → recompute from the new pair
       //  - neither supplied, no explicit markup → leave current markup alone
+      //
+      // `markup_percent` mirrors `markup` (the two fields are kept in
+      // lock-step by `createOrderFromQuote`); write both together so
+      // the row never goes stale after a price edit.
       if (item.markup !== undefined) {
         patch.markup = item.markup;
+        patch.markup_percent = item.markup;
       } else if (
         item.selling_price !== undefined ||
         item.cost_price !== undefined
       ) {
         const newSelling = item.selling_price ?? current.selling_price;
         const newCost = item.cost_price ?? current.cost_price;
-        if (Number(newCost) > 0) {
-          patch.markup =
-            Math.round(((Number(newSelling) - Number(newCost)) / Number(newCost)) * 10000) / 100;
-        } else {
-          patch.markup = 0;
-        }
+        const newMarkup =
+          Number(newCost) > 0
+            ? Math.round(((Number(newSelling) - Number(newCost)) / Number(newCost)) * 10000) / 100
+            : 0;
+        patch.markup = newMarkup;
+        patch.markup_percent = newMarkup;
       }
 
-      await pb.collection(ORDER_ITEMS_COLLECTION).update(item.id, patch);
+      // Empty patch: nothing to write. Skip the PB round-trip so we
+      // don't stamp `updated` and don't bump the row's `updated` ts.
+      if (Object.keys(patch).length === 0) {
+        continue;
+      }
+
+      await pb.collection(orderItemsCollection).update(item.id, patch);
       updated.push(item.id);
     } catch (err) {
       failed.push({