[FIX] helpers: Add generic helper to sanitize sheet names

Some commands or functionalities can dispatch the creation of new sheets. Unfortunately, the sheet names cannot contain specific characters. This revision introduces a generic helper to sanitize the names before using it inside the CREATE_SHEET command. closes #5298 Task: 4347719 X-original-commit: e84a864 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
odoo · Dec 3, 2024 · 62762cf · 62762cf
1 parent 9ef03b3
commit 62762cf
Show file tree

Hide file tree

Showing 10 changed files with 30 additions and 24 deletions.
diff --git a/src/constants.ts b/src/constants.ts
@@ -245,8 +245,8 @@ export const DEBOUNCE_TIME = 200;
 export const MESSAGE_VERSION = 1;
 
 // Sheets
-export const FORBIDDEN_SHEET_CHARS = ["'", "*", "?", "/", "\\", "[", "]"] as const;
-export const FORBIDDEN_IN_EXCEL_REGEX = /'|\*|\?|\/|\\|\[|\]/;
+export const FORBIDDEN_SHEETNAME_CHARS = ["'", "*", "?", "/", "\\", "[", "]"] as const;
+export const FORBIDDEN_SHEETNAME_CHARS_IN_EXCEL_REGEX = /'|\*|\?|\/|\\|\[|\]/;
 
 // Cells
 export const FORMULA_REF_IDENTIFIER = "|";

diff --git a/src/helpers/misc.ts b/src/helpers/misc.ts
@@ -1,11 +1,13 @@
 //------------------------------------------------------------------------------
 // Miscellaneous
 //------------------------------------------------------------------------------
-import { NEWLINE } from "../constants";
+import { FORBIDDEN_SHEETNAME_CHARS_IN_EXCEL_REGEX, NEWLINE } from "../constants";
 import { ConsecutiveIndexes, Lazy, UID } from "../types";
 import { SearchOptions } from "../types/find_and_replace";
 import { Cloneable, DebouncedFunction } from "./../types/misc";
 
+const sanitizeSheetNameRegex = new RegExp(FORBIDDEN_SHEETNAME_CHARS_IN_EXCEL_REGEX, "g");
+
 /**
  * Remove quotes from a quoted string
  * ```js
@@ -109,6 +111,11 @@ export function getCanonicalSymbolName(symbolName: string): string {
   return symbolName;
 }
 
+/** Replace the excel-excluded characters of a sheetName */
+export function sanitizeSheetName(sheetName: string, replacementChar: string = " "): string {
+  return sheetName.replace(sanitizeSheetNameRegex, replacementChar);
+}
+
 export function clip(val: number, min: number, max: number): number {
   return val < min ? min : val > max ? max : val;
 }

diff --git a/src/helpers/ui/sheet_interactive.ts b/src/helpers/ui/sheet_interactive.ts
@@ -1,4 +1,4 @@
-import { FORBIDDEN_SHEET_CHARS } from "../../constants";
+import { FORBIDDEN_SHEETNAME_CHARS } from "../../constants";
 import { _t } from "../../translation";
 import { CommandResult, SpreadsheetChildEnv, UID } from "../../types";
 
