[FIX] figure,chart: Enforce unicity of figure ids

The issue of duplicated chart ids was first addressed in PR #2102 by trying to defined chart ids per sheet (just like figures) Unfortunately, the fix was not appropriate for several reasons: 1. Some commands in Odoo were not dispatching the sheetId along with the chartId, making the mapping sheetId, chartId hazardous 2. There was absolutely 0 verification that the commands targeting a chartId were also providing a sheet Id that matched. So the said commands cannot be trusted either This commit is exploring the other solution that is forcing the unicity of a figure id. The data are adapted so that figures with a duplicated id well have the latter updated to ensure unicity. This commit also tries to solve the wrong `sheetId` parameter in `UPDATE_CHART` by simply ignoring it in the commands. It's not necesarry since we now have the unicity of figure ids. closes #2164 X-original-commit: f187fdd Signed-off-by: Pierre Rousseau (pro) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
odoo · Mar 3, 2023 · a829d0c · a829d0c
1 parent 0681d27
commit a829d0c
Show file tree

Hide file tree

Showing 14 changed files with 607 additions and 54 deletions.
diff --git a/src/components/figures/chart.ts b/src/components/figures/chart.ts
@@ -131,6 +131,7 @@ export class ChartFigure extends Component<Props, SpreadsheetEnv> {
       name: _lt("Edit"),
       sequence: 1,
       action: () => this.env.openSidePanel("ChartPanel", { figure: this.props.figure }),
+      isVisible: () => !!this.env.getters.getChartDefinition(this.props.figure.id),
     });
     registry.add("delete", {
       name: _lt("Delete"),

diff --git a/src/components/side_panel/chart_panel.ts b/src/components/side_panel/chart_panel.ts
@@ -7,6 +7,7 @@ import {
   DispatchResult,
   Figure,
   SpreadsheetEnv,
+  UID,
 } from "../../types/index";
 import { ColorPicker } from "../color_picker";
 import * as icons from "../icons";
@@ -168,6 +169,8 @@ export class ChartPanel extends Component<Props, SpreadsheetEnv> {
   static components = { SelectionInput, ColorPicker };
   private getters = this.env.getters;
 
+  private chartSheetId: UID = this.findSheetId(this.props.figure.id);
+
   private state: ChartPanelState = useState(this.initialState(this.props.figure));
 
   setup() {
@@ -177,14 +180,15 @@ export class ChartPanel extends Component<Props, SpreadsheetEnv> {
         return;
       }
       if (nextProps.figure.id !== this.props.figure.id) {
+        this.chartSheetId = this.findSheetId(nextProps.figure.id);
         this.state.panel = "configuration";
         this.state.fillColorTool = false;
         this.state.datasetDispatchResult = undefined;
         this.state.labelsDispatchResult = undefined;
         this.state.chart = this.env.getters.getChartDefinitionUI(
           this.env.getters.getActiveSheetId(),
           nextProps.figure.id
-        );
+        )!;
       }
     });
   }
@@ -243,7 +247,7 @@ export class ChartPanel extends Component<Props, SpreadsheetEnv> {
   private updateChart(definition: ChartUIDefinitionUpdate): DispatchResult {
     return this.env.dispatch("UPDATE_CHART", {
       id: this.props.figure.id,
-      sheetId: this.getters.getActiveSheetId(),
+      sheetId: this.chartSheetId,
       definition,
     });
   }
@@ -271,10 +275,14 @@ export class ChartPanel extends Component<Props, SpreadsheetEnv> {
   }
 
   private initialState(figure: Figure): ChartPanelState {
+    const sheetId = this.findSheetId(figure.id);
     return {
-      chart: this.env.getters.getChartDefinitionUI(this.env.getters.getActiveSheetId(), figure.id),
+      chart: this.env.getters.getChartDefinitionUI(sheetId, figure.id)!,
       panel: "configuration",
       fillColorTool: false,
     };
   }
+  private findSheetId(figureId: string): string {
+    return this.env.getters.getFigureSheetId(figureId) || "";
+  }
 }
diff --git a/src/migrations/data.ts b/src/migrations/data.ts
@@ -4,7 +4,7 @@ import {
   FORBIDDEN_IN_EXCEL_REGEX,
   FORMULA_REF_IDENTIFIER,
 } from "../constants";
-import { getItemId, toXC, toZone } from "../helpers/index";
+import { getItemId, toXC, toZone, UuidGenerator } from "../helpers/index";
 import { StateUpdateMessage } from "../types/collaborative/transport_service";
 import {
   CoreCommand,
@@ -43,17 +43,7 @@ export function load(data?: any): WorkbookData {
       data = migrate(data);
     }
   }
