[FIX] pivot: finer grained cycle detection

Steps to reproduce: - insert a fixed pivot (not dynamic!) - insert a new calculated measure - in the calculated measure formula, reference one of a cell which contains a header formula => the cell becomes a #CYCLE even though there's no circular dependendy between the calculated measure and the other dimension header closes #5026 Task: 4214188 Signed-off-by: Rémi Rahir (rar) <[email protected]>
odoo · Oct 11, 2024 · 77c19d8 · 77c19d8
1 parent 246d233
commit 77c19d8
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 6 deletions.
diff --git a/src/functions/helper_lookup.ts b/src/functions/helper_lookup.ts
@@ -2,7 +2,7 @@ import { zoneToXc } from "../helpers";
 import { _t } from "../translation";
 import { EvalContext, FunctionResultObject, Getters, Maybe, Range, UID } from "../types";
 import { EvaluationError, InvalidReferenceError } from "../types/errors";
-import { PivotCoreDefinition } from "../types/pivot";
+import { PivotCoreDefinition, PivotCoreMeasure } from "../types/pivot";
 
 /**
  * Get the pivot ID from the formula pivot ID.
@@ -37,7 +37,8 @@ export function assertDomainLength(domain: Maybe<FunctionResultObject>[]) {
 
 export function addPivotDependencies(
   evalContext: EvalContext,
-  coreDefinition: PivotCoreDefinition
+  coreDefinition: PivotCoreDefinition,
+  forMeasures: PivotCoreMeasure[]
 ) {
   //TODO This function can be very costly when used with PIVOT.VALUE and PIVOT.HEADER
   const dependencies: Range[] = [];
@@ -52,7 +53,7 @@ export function addPivotDependencies(
     dependencies.push(range);
   }
 
-  for (const measure of coreDefinition.measures) {
+  for (const measure of forMeasures) {
     if (measure.computedBy) {
       const formula = evalContext.getters.getMeasureCompiledFormula(measure);
       dependencies.push(...formula.dependencies.filter((range) => !range.invalidXc));

diff --git a/src/functions/module_lookup.ts b/src/functions/module_lookup.ts
@@ -709,7 +709,11 @@ export const PIVOT_VALUE = {
     const pivot = this.getters.getPivot(pivotId);
     const coreDefinition = this.getters.getPivotCoreDefinition(pivotId);
 
-    addPivotDependencies(this, coreDefinition);
+    addPivotDependencies(
+      this,
+      coreDefinition,
+      coreDefinition.measures.filter((m) => m.id === _measure)
+    );
     pivot.init({ reload: pivot.needsReevaluation });
     const error = pivot.assertIsValid({ throwOnError: false });
     if (error) {
@@ -747,7 +751,7 @@ export const PIVOT_HEADER = {
     assertDomainLength(domainArgs);
     const pivot = this.getters.getPivot(_pivotId);
     const coreDefinition = this.getters.getPivotCoreDefinition(_pivotId);
-    addPivotDependencies(this, coreDefinition);
+    addPivotDependencies(this, coreDefinition, []);
     pivot.init({ reload: pivot.needsReevaluation });
     const error = pivot.assertIsValid({ throwOnError: false });
     if (error) {
@@ -813,7 +817,7 @@ export const PIVOT = {
     const pivotId = getPivotId(_pivotFormulaId, this.getters);
     const pivot = this.getters.getPivot(pivotId);
     const coreDefinition = this.getters.getPivotCoreDefinition(pivotId);
-    addPivotDependencies(this, coreDefinition);
+    addPivotDependencies(this, coreDefinition, coreDefinition.measures);
     pivot.init({ reload: pivot.needsReevaluation });
     const error = pivot.assertIsValid({ throwOnError: false });
     if (error) {

diff --git a/tests/pivots/pivot_calculated_measure.test.ts b/tests/pivots/pivot_calculated_measure.test.ts
@@ -500,6 +500,54 @@ describe("Pivot calculated measure", () => {
     expect(getEvaluatedCell(model, "A3").message).toEqual("Circular reference");
   });
 
+  test("can depend on a cell containing another header", () => {
+    const grid = {
+      A1: '=PIVOT.HEADER(1, "Customer", "Alice")', // not referenced by the computed measure
+      A10: '=PIVOT.HEADER(1, "Customer", "Alice")', // referenced by the computed measure
+      A2: "Customer",
+      A3: "Alice",
+    };
+    const model = createModelFromGrid(grid);
+    const sheetId = model.getters.getActiveSheetId();
+    addPivot(model, "A2:A3", {
+      rows: [{ fieldName: "Customer" }],
+      measures: [
+        {
+          id: "calculated",
+          fieldName: "calculated",
+          aggregator: "sum",
+          computedBy: { formula: "=A10", sheetId },
+        },
+      ],
+    });
+    expect(getEvaluatedCell(model, "A10").value).toEqual("Alice");
+    expect(getEvaluatedCell(model, "A1").value).toEqual("Alice");
+  });
+
+  test("can depend on a cell containing another value", () => {
+    const grid = {
+      A1: '=PIVOT.VALUE(1, "Customer")', // not referenced by the computed measure
+      A10: '=PIVOT.VALUE(1, "Customer")', // referenced by the computed measure
+      A2: "Customer",
+      A3: "Alice",
+    };
+    const model = createModelFromGrid(grid);
+    const sheetId = model.getters.getActiveSheetId();
+    addPivot(model, "A2:A3", {
+      measures: [
+        { id: "Customer", fieldName: "Customer", aggregator: "sum" },
+        {
+          id: "calculated",
+          fieldName: "calculated",
+          aggregator: "sum",
+          computedBy: { formula: "=A10", sheetId },
+        },
+      ],
+    });
+    expect(getEvaluatedCell(model, "A10").value).toEqual(0);
+    expect(getEvaluatedCell(model, "A1").value).toEqual(0);
+  });
+
   test("measures symbols are scoped to the formula", () => {
     // prettier-ignore
     const grid = {