From 941317f65f28863abe3c589aa4a9cc1f207f3138 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= <lul@odoo.com>
Date: Mon, 30 Sep 2024 14:48:29 +0000
Subject: [PATCH] [FIX] pivot: remove forbidden chars when duplicating
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Steps to reproduce:

- insert a new pivot
- rename the pivot with one of the following character
  in its name: ', *, ?, /, \, [, ]
- duplicate the pivot

=> boom

The command to create the new sheet is rejected (the name contains invalid
characters). But we later tries to activate the sheet (which doesn doesn't
exist since it was never created)

closes odoo/o-spreadsheet#5047

Task: 4221325
X-original-commit: 31e1698c28195f8b2f66bd78f524994a3be86558
Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
---
 src/plugins/ui_feature/insert_pivot.ts | 25 +++++++++--------
 tests/pivots/pivot_plugin.test.ts      | 39 +++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/src/plugins/ui_feature/insert_pivot.ts b/src/plugins/ui_feature/insert_pivot.ts
index a964c60519..f86a1c82e3 100644
--- a/src/plugins/ui_feature/insert_pivot.ts
+++ b/src/plugins/ui_feature/insert_pivot.ts
@@ -1,4 +1,4 @@
-import { PIVOT_TABLE_CONFIG } from "../../constants";
+import { FORBIDDEN_IN_EXCEL_REGEX, PIVOT_TABLE_CONFIG } from "../../constants";
 import { createPivotFormula } from "../../helpers/pivot/pivot_helpers";
 import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_pivot";
 import { getZoneArea, positionToZone } from "../../helpers/zones";
@@ -85,7 +85,7 @@ export class InsertPivotPlugin extends UIPlugin {
     const position = this.getters.getSheetIds().indexOf(activeSheetId) + 1;
     const formulaId = this.getters.getPivotFormulaId(newPivotId);
     const newPivotName = this.getters.getPivotName(newPivotId);
-    this.dispatch("CREATE_SHEET", {
+    const result = this.dispatch("CREATE_SHEET", {
       sheetId: newSheetId,
       name: this.getPivotDuplicateSheetName(
         _t("%(newPivotName)s (Pivot #%(formulaId)s)", {
@@ -95,21 +95,24 @@ export class InsertPivotPlugin extends UIPlugin {
       ),
       position,
     });
-    this.dispatch("ACTIVATE_SHEET", { sheetIdFrom: activeSheetId, sheetIdTo: newSheetId });
-    this.dispatch("UPDATE_CELL", {
-      sheetId: newSheetId,
-      col: 0,
-      row: 0,
-      content: `=PIVOT(${formulaId})`,
-    });
+    if (result.isSuccessful) {
+      this.dispatch("ACTIVATE_SHEET", { sheetIdFrom: activeSheetId, sheetIdTo: newSheetId });
+      this.dispatch("UPDATE_CELL", {
+        sheetId: newSheetId,
+        col: 0,
+        row: 0,
+        content: `=PIVOT(${formulaId})`,
+      });
+    }
   }
 
   private getPivotDuplicateSheetName(pivotName: string) {
     let i = 1;
     const names = this.getters.getSheetIds().map((id) => this.getters.getSheetName(id));
-    let name = pivotName;
+    const sanitizedName = pivotName.replace(new RegExp(FORBIDDEN_IN_EXCEL_REGEX, "g"), " ");
+    let name = sanitizedName;
     while (names.includes(name)) {
-      name = `${pivotName} (${i})`;
+      name = `${sanitizedName} (${i})`;
       i++;
     }
     return name;
diff --git a/tests/pivots/pivot_plugin.test.ts b/tests/pivots/pivot_plugin.test.ts
index b3b94cb060..7d07ad2c09 100644
--- a/tests/pivots/pivot_plugin.test.ts
+++ b/tests/pivots/pivot_plugin.test.ts
@@ -1,6 +1,7 @@
 import { CommandResult } from "../../src";
+import { FORBIDDEN_SHEET_CHARS } from "../../src/constants";
 import { EMPTY_PIVOT_CELL } from "../../src/helpers/pivot/table_spreadsheet_pivot";
-import { selectCell, setCellContent } from "../test_helpers/commands_helpers";
+import { renameSheet, selectCell, setCellContent } from "../test_helpers/commands_helpers";
 import { createModelFromGrid, toCellPosition } from "../test_helpers/helpers";
 import { addPivot, updatePivot } from "../test_helpers/pivot_helpers";
 
@@ -163,4 +164,40 @@ describe("Pivot plugin", () => {
     const thirdCalculatedName = model.getters.generateNewCalculatedMeasureName(definition.measures);
     expect(thirdCalculatedName).toBe("Calculated measure 1");
   });
+
+  test("forbidden characters are removed from new sheet name when duplicating a pivot", () => {
+    const grid = {
+      A1: "Customer",
+      A2: "Alice",
+    };
+    const model = createModelFromGrid(grid);
+    addPivot(model, "A1:A2", { name: `forbidden: ${FORBIDDEN_SHEET_CHARS}` }, "pivot1");
+    model.dispatch("DUPLICATE_PIVOT_IN_NEW_SHEET", {
+      newPivotId: "pivot2",
+      newSheetId: "Sheet2",
+      pivotId: "pivot1",
+    });
+    expect(model.getters.getSheetName("Sheet2")).toEqual(
+      "forbidden:  , , , , , ,  (copy) (Pivot #2)"
+    );
+    expect(model.getters.getPivotName("pivot2")).toEqual("forbidden: ',*,?,/,\\,[,] (copy)");
+  });
+
+  test("sheet names with forbidden characters cannot conflict", () => {
+    const grid = {
+      A1: "Customer",
+      A2: "Alice",
+    };
+    const model = createModelFromGrid(grid);
+    const sheetId = model.getters.getActiveSheetId();
+    const name = "forbidden: /";
+    renameSheet(model, sheetId, "forbidden:   (copy) (Pivot #2)");
+    addPivot(model, "A1:A2", { name }, "pivot1");
+    model.dispatch("DUPLICATE_PIVOT_IN_NEW_SHEET", {
+      newPivotId: "pivot2",
+      newSheetId: "Sheet2",
+      pivotId: "pivot1",
+    });
+    expect(model.getters.getSheetName("Sheet2")).toEqual("forbidden:   (copy) (Pivot #2) (1)");
+  });
 });