-
-  // sanity check: try to fix missing fields/corrupted state by providing
-  // sensible default values
-  data = Object.assign(createEmptyWorkbookData(), data, { version: CURRENT_VERSION });
-  data.sheets = data.sheets.map((s, i) =>
-    Object.assign(createEmptySheet(`Sheet${i + 1}`, `Sheet${i + 1}`), s)
-  );
-
-  if (data.sheets.length === 0) {
-    data.sheets.push(createEmptySheet(INITIAL_SHEET_ID, "Sheet1"));
-  }
+  data = repairData(data);
   return data;
 }
 
@@ -73,6 +63,7 @@ function migrate(data: any): WorkbookData {
   for (let i = index; i < MIGRATIONS.length; i++) {
     data = MIGRATIONS[i].applyMigration(data);
   }
+
   return data;
 }
 
@@ -294,6 +285,56 @@ const MIGRATIONS: Migration[] = [
   },
 ];
 
+/**
+ * This function is used to repair faulty data independently of the migration.
+ */
+export function repairData(data: Partial<WorkbookData>): Partial<WorkbookData> {
+  data = forceUnicityOfFigure(data);
+  data = setDefaults(data);
+  return data;
+}
+
+/**
+ * Force the unicity of figure ids accross sheets
+ */
+function forceUnicityOfFigure(data: Partial<WorkbookData>): Partial<WorkbookData> {
+  if (data.uniqueFigureIds) {
+    return data;
+  }
+  const figureIds = new Set();
+  const uuidGenerator = new UuidGenerator();
+  for (const sheet of data.sheets || []) {
+    for (const figure of sheet.figures || []) {
+      if (figureIds.has(figure.id)) {
+        figure.id += uuidGenerator.uuidv4();
+      }
+      figureIds.add(figure.id);
+    }
+  }
+
+  data.uniqueFigureIds = true;
+  return data;
+}
+
+/**
+ * sanity check: try to fix missing fields/corrupted state by providing
+ * sensible default values
+ */
+function setDefaults(data: Partial<WorkbookData>): Partial<WorkbookData> {
+  data = Object.assign(createEmptyWorkbookData(), data, { version: CURRENT_VERSION });
+  data.sheets = data.sheets
+    ? data.sheets.map((s, i) =>
+        Object.assign(createEmptySheet(`Sheet${i + 1}`, `Sheet${i + 1}`), s)
+      )
+    : [];
+
+  if (data.sheets.length === 0) {
+    data.sheets.push(createEmptySheet(INITIAL_SHEET_ID, "Sheet1"));
+  }
+
+  return data;
+}
+
 /**
  * The goal of this function is to repair corrupted/wrong initial messages caused by
  * a bug.
@@ -346,7 +387,7 @@ function fixTranslatedSheetIds(
   return messages;
 }
 
-function dropCommands(initialMessages, commandType: string) {
+function dropCommands(initialMessages: StateUpdateMessage[], commandType: string) {
   const messages: StateUpdateMessage[] = [];
   for (const message of initialMessages) {
     if (message.type === "REMOTE_REVISION") {
@@ -388,6 +429,7 @@ export function createEmptyWorkbookData(sheetName = "Sheet1"): WorkbookData {
     formats: {},
     borders: {},
     revisionId: DEFAULT_REVISION_ID,
+    uniqueFigureIds: true,
   };
   return data;
 }

diff --git a/src/plugins/core/chart.ts b/src/plugins/core/chart.ts
@@ -127,10 +127,19 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
     const success: CommandResult = CommandResult.Success;
     switch (cmd.type) {
       case "UPDATE_CHART":
+        return this.checkValidations(
+          cmd,
+          this.chainValidations(this.checkEmptyDataset, this.checkDataset, this.checkChartExists),
+          this.checkLabelRange
+        );
       case "CREATE_CHART":
         return this.checkValidations(
           cmd,
-          this.chainValidations(this.checkEmptyDataset, this.checkDataset),
+          this.chainValidations(
+            this.checkEmptyDataset,
+            this.checkDataset,
+            this.checkChartDuplicate
+          ),
           this.checkLabelRange
         );
       default:
@@ -222,8 +231,11 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
       .map((chart) => chart[0]);
   }
 
-  getChartDefinitionUI(sheetId: UID, figureId: UID): ChartUIDefinition {
+  getChartDefinitionUI(sheetId: UID, figureId: UID): ChartUIDefinition | undefined {
     const data: ChartDefinition = this.chartFigures[figureId];
+    if (!data) {
+      return undefined;
+    }
     const dataSets: string[] = data.dataSets
       .map((ds: DataSet) => (ds ? this.getters.getRangeString(ds.dataRange, sheetId) : ""))
       .filter((ds) => {
@@ -245,13 +257,17 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
     };
   }
 
-  private getChartDefinitionExcel(sheetId: UID, figureId: UID): ExcelChartDefinition {
+  private getChartDefinitionExcel(sheetId: UID, figureId: UID): ExcelChartDefinition | undefined {
     const data: ChartDefinition = this.chartFigures[figureId];
+    const dataUI = this.getChartDefinitionUI("forceSheetReference", figureId);
+    if (!data || !dataUI) {
+      return undefined;
+    }
     const dataSets: ExcelChartDataset[] = data.dataSets
       .map((ds: DataSet) => this.toExcelDataset(ds))
       .filter((ds) => ds.range !== ""); // && range !== INCORRECT_RANGE_STRING ? show incorrect #ref ?
     return {
-      ...this.getChartDefinitionUI("forceSheetReference", figureId),
+      ...dataUI,
       backgroundColor: data.background,
       dataSets,
     };
@@ -306,10 +322,17 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
     if (data.sheets) {
       for (let sheet of data.sheets) {
         const sheetFigures = this.getters.getFigures(sheet.id);
-        const figures = sheetFigures as FigureData<any>[];
-        for (let figure of figures) {
+        const figures: FigureData<any>[] = [];
+        for (let sheetFigure of sheetFigures) {
+          const figure = sheetFigure as FigureData<any>;
           if (figure && figure.tag === "chart") {
-            figure.data = this.getChartDefinitionUI(sheet.id, figure.id);
+            const data = this.getChartDefinitionUI(sheet.id, figure.id);
+            if (data) {
+              figure.data = data;
+              figures.push(figure);
+            }
+          } else {
+            figures.push(figure);
           }
         }
         sheet.figures = figures;
@@ -320,10 +343,17 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
   exportForExcel(data: ExcelWorkbookData) {
     for (let sheet of data.sheets) {
       const sheetFigures = this.getters.getFigures(sheet.id);
-      const figures = sheetFigures as FigureData<ExcelChartDefinition>[];
-      for (let figure of figures) {
+      const figures: FigureData<ExcelChartDefinition>[] = [];
+      for (let sheetFigure of sheetFigures) {
+        const figure = sheetFigure as FigureData<ExcelChartDefinition>;
         if (figure && figure.tag === "chart") {
-          figure.data = this.getChartDefinitionExcel(sheet.id, figure.id);
+          const data = this.getChartDefinitionExcel(sheet.id, figure.id);
+          if (data) {
+            figure.data = data;
+            figures.push(figure);
+          }
+        } else {
+          figures.push(figure);
         }
       }
       sheet.charts = figures;
@@ -504,4 +534,19 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
     const invalidLabels = !rangeReference.test(cmd.definition.labelRange || "");
     return invalidLabels ? CommandResult.InvalidLabelRange : CommandResult.Success;
   }
+
+  private checkChartDuplicate(cmd: CreateChartCommand): CommandResult {
+    for (const sheet of this.getters.getSheets()) {
+      if (this.getters.getFigure(sheet.id, cmd.id)) {
+        return CommandResult.DuplicatedChartId;
+      }
+    }
+    return CommandResult.Success;
+  }
+
+  private checkChartExists(cmd: UpdateChartCommand): CommandResult {
+    return this.getters.getFigure(cmd.sheetId, cmd.id)
+      ? CommandResult.Success
+      : CommandResult.ChartDoesNotExist;
+  }
 }
diff --git a/src/plugins/core/figures.ts b/src/plugins/core/figures.ts
@@ -14,7 +14,7 @@ interface FigureState {
 }
 
 export class FigurePlugin extends CorePlugin<FigureState> implements FigureState {
-  static getters = ["getFigures", "getFigure"] as const;
+  static getters = ["getFigures", "getFigure", "getFigureSheetId"] as const;
   readonly figures: {
     [sheet: string]: Record<UID, Figure | undefined> | undefined;
   } = {};
@@ -116,6 +116,12 @@ export class FigurePlugin extends CorePlugin<FigureState> implements FigureState
     return this.figures[sheetId]?.[figureId];
   }
 
+  getFigureSheetId(figureId: string): UID | undefined {
+    return Object.keys(this.figures).find(
+      (sheetId) => this.figures[sheetId]?.[figureId] !== undefined
+    );
+  }
+
   // ---------------------------------------------------------------------------
   // Import/Export
   // ---------------------------------------------------------------------------

diff --git a/src/types/commands.ts b/src/types/commands.ts
@@ -1031,6 +1031,8 @@ export const enum CommandResult {
   InvalidViewportSize,
   FigureDoesNotExist,
   DuplicatedFigureId,
+  DuplicatedChartId,
+  ChartDoesNotExist,
 }
 
 export interface CommandHandler<T> {

diff --git a/src/types/workbook_data.ts b/src/types/workbook_data.ts
@@ -52,6 +52,7 @@ export interface WorkbookData {
   borders: { [key: number]: Border };
   entities: { [key: string]: { [key: string]: any } };
   revisionId: UID;
+  uniqueFigureIds: boolean;
 }
 
 export interface ExcelWorkbookData extends WorkbookData {