Fix invoice period tree review findings (#253)

## Summary - preserve complete snapshot item payloads during inline edits and reject partial text-field payloads - force snapshot refreshes after parent/mutation changes with one bounded recovery retry - make legacy tree-action fallback create, confirm, and apply a fresh compatible preview - keep collection labeling localized and report the correct changed count ## Verification - focused object-tree and snapshot suites: 30 tests passed - focused ESLint and `git diff --check` clean - production build and selected-customer mocked Playwright flow passed before final review fixes - App Store Readiness and Qodana green on exact head; Automated Tests in progress - independent QA and reviewer gates: GO Resolves all inline review threads on the current head.
2026-08-03 13:04:19 +02:00
parent 3639527b0e
commit 7a5ee1aa5b
4 changed files with 276 additions and 58 deletions
@@ -298,6 +298,18 @@ const refreshSnapshot = async ({ force = false, consumedKeys = [] as any[] } = {
    }
  }
 };
+const refreshSnapshotAfterMutation = async ({ consumedKeys = [] as any[] } = {}) => {
+  if (!activeSnapshot.value) {
+    return null;
+  }
+  const refreshedSnapshot = await refreshSnapshot({ force: true, consumedKeys });
+  if (refreshedSnapshot || !activeSnapshot.value || legacySnapshotFallbackAllowed.value) {
+    return refreshedSnapshot;
+  }
+  // A mutation has already committed at this point. Give a transient snapshot
+  // failure one bounded retry so actions do not remain latched off indefinitely.
+  return refreshSnapshot({ force: true, consumedKeys });
+};
 const expandableNodeKeys = computed(() =>
  Object.values(nodeById.value)
    .filter((node) => node?.isLeaf !== true && node?.disabled !== true && node?.id !== undefined && node?.id !== null)
@@ -345,9 +357,11 @@ watch(
 );

 watch(legacyDataSignature, () => {
-  if (!activeSnapshot.value) {
-    replaceRootNodes(buildLegacyRootNodes());
+  if (activeSnapshot.value) {
+    void refreshSnapshot({ force: true });
+    return;
  }
+  replaceRootNodes(buildLegacyRootNodes());
 });

 watch(
@@ -1371,6 +1385,9 @@ const getNodeFlags = (node: TreeNode) => {
  return [...entityFlags, ...fallbackFieldFlags];
 };
 const handleFlagStatusChanged = () => {
+  if (activeSnapshot.value) {
+    void refreshSnapshotAfterMutation();
+  }
  emit("refresh");
 };

@@ -1596,6 +1613,9 @@ const canEditOrderField = (order: any, column: string) => {
 };
 const canEditOrderItemField = () => SessionUser.hasPermission?.("edit_order_items") === true;
 const refreshAfterInlineEdit = async () => {
+  if (activeSnapshot.value) {
+    await refreshSnapshotAfterMutation();
+  }
  emit("refresh");
 };
 const collectionObjectForNode = (node: TreeNode) => {
@@ -1634,9 +1654,23 @@ const findCachedOrderItem = (itemId: number) => {
      return match;
    }
  }
-  return null;
+  return nodeById.value[makeNodeId(TREE_NODE_TYPES.ORDER_ITEM, itemId)]?.meta?.item ?? null;
 };
 const showEditOrderItemFieldForm = async (id: number, column: string, value: any, onAfterSubmit: any = null) => {
+  const item = findCachedOrderItem(toPositiveInteger(id) || 0);
+  const hasCompleteUpdatePayload =
+    item &&
+    Number.isFinite(Number(item.price)) &&
+    Number.isFinite(Number(item.quantity ?? item.amount)) &&
+    Object.prototype.hasOwnProperty.call(item, "reference") &&
+    Object.prototype.hasOwnProperty.call(item, "notes");
+  if (!hasCompleteUpdatePayload) {
+    return Swal.fire({
+      icon: "error",
+      title: loadFailedMessage(),
+    });
+  }
+
  const result = await Swal.fire({
    title: fieldLabel(
      {
@@ -1658,21 +1692,18 @@ const showEditOrderItemFieldForm = async (id: number, column: string, value: any
    return result;
  }

-  const item = findCachedOrderItem(toPositiveInteger(id) || 0);
  const nextItem = {
-    ...(item || {}),
+    ...item,
    [column]: result.value,
  };
  await SessionUser.request("/order/items", "PUT", {
    id,
-    price: Number(nextItem.price ?? 0),
+    price: Number(nextItem.price),
    notes: String(nextItem.notes ?? ""),
    reference: String(nextItem.reference ?? ""),
-    quantity: Number(nextItem.quantity ?? nextItem.amount ?? 1),
+    quantity: Number(nextItem.quantity ?? nextItem.amount),
  });
-  if (item) {
-    item[column] = result.value;
-  }
+  item[column] = result.value;
  if (typeof onAfterSubmit === "function") {
    await onAfterSubmit();
  }
@@ -2049,6 +2080,9 @@ const runActionWithPreview = async (
      timer: 1400,
      showConfirmButton: false,
    });
+    if (activeSnapshot.value) {
+      await refreshSnapshotAfterMutation();
+    }
    emit("refresh");
  } catch (error: any) {
    await Swal.fire({
@@ -2123,6 +2157,47 @@ const chooseMergeTargetInvoiceCollection = async (invoiceCollectionIds: number[]
  return normalizeInvoiceCollectionId(result.value);
 };

+const confirmCollectionBulkActionPreview = async (action: string, previewRaw: any, preview: any) => {
+  const blockers = preview.blockers;
+  if (blockers.length > 0 || preview.changedCount === 0) {
+    await Swal.fire({
+      icon: blockers.length > 0 ? "error" : "info",
+      title:
+        blockers.length > 0
+          ? t("invoicing_period.invoice_collection_actions.preview.blocked_title")
+          : t("invoicing_period.invoice_collection_actions.preview.no_changes_title"),
+      html: renderBulkActionPreviewHtml(previewRaw),
+    });
+    return null;
+  }
+
+  const confirmation = await Swal.fire({
+    icon: "warning",
+    title: t("invoicing_period.invoice_collection_actions.preview.title", {
+      action: getBulkActionLabel(action),
+    }),
+    html: renderBulkActionPreviewHtml(previewRaw),
+    input: "text",
+    inputLabel: t("invoicing_period.invoice_collection_actions.preview.confirmation_label", {
+      phrase: preview.confirmationPhrase,
+    }),
+    inputPlaceholder: preview.confirmationPhrase,
+    showCancelButton: true,
+    confirmButtonText: t("invoicing_period.invoice_collection_actions.preview.confirm_button"),
+    cancelButtonText: commonText("cancel", "Annuller"),
+    inputValidator: (value) => {
+      if (String(value ?? "").trim() !== String(preview.confirmationPhrase ?? "")) {
+        return t("invoicing_period.invoice_collection_actions.preview.confirmation_mismatch", {
+          phrase: preview.confirmationPhrase,
+        });
+      }
+      return undefined;
+    },
+  });
+
+  return confirmation.isConfirmed ? confirmation : null;
+};
+
 const runCollectionBulkAction = async (
  action: string,
  ids: any[] = collectionIds(),
@@ -2184,50 +2259,13 @@ const runCollectionBulkAction = async (
        }
      );
    }
-    const previewRaw = getApiPayload(previewResponse);
-    const preview = normalizeInvoiceCollectionActionPreview(previewRaw, ids.length);
-    const blockers = preview.blockers;
-    const changedCount = preview.changedCount;
-
-    if (blockers.length > 0 || changedCount === 0) {
-      await Swal.fire({
-        icon: blockers.length > 0 ? "error" : "info",
-        title:
-          blockers.length > 0
-            ? t("invoicing_period.invoice_collection_actions.preview.blocked_title")
-            : t("invoicing_period.invoice_collection_actions.preview.no_changes_title"),
-        html: renderBulkActionPreviewHtml(previewRaw),
-      });
-      return;
-    }
-
-    const confirmation = await Swal.fire({
-      icon: "warning",
-      title: t("invoicing_period.invoice_collection_actions.preview.title", {
-        action: getBulkActionLabel(action),
-      }),
-      html: renderBulkActionPreviewHtml(previewRaw),
-      input: "text",
-      inputLabel: t("invoicing_period.invoice_collection_actions.preview.confirmation_label", {
-        phrase: preview.confirmationPhrase,
-      }),
-      inputPlaceholder: preview.confirmationPhrase,
-      showCancelButton: true,
-      confirmButtonText: t("invoicing_period.invoice_collection_actions.preview.confirm_button"),
-      cancelButtonText: commonText("cancel", "Annuller"),
-      inputValidator: (value) => {
-        if (String(value ?? "").trim() !== String(preview.confirmationPhrase ?? "")) {
-          return t("invoicing_period.invoice_collection_actions.preview.confirmation_mismatch", {
-            phrase: preview.confirmationPhrase,
-          });
-        }
-        return undefined;
-      },
-    });
-
-    if (!confirmation.isConfirmed) {
+    let previewRaw = getApiPayload(previewResponse);
+    let preview = normalizeInvoiceCollectionActionPreview(previewRaw, ids.length);
+    let confirmation = await confirmCollectionBulkActionPreview(action, previewRaw, preview);
+    if (!confirmation) {
      return;
    }
+    let confirmationText = confirmation.value;

    let applyResponse: any;
    if (useTreeAction) {
@@ -2240,6 +2278,23 @@ const runCollectionBulkAction = async (
        if (!isInvoiceCollectionTreeActionUnavailable(error)) {
          throw error;
        }
+        const legacyPreviewResponse = await SessionUser.objects.collectedOrderInvoices.functions.bulk_action_preview(
+          action,
+          ids,
+          options,
+          localeValue(),
+          {
+            customer_number: activeSnapshot.value?.customer_number,
+            snapshot_revision: activeSnapshot.value?.snapshot_revision,
+          }
+        );
+        previewRaw = getApiPayload(legacyPreviewResponse);
+        preview = normalizeInvoiceCollectionActionPreview(previewRaw, ids.length);
+        confirmation = await confirmCollectionBulkActionPreview(action, previewRaw, preview);
+        if (!confirmation) {
+          return;
+        }
+        confirmationText = confirmation.value;
        useTreeAction = false;
      }
    }
@@ -2249,7 +2304,7 @@ const runCollectionBulkAction = async (
        action,
        invoice_collection_ids: ids,
        options,
-        confirmation_text: confirmation.value,
+        confirmation_text: confirmationText,
        locale: localeValue(),
        customer_number: activeSnapshot.value?.customer_number,
        snapshot_revision: activeSnapshot.value?.snapshot_revision,
@@ -2285,7 +2340,7 @@ const runCollectionBulkAction = async (
    rootNodes.value.forEach((node) => collectConsumedNodeKeys(node));
    const consumedKeys = checkedKeys.value.filter((key) => consumedNodeKeys.has(String(key)));
    if (activeSnapshot.value) {
-      const refreshedSnapshot = await refreshSnapshot({ force: true, consumedKeys });
+      const refreshedSnapshot = await refreshSnapshotAfterMutation({ consumedKeys });
      if (!refreshedSnapshot) {
        const consumed = new Set(consumedKeys.map(String));
        checkedKeys.value = checkedKeys.value.filter((key) => !consumed.has(String(key)));
@@ -2297,7 +2352,7 @@ const runCollectionBulkAction = async (
      icon: "success",
      title: t("invoicing_period.invoice_collection_actions.success_title"),
      text: t("invoicing_period.invoice_collection_actions.success_text", {
-        count: applied?.result?.changed_count ?? applied?.summary?.changed_count ?? changedCount,
+        count: applied?.result?.changed_count ?? applied?.summary?.changed_count ?? preview.changedCount,
      }),
      timer: 2000,
      showConfirmButton: false,
@@ -2305,7 +2360,7 @@ const runCollectionBulkAction = async (
    emit("refresh");
  } catch (error: any) {
    if (activeSnapshot.value && Number(error?.response?.status) === 409) {
-      await refreshSnapshot({ force: true });
+      await refreshSnapshotAfterMutation();
    }
    await Swal.fire({
      icon: "error",
@@ -326,6 +326,7 @@ export const buildCompleteSnapshotRootNodes = (snapshot: CompleteInvoicingPeriod
  const customer = snapshot.customer;
  const collectionNodes = [...snapshot.collections].sort((left, right) => left.id - right.id).map((collection) => {
    const node = makeCollectionNode(collection.id, collection.orders, customer, {
+      label: labels.collection(collection.id),
      collectionSummary: collection,
      invoiceState: asString(collection.invoice_state ?? collection.state),
      completeOrderCount: collection.complete_order_count,
@@ -114,8 +114,8 @@ vi.mock("vue-i18n", () => ({
 vi.mock("@/components/displays/buttons/EditableTableColumn.vue", () => ({
  default: {
    name: "EditableTableColumn",
-    props: ["object", "column", "parseFunction"],
-    template: `<span class="editable-table-column-stub">{{ parseFunction ? parseFunction(object?.[column]) : object?.[column] }}</span>`,
+    props: ["object", "column", "parseFunction", "editFunction", "loadList", "cellTestId"],
+    template: `<button type="button" class="editable-table-column-stub" :data-testid="cellTestId" @click="editFunction(object.id, column, object[column], loadList)">{{ parseFunction ? parseFunction(object?.[column]) : object?.[column] }}</button>`,
  },
 }));

@@ -344,6 +344,17 @@ const expandNode = async (wrapper, key) => {
 describe("InvoicingPeriodObjectTree", () => {
  beforeEach(() => {
    vi.clearAllMocks();
+    Swal.fire.mockReset().mockResolvedValue({ isConfirmed: false });
+    mocks.request.mockImplementation(async (url, method) => {
+      if (url === "/order/items" && method === "GET") {
+        return {
+          data: {
+            data: mocks.orderItems,
+          },
+        };
+      }
+      return { data: { data: [] } };
+    });
    mocks.orderItems.splice(0, mocks.orderItems.length, {
      id: 501,
      order_id: 9001,
@@ -402,6 +413,89 @@ describe("InvoicingPeriodObjectTree", () => {
    expect(price.find(".editable-table-column-stub").exists()).toBe(true);
  });

+  it("preserves untouched snapshot item values during an inline edit", async () => {
+    mocks.orderItems[0].reference = "KEEP-REFERENCE";
+    mocks.orderItems[0].notes = "KEEP-NOTES";
+    mocks.request.mockImplementation(async (url, method) => {
+      if (url === "/superuser/invoicing/period/tree" && method === "GET") {
+        return snapshotResponse("rev-edit");
+      }
+      return { data: { data: [] } };
+    });
+    Swal.fire.mockResolvedValueOnce({ isConfirmed: true, value: "135" });
+    const wrapper = mountTree({ capabilities: { object_tree_v2: true } });
+    await flushPromises();
+
+    await expandNode(wrapper, "collected_order_invoice:3001");
+    await expandNode(wrapper, "category:3001:collection_orders");
+    await expandNode(wrapper, "order:9001");
+    await expandNode(wrapper, "category:9001:order_items");
+    await wrapper.get('[data-testid="invoice-period-tree-field-editor-order_item:501-price"]').trigger("click");
+    await flushPromises();
+
+    expect(mocks.request).toHaveBeenCalledWith("/order/items", "PUT", {
+      id: 501,
+      price: 135,
+      notes: "KEEP-NOTES",
+      reference: "KEEP-REFERENCE",
+      quantity: 2,
+    });
+    expect(mocks.request.mock.calls.filter(([url]) => url === "/superuser/invoicing/period/tree")).toHaveLength(2);
+  });
+
+  it("rejects partial snapshot items before a full inline update can erase text fields", async () => {
+    delete mocks.orderItems[0].notes;
+    mocks.request.mockImplementation(async (url, method) => {
+      if (url === "/superuser/invoicing/period/tree" && method === "GET") {
+        return snapshotResponse("rev-partial-item");
+      }
+      return { data: { data: [] } };
+    });
+    const wrapper = mountTree({ capabilities: { object_tree_v2: true } });
+    await flushPromises();
+
+    await expandNode(wrapper, "collected_order_invoice:3001");
+    await expandNode(wrapper, "category:3001:collection_orders");
+    await expandNode(wrapper, "order:9001");
+    await expandNode(wrapper, "category:9001:order_items");
+    await wrapper.get('[data-testid="invoice-period-tree-field-editor-order_item:501-price"]').trigger("click");
+    await flushPromises();
+
+    expect(mocks.request.mock.calls.some(([url, method]) => url === "/order/items" && method === "PUT")).toBe(false);
+    expect(Swal.fire).toHaveBeenCalledWith(expect.objectContaining({ icon: "error" }));
+  });
+
+  it("retries a transient snapshot failure after an inline mutation", async () => {
+    let snapshotCalls = 0;
+    mocks.request.mockImplementation(async (url, method) => {
+      if (url === "/superuser/invoicing/period/tree" && method === "GET") {
+        snapshotCalls += 1;
+        if (snapshotCalls === 2) {
+          throw new Error("temporary snapshot failure");
+        }
+        return snapshotResponse(`rev-retry-${snapshotCalls}`, { queue_economic: true });
+      }
+      return { data: { data: [] } };
+    });
+    Swal.fire.mockResolvedValueOnce({ isConfirmed: true, value: "135" });
+    const wrapper = mountTree({ capabilities: { object_tree_v2: true } });
+    await flushPromises();
+
+    await expandNode(wrapper, "collected_order_invoice:3001");
+    await expandNode(wrapper, "category:3001:collection_orders");
+    await expandNode(wrapper, "order:9001");
+    await expandNode(wrapper, "category:9001:order_items");
+    await wrapper.get('[data-testid="invoice-period-tree-field-editor-order_item:501-price"]').trigger("click");
+    await flushPromises();
+
+    expect(snapshotCalls).toBe(3);
+    expect(
+      wrapper
+        .get('[data-testid="invoice-period-tree-action-wheel-collected_order_invoice:3001"]')
+        .attributes("data-action-count")
+    ).toBe("1");
+  });
+
  it("keeps a visible expand-all toggle and reuses loaded branches after collapse", async () => {
    const wrapper = mountTree();
    const toggle = () => wrapper.get('[data-testid="invoice-period-tree-toggle-all"]');
@@ -796,6 +890,73 @@ describe("InvoicingPeriodObjectTree", () => {
    expect(mocks.request.mock.calls.some(([url]) => url === "/order/items")).toBe(false);
  });

+  it("refreshes an active snapshot when parent transaction data changes", async () => {
+    mocks.request
+      .mockResolvedValueOnce(snapshotResponse("rev-parent-1", { queue_economic: true }))
+      .mockResolvedValueOnce(snapshotResponse("rev-parent-2", { queue_economic: true }));
+    const wrapper = mountTree({ capabilities: { object_tree_v2: true } });
+    await flushPromises();
+
+    await wrapper.setProps({
+      transactions: [{ ...mocks.order, booked: true, total_net_amount: 245 }],
+    });
+    await flushPromises();
+
+    expect(mocks.request.mock.calls.filter(([url]) => url === "/superuser/invoicing/period/tree")).toHaveLength(2);
+  });
+
+  it("creates and confirms a legacy preview before falling back from tree apply", async () => {
+    const actions = { queue_economic: true };
+    mocks.request.mockResolvedValueOnce(snapshotResponse("rev-fallback", actions));
+    SessionUser.objects.collectedOrderInvoices.functions.period_tree_action_preview.mockResolvedValueOnce({
+      data: {
+        data: {
+          preview_id: "tree-preview",
+          confirmation_phrase: "CONFIRM TREE",
+          summary: { collection_count: 1, changed_count: 1 },
+          blockers: [],
+        },
+      },
+    });
+    SessionUser.objects.collectedOrderInvoices.functions.period_tree_action_apply.mockRejectedValueOnce({
+      response: { status: 404 },
+    });
+    SessionUser.objects.collectedOrderInvoices.functions.bulk_action_preview.mockResolvedValueOnce({
+      data: {
+        data: {
+          preview_id: "legacy-preview",
+          confirmation_phrase: "CONFIRM LEGACY",
+          summary: { collection_count: 1, changed_count: 1 },
+          blockers: [],
+        },
+      },
+    });
+    SessionUser.objects.collectedOrderInvoices.functions.bulk_action_apply.mockResolvedValueOnce({
+      data: { data: { jobs: [{ id: 1 }] } },
+    });
+    Swal.fire
+      .mockResolvedValueOnce({ isConfirmed: true, value: "CONFIRM TREE" })
+      .mockResolvedValueOnce({ isConfirmed: true, value: "CONFIRM LEGACY" });
+
+    const wrapper = mountTree({ capabilities: { object_tree_v2: true } });
+    await flushPromises();
+    await wrapper.get('[data-node-key="collected_order_invoice:3001"] .b-checkbox-stub').trigger("click");
+    await flushPromises();
+    await wrapper.get('[data-testid="invoice-period-tree-actions-trigger-collected_order_invoice"]').trigger("click");
+    await wrapper.get('[data-testid="invoice-period-tree-action-collection:queue-economic"]').trigger("click");
+    await flushPromises();
+
+    expect(SessionUser.objects.collectedOrderInvoices.functions.bulk_action_preview).toHaveBeenCalledTimes(1);
+    expect(SessionUser.objects.collectedOrderInvoices.functions.bulk_action_apply).toHaveBeenCalledWith(
+      expect.objectContaining({
+        preview_id: "legacy-preview",
+        confirmation_text: "CONFIRM LEGACY",
+      })
+    );
+    expect(Swal.fire.mock.calls.some(([options]) => options?.icon === "success")).toBe(true);
+    expect(Swal.fire.mock.calls.some(([options]) => options?.icon === "error")).toBe(false);
+  });
+
  it("keeps legacy collection mutations disabled while a superseding snapshot request is pending", async () => {
    const pending = [];
    mocks.request.mockImplementation((url, _method, _parameters, _catcher, _then, options) =>
@@ -89,6 +89,7 @@ describe("invoicing period selected-customer tree snapshot", () => {
    });

    const roots = buildCompleteSnapshotRootNodes(snapshot, labels);
+    expect(roots[0].label).toBe("Collection 3001");
    expect(roots[0].meta).toMatchObject({
      periodOrderCount: 1,
      periodTotalNetAmount: 200,