[FIX] pivot: Ensure computed measure range adaptation

Currently, ODOO pivots computed measures are not properly updated upon
sheet structure modification. To be precise, their definition, which is
stored in the core plugin `PivotCorePlugin` is properly updated but the
runtime definition, stored in `PivotUIPlugin`, is not.

This occurs because the mecanism to invalidate the runtime definition
explicitely ignores the ODOO pivots. histoically, this was set up to
avoid useless reloading of ODOO pivots which could end up making server
calls but this logic is properly handled in the function `onDefinitionChange`.

We can see that in the case of spreadsheet pivots, we already
notify all plugins of such a change, but by "pure accident", as we
dispatch an "UPDATE_PIVOT" command at every range adaptation, regardless
of whether it was necessary or not. This means that the spreadsheet
pivots beneficiated of two mecanisms to update their runtime (in core,
an UPDATE_PIVOT, and the `invalidateEvaluationCommands` mecanism) which
means that invalidation work was done two times.

The investigation also led to the discovery of a missing check on the
command "ADD_PIVOT" which has been reported in  https://www.odoo.com/odoo/2328/tasks/5360591

We also noted that there is a double handling of commands between the
handling of `invalidateEvaluationCommands` and the specific command
handlers in `PivotUIPlugin`. We could clean this up in master.

Note that additional tests regarding the Odoo pivots will be added in
Odoo repository to ensure the validity of the fix.

closes #7567

Task: 5358213
X-original-commit: 246af93
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>

Author: rrahir

packages/o-spreadsheet-engine/src/helpers/pivot/pivot_registry.ts

@@ -30,7 +30,6 @@ type PivotDefinitionConstructor = new (
 export interface PivotRegistryItem {
   ui: PivotUIConstructor;
   definition: PivotDefinitionConstructor;
-  externalData: boolean;
   dateGranularities: string[];
   datetimeGranularities: string[];
   isMeasureCandidate: (field: PivotField) => boolean;
@@ -55,7 +54,6 @@ const dateGranularities = [
 pivotRegistry.add("SPREADSHEET", {
   ui: SpreadsheetPivot,
   definition: SpreadsheetPivotRuntimeDefinition,
-  externalData: false,
   dateGranularities: [...dateGranularities],
   datetimeGranularities: [...dateGranularities, "hour_number", "minute_number", "second_number"],
   isMeasureCandidate: (field: PivotField) => field.type !== "boolean",

packages/o-spreadsheet-engine/src/plugins/core/pivot.ts

@@ -322,18 +322,19 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
       if (!pivot) {
         continue;
       }
-      for (const measure of pivot.definition.measures) {
+      const def = deepCopy(pivot.definition);
+
+      for (const measure of def.measures) {
         if (measure.computedBy?.formula === formulaString) {
-          const measureIndex = pivot.definition.measures.indexOf(measure);
-          this.history.update(
-            "pivots",
-            pivotId,
-            "definition",
-            "measures",
-            measureIndex,
-            "computedBy",
-            { formula: newFormulaString, sheetId }
-          );
+          const measureIndex = def.measures.indexOf(measure);
+          if (measureIndex !== -1) {
+            def.measures[measureIndex].computedBy = {
+              formula: newFormulaString,
+              sheetId,
+            };
+          }
+
+          this.dispatch("UPDATE_PIVOT", { pivotId, pivot: def });
         }
       }
     }

packages/o-spreadsheet-engine/src/plugins/core/spreadsheet_pivot.ts

@@ -45,6 +45,10 @@ export class SpreadsheetPivotCorePlugin extends CorePlugin {
         const range = this.getters.getRangeFromZone(sheetId, zone);
         const adaptedRange = adaptPivotRange(range, applyChange);
 
+        if (adaptedRange === range) {
+          return;
+        }
+
         const dataSet = adaptedRange && {
           sheetId: adaptedRange.sheetId,
           zone: adaptedRange.zone,

packages/o-spreadsheet-engine/src/plugins/ui_core_views/pivot_ui.ts

@@ -1,6 +1,7 @@
 import { astToFormula } from "../../formulas/formula_formatter";
 import { Token } from "../../formulas/tokenizer";
 import { toScalar } from "../../functions/helper_matrices";
+import { deepCopy } from "../../helpers";
 import { deepEquals, getUniqueText } from "../../helpers/misc";
 import {
   getFirstPivotFunction,
@@ -65,9 +66,7 @@ export class PivotUIPlugin extends CoreViewPlugin {
   handle(cmd: Command) {
     if (invalidateEvaluationCommands.has(cmd.type)) {
       for (const pivotId of this.getters.getPivotIds()) {
-        if (!pivotRegistry.get(this.getters.getPivotCoreDefinition(pivotId).type).externalData) {
-          this.setupPivot(pivotId, { recreate: true });
-        }
+        this.setupPivot(pivotId, { recreate: true });
       }
     }
     switch (cmd.type) {
@@ -320,7 +319,7 @@ export class PivotUIPlugin extends CoreViewPlugin {
   }
 
   setupPivot(pivotId: UID, { recreate } = { recreate: false }) {
-    const definition = this.getters.getPivotCoreDefinition(pivotId);
+    const definition = deepCopy(this.getters.getPivotCoreDefinition(pivotId));
     if (!(pivotId in this.pivots)) {
       const Pivot = withPivotPresentationLayer(pivotRegistry.get(definition.type).ui);
       this.pivots[pivotId] = new Pivot(this.custom, { definition, getters: this.getters });

tests/collaborative/collaborative_history.test.ts

@@ -1,5 +1,4 @@
 import { DEFAULT_REVISION_ID, MESSAGE_VERSION } from "@odoo/o-spreadsheet-engine/constants";
-import { pivotRegistry } from "@odoo/o-spreadsheet-engine/helpers/pivot/pivot_registry";
 import { LineChartDefinition } from "@odoo/o-spreadsheet-engine/types/chart/line_chart";
 import { StateUpdateMessage } from "@odoo/o-spreadsheet-engine/types/collaborative/transport_service";
 import { Model } from "../../src";
@@ -1025,7 +1024,6 @@ describe("Collaborative local history", () => {
   });
 
   test("remove pivot, new user joins, then undo", () => {
-    pivotRegistry.get("SPREADSHEET").externalData = true; // simulate external pivot
     const network = new MockTransportService();
     const data = {
       revisionId: DEFAULT_REVISION_ID,
@@ -1073,7 +1071,6 @@ describe("Collaborative local history", () => {
     const bob = new Model(data, configBob, messages);
     undo(alice);
     expect(getEvaluatedCell(bob, "B3").value).toEqual(10);
-    pivotRegistry.get("SPREADSHEET").externalData = false;
   });
 
   test("Concurrently undo a command on which another is based", () => {

tests/pivots/pivot_calculated_measure.test.ts

@@ -1,10 +1,12 @@
 import {
   activateSheet,
-  addColumns,
+  addRows,
   createSheet,
   deleteSheet,
+  redo,
   setCellContent,
   setFormat,
+  undo,
 } from "../test_helpers/commands_helpers";
 import { getEvaluatedCell, getEvaluatedGrid } from "../test_helpers/getters_helpers";
 import { createModelFromGrid } from "../test_helpers/helpers";
@@ -1063,16 +1065,21 @@ describe("Pivot calculated measure", () => {
       ],
     });
     expect(getEvaluatedCell(model, "A4").value).toEqual(42);
-    addColumns(model, "before", "A", 1);
+    addRows(model, "before", 2, 1);
     expect(model.getters.getPivotCoreDefinition("1").measures).toEqual([
       {
         id: "calculated",
         fieldName: "calculated",
         aggregator: "sum",
-        computedBy: { formula: "=B3", sheetId },
+        computedBy: { formula: "=A4", sheetId },
       },
     ]);
-    expect(getEvaluatedCell(model, "B4").value).toEqual(42);
+    expect(getEvaluatedCell(model, "A5").value).toEqual(42);
+
+    undo(model);
+    expect(getEvaluatedCell(model, "A4").value).toEqual(42);
+    redo(model);
+    expect(getEvaluatedCell(model, "A5").value).toEqual(42);
   });
 
   test("references becomes invalid when sheet is deleted", () => {

tests/pivots/spreadsheet_pivot/spreadsheet_pivot.test.ts

@@ -10,6 +10,7 @@ import {
 } from "../../../src";
 import { positions, toZone } from "../../../src/helpers";
 import {
+  addRows,
   createSheet,
   deleteContent,
   deleteSheet,
@@ -645,6 +646,22 @@ describe("Spreadsheet Pivot", () => {
     );
   });
 
+  test("Modifying a sheet structure adapts the pivot range", () => {
+    const model = createModelWithPivot("A1:I5");
+    setCellContent(model, "A26", `=pivot(1)`);
+    expect(model.getters.getPivot("1").isValid()).toBeTruthy();
+    expect(getEvaluatedCell(model, "A26").value).toEqual("My pivot");
+    addRows(model, "before", 0, 1);
+    expect(model.getters.getPivot("1").isValid()).toBeTruthy();
+    expect(getEvaluatedCell(model, "A27").value).toEqual("My pivot");
+    undo(model);
+    expect(model.getters.getPivot("1").isValid()).toBeTruthy();
+    expect(getEvaluatedCell(model, "A26").value).toEqual("My pivot");
+    redo(model);
+    expect(model.getters.getPivot("1").isValid()).toBeTruthy();
+    expect(getEvaluatedCell(model, "A27").value).toEqual("My pivot");
+  });
+
   test("Sum with a field that contains a string should work", () => {
     const model = createModelWithPivot("A1:I5");
     updatePivot(model, "1", {

tests/pivots/spreadsheet_pivot/spreadsheet_pivot_side_panel.test.ts

@@ -743,11 +743,16 @@ describe("Spreadsheet pivot side panel", () => {
   test("Invalid pivot dimensions are displayed as such in the side panel", async () => {
     setCellContent(model, "A1", "ValidDimension");
     setCellContent(model, "A2", "10");
-    addPivot(model, "A1:A2", {
-      columns: [{ fieldName: "ValidDimension" }],
-      rows: [{ fieldName: "InvalidDimension" }],
-    });
-    env.openSidePanel("PivotSidePanel", { pivotId: "1" });
+    addPivot(
+      model,
+      "A1:A2",
+      {
+        columns: [{ fieldName: "ValidDimension" }],
+        rows: [{ fieldName: "InvalidDimension" }],
+      },
+      "2"
+    );
+    env.openSidePanel("PivotSidePanel", { pivotId: "2" });
     await nextTick();
     const pivotDimensionEls = fixture.querySelectorAll<HTMLElement>(".pivot-dimension")!;
     const validDimensionEl = pivotDimensionEls[0];