@@ -20,7 +20,7 @@ export function interactiveRenameSheet(
     env.raiseError(
       _t(
         "Some used characters are not allowed in a sheet name (Forbidden characters are %s).",
-        FORBIDDEN_SHEET_CHARS.join(" ")
+        FORBIDDEN_SHEETNAME_CHARS.join(" ")
       ),
       errorCallback
     );

diff --git a/src/index.ts b/src/index.ts
@@ -88,6 +88,7 @@ import {
   positionToZone,
   reduceZoneOnDeletion,
   rgbaToHex,
+  sanitizeSheetName,
   splitReference,
   toCartesian,
   toUnboundedZone,
@@ -340,6 +341,7 @@ export const helpers = {
   createPivotFormula,
   areDomainArgsFieldsValid,
   splitReference,
+  sanitizeSheetName,
 };
 
 export const links = {

diff --git a/src/migrations/migration_steps.ts b/src/migrations/migration_steps.ts
@@ -1,9 +1,5 @@
-import {
-  BACKGROUND_CHART_COLOR,
-  FORBIDDEN_IN_EXCEL_REGEX,
-  FORMULA_REF_IDENTIFIER,
-} from "../constants";
-import { getItemId } from "../helpers";
+import { BACKGROUND_CHART_COLOR, FORMULA_REF_IDENTIFIER } from "../constants";
+import { getItemId, sanitizeSheetName } from "../helpers";
 import { toXC } from "../helpers/coordinates";
 import { getMaxObjectId } from "../helpers/pivot/pivot_helpers";
 import { DEFAULT_TABLE_CONFIG } from "../helpers/table_presets";
@@ -109,13 +105,13 @@ migrationStepRegistry
     versionFrom: "7",
     migrate(data: any): any {
       const namesTaken: string[] = [];
-      const globalForbiddenInExcel = new RegExp(FORBIDDEN_IN_EXCEL_REGEX, "g");
       for (let sheet of data.sheets || []) {
         if (!sheet.name) {
           continue;
         }
         const oldName = sheet.name;
-        const escapedName: string = oldName.replace(globalForbiddenInExcel, "_");
+        sanitizeSheetName;
+        const escapedName: string = sanitizeSheetName(oldName, "_");
         let i = 1;
         let newName = escapedName;
         while (namesTaken.includes(newName)) {

diff --git a/src/plugins/core/sheet.ts b/src/plugins/core/sheet.ts
@@ -1,4 +1,4 @@
-import { FORBIDDEN_IN_EXCEL_REGEX } from "../../constants";
+import { FORBIDDEN_SHEETNAME_CHARS_IN_EXCEL_REGEX } from "../../constants";
 import {
   createDefaultRows,
   deepCopy,
@@ -649,7 +649,7 @@ export class SheetPlugin extends CorePlugin<SheetState> implements SheetState {
     ) {
       return CommandResult.DuplicatedSheetName;
     }
-    if (FORBIDDEN_IN_EXCEL_REGEX.test(name!)) {
+    if (FORBIDDEN_SHEETNAME_CHARS_IN_EXCEL_REGEX.test(name!)) {
       return CommandResult.ForbiddenCharactersInSheetName;
     }
     return CommandResult.Success;

diff --git a/src/plugins/ui_feature/insert_pivot.ts b/src/plugins/ui_feature/insert_pivot.ts
@@ -1,4 +1,5 @@
-import { FORBIDDEN_IN_EXCEL_REGEX, PIVOT_TABLE_CONFIG } from "../../constants";
+import { PIVOT_TABLE_CONFIG } from "../../constants";
+import { sanitizeSheetName } from "../../helpers";
 import { createPivotFormula } from "../../helpers/pivot/pivot_helpers";
 import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_pivot";
 import { getZoneArea, positionToZone } from "../../helpers/zones";
@@ -113,7 +114,7 @@ export class InsertPivotPlugin extends UIPlugin {
   private getPivotDuplicateSheetName(pivotName: string) {
     let i = 1;
     const names = this.getters.getSheetIds().map((id) => this.getters.getSheetName(id));
-    const sanitizedName = pivotName.replace(new RegExp(FORBIDDEN_IN_EXCEL_REGEX, "g"), " ");
+    const sanitizedName = sanitizeSheetName(pivotName);
     let name = sanitizedName;
     while (names.includes(name)) {
       name = `${sanitizedName} (${i})`;

diff --git a/tests/model/model_import_export.test.ts b/tests/model/model_import_export.test.ts
@@ -4,7 +4,7 @@ import {
   DEFAULT_CELL_HEIGHT,
   DEFAULT_CELL_WIDTH,
   DEFAULT_REVISION_ID,
-  FORBIDDEN_SHEET_CHARS,
+  FORBIDDEN_SHEETNAME_CHARS,
 } from "../../src/constants";
 import { toCartesian, toZone } from "../../src/helpers";
 import { DEFAULT_TABLE_CONFIG } from "../../src/helpers/table_presets";
@@ -212,7 +212,7 @@ describe("Migrations", () => {
       stacked: false,
     });
   });
-  test.each(FORBIDDEN_SHEET_CHARS)("migrate version 7: sheet Names", (char) => {
+  test.each(FORBIDDEN_SHEETNAME_CHARS)("migrate version 7: sheet Names", (char) => {
     const model = new Model({
       version: 7,
       sheets: [

diff --git a/tests/pivots/pivot_plugin.test.ts b/tests/pivots/pivot_plugin.test.ts
@@ -1,5 +1,5 @@
 import { CommandResult } from "../../src";
-import { FORBIDDEN_SHEET_CHARS } from "../../src/constants";
+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";
 import { createModelFromGrid, toCellPosition } from "../test_helpers/helpers";
@@ -191,7 +191,7 @@ describe("Pivot plugin", () => {
       A2: "Alice",
     };
     const model = createModelFromGrid(grid);
-    addPivot(model, "A1:A2", { name: `forbidden: ${FORBIDDEN_SHEET_CHARS}` }, "pivot1");
+    addPivot(model, "A1:A2", { name: `forbidden: ${FORBIDDEN_SHEETNAME_CHARS}` }, "pivot1");
     model.dispatch("DUPLICATE_PIVOT_IN_NEW_SHEET", {
       newPivotId: "pivot2",
       newSheetId: "Sheet2",

diff --git a/tests/sheet/sheets_plugin.test.ts b/tests/sheet/sheets_plugin.test.ts
@@ -1,4 +1,4 @@
-import { FORBIDDEN_SHEET_CHARS } from "../../src/constants";
+import { FORBIDDEN_SHEETNAME_CHARS } from "../../src/constants";
 import { getCanonicalSymbolName, numberToLetters, toZone } from "../../src/helpers";
 import { Model } from "../../src/model";
 import { CommandResult } from "../../src/types";
@@ -39,7 +39,7 @@ import "../test_helpers/helpers";
 import { createEqualCF, testUndoRedo, toRangesData } from "../test_helpers/helpers";
 
 jest.mock("../../src/helpers/uuid", () => require("../__mocks__/uuid"));
-
+FORBIDDEN_SHEETNAME_CHARS;
 describe("sheets", () => {
   test("can create a new sheet, then undo, then redo", () => {
     const model = new Model();
@@ -141,7 +141,7 @@ describe("sheets", () => {
     ).toBeCancelledBecause(CommandResult.DuplicatedSheetName);
   });
 
-  test.each(FORBIDDEN_SHEET_CHARS)("Cannot rename a sheet with a %s in the name", (char) => {
+  test.each(FORBIDDEN_SHEETNAME_CHARS)("Cannot rename a sheet with a %s in the name", (char) => {
     const model = new Model();
     expect(
       renameSheet(model, model.getters.getActiveSheetId(), `my life ${char}`)