[FIX] pivot: prevent duplicating pivot in error

Trying to duplicate a pivot in error would raise a traceback when trying to call pivot.getTableStructure(). closes #5393 Task: 4361353 X-original-commit: 707d198 Signed-off-by: Rémi Rahir (rar) <[email protected]>
odoo · Jan 2, 2025 · 8a21728 · 8a21728
1 parent 948abaa
commit 8a21728
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 5 deletions.
diff --git a/src/components/side_panel/pivot/pivot_title_section/pivot_title_section.ts b/src/components/side_panel/pivot/pivot_title_section/pivot_title_section.ts
@@ -1,7 +1,7 @@
 import { Component } from "@odoo/owl";
 import { ActionSpec } from "../../../../actions/action";
 import { _t } from "../../../../translation";
-import { SpreadsheetChildEnv, UID } from "../../../../types";
+import { CommandResult, SpreadsheetChildEnv, UID } from "../../../../types";
 import { TextInput } from "../../../text_input/text_input";
 import { CogWheelMenu } from "../../components/cog_wheel_menu/cog_wheel_menu";
 import { Section } from "../../components/section/section";
@@ -55,7 +55,14 @@ export class PivotTitleSection extends Component<Props, SpreadsheetChildEnv> {
       newPivotId,
       newSheetId,
     });
-    const text = result.isSuccessful ? _t("Pivot duplicated.") : _t("Pivot duplication failed");
+    let text: string;
+    if (result.isSuccessful) {
+      text = _t("Pivot duplicated.");
+    } else if (result.isCancelledBecause(CommandResult.PivotInError)) {
+      text = _t("Cannot duplicate a pivot in error.");
+    } else {
+      text = _t("Pivot duplication failed.");
+    }
     const type = result.isSuccessful ? "success" : "danger";
     this.env.notifyUser({
       text,

diff --git a/src/plugins/ui_feature/insert_pivot.ts b/src/plugins/ui_feature/insert_pivot.ts
@@ -5,12 +5,26 @@ import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_piv
 import { getZoneArea, positionToZone } from "../../helpers/zones";
 import { _t } from "../../translation";
 import { CellPosition, HeaderIndex, PivotTableCell, PivotTableData, UID, Zone } from "../../types";
-import { Command } from "../../types/commands";
+import { Command, CommandResult } from "../../types/commands";
 import { UIPlugin } from "../ui_plugin";
 
 export class InsertPivotPlugin extends UIPlugin {
   static getters = [] as const;
 
+  allowDispatch(cmd: Command) {
+    switch (cmd.type) {
+      case "DUPLICATE_PIVOT_IN_NEW_SHEET":
+        if (!this.getters.isExistingPivot(cmd.pivotId)) {
+          return CommandResult.PivotIdNotFound;
+        }
+        if (!this.getters.getPivot(cmd.pivotId).isValid()) {
+          return CommandResult.PivotInError;
+        }
+        break;
+    }
+    return CommandResult.Success;
+  }
+
   handle(cmd: Command) {
     switch (cmd.type) {
       case "INSERT_NEW_PIVOT":

diff --git a/src/types/commands.ts b/src/types/commands.ts
@@ -1315,6 +1315,7 @@ export const enum CommandResult {
   SheetIsHidden = "SheetIsHidden",
   InvalidTableResize = "InvalidTableResize",
   PivotIdNotFound = "PivotIdNotFound",
+  PivotInError = "PivotInError",
   EmptyName = "EmptyName",
   ValueCellIsInvalidFormula = "ValueCellIsInvalidFormula",
   InvalidDefinition = "InvalidDefinition",

diff --git a/tests/pivots/pivot_plugin.test.ts b/tests/pivots/pivot_plugin.test.ts
@@ -1,4 +1,4 @@
-import { CommandResult } from "../../src";
+import { CommandResult, Model } from "../../src";
 import { FORBIDDEN_SHEETNAME_CHARS } from "../../src/constants";
 import { EMPTY_PIVOT_CELL } from "../../src/helpers/pivot/table_spreadsheet_pivot";
 import { renameSheet, selectCell, setCellContent } from "../test_helpers/commands_helpers";
@@ -263,4 +263,15 @@ describe("Pivot plugin", () => {
       type: "HEADER",
     });
   });
+
+  test("DUPLICATE_PIVOT_IN_NEW_SHEET is prevented if the pivot is in error", () => {
+    const model = new Model();
+    addPivot(model, "A1:A2", {}, "pivot1");
+    const result = model.dispatch("DUPLICATE_PIVOT_IN_NEW_SHEET", {
+      newPivotId: "pivot2",
+      newSheetId: "Sheet2",
+      pivotId: "pivot1",
+    });
+    expect(result).toBeCancelledBecause(CommandResult.PivotInError);
+  });
 });
diff --git a/tests/pivots/spreadsheet_pivot/spreadsheet_pivot_side_panel.test.ts b/tests/pivots/spreadsheet_pivot/spreadsheet_pivot_side_panel.test.ts
@@ -35,9 +35,11 @@ describe("Spreadsheet pivot side panel", () => {
   let model: Model;
   let fixture: HTMLElement;
   let env: SpreadsheetChildEnv;
+  let notifyUser: jest.Mock;
 
   beforeEach(async () => {
-    ({ env, model, fixture } = await mountSpreadsheet());
+    notifyUser = jest.fn();
+    ({ env, model, fixture } = await mountSpreadsheet(undefined, { notifyUser }));
     // prettier-ignore
     const grid = {
       A1: "Customer", B1: "Product", C1: "Amount",
@@ -272,6 +274,19 @@ describe("Spreadsheet pivot side panel", () => {
     expect(model.getters.getPivotName("1")).toEqual(name);
   });
 
+  test("Cannot duplicate a pivot in error", async () => {
+    updatePivot(model, "1", { dataSet: undefined });
+    expect(model.getters.getPivot("1").isValid()).toBe(false);
+
+    await click(fixture, SELECTORS.COG_WHEEL);
+    await click(fixture, SELECTORS.DUPLICATE_PIVOT);
+    expect(notifyUser).toHaveBeenCalledWith({
+      sticky: false,
+      text: "Cannot duplicate a pivot in error.",
+      type: "danger",
+    });
+  });
+
   test("Can duplicate a pivot and undo the whole action with one step backward", async () => {
     await click(fixture, SELECTORS.COG_WHEEL);
     await click(fixture, SELECTORS.DUPLICATE_PIVOT);