From d08e1f922763de06180659dc0609dcfeda03485a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= <lul@odoo.com>
Date: Fri, 27 Sep 2024 08:37:07 +0200
Subject: [PATCH] [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

Task: 4214188
---
 src/functions/helper_lookup.ts                |  7 +--
 src/functions/module_lookup.ts                | 10 ++--
 tests/pivots/pivot_calculated_measure.test.ts | 48 +++++++++++++++++++
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/src/functions/helper_lookup.ts b/src/functions/helper_lookup.ts
index 94bdc3e233..6cdc162065 100644
--- 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
index b17a9082cf..76d39d9b25 100644
--- 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
index ec9ac42f11..f49a7366c7 100644
--- 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 = {