[FIX] Migration: Fix chart migration after 18.5.1

The current introduction of the carousels came required a change of
data structure regarding the charts, namely, split their id from the one
of their container.

Some migration tools (repairInitialMessages) were adapted accordingly
but there were some mmistakes in the adaptation:
- the condition set on the versions was inverted
- the test was adapted but only acounted for the versions that came
  after the refactoring, hiding the previous issue
- the handler  `fixChartDefinitions` did not account for chart
  definitions existing in the carousels

The handler `fixChartDefinitions` was added to level partial definitions
in old chart related commands that were issued around version 16.0
This means that if a user were to create a model with data that come from
version 18.5.1, the data necessarily have been upgraded (snapshotted)
and it is therefore guaranteed that the changes of those commands have
been incorporated in the loaded data, which means the
`fixChartDefinition` no longer makes sense.

This revision fixes the aforementioned issues by skipping `fixChartDefinitions`
for data above 18.5.1. The tests have also been adapted to ensure
previous versions are still properly adapted.

closes #7550

Task: 5365954
Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>

Author: rrahir

src/migrations/data.ts

@@ -278,17 +278,23 @@ function dropCommands(initialMessages: StateUpdateMessage[], commandType: string
 }
 
 function fixChartDefinitions(data: Partial<WorkbookData>, initialMessages: StateUpdateMessage[]) {
+  /**
+   * Revisions created after version 18.5.1 contain the full chart definition in the command
+   * if the data was alreay updated to 18.5.1, then those older revision cannot (by definition) be reaplied
+   * and should not be replayed.
+   * FIXME: every command should be versionned when upgraded to allow finer tuning.
+   */
+  if (!data.version || compareVersions(String(data.version), "18.5.1") >= 0) {
+    return initialMessages;
+  }
   const messages: StateUpdateMessage[] = [];
   const map = {};
   for (const sheet of data.sheets || []) {
     sheet.figures?.forEach((figure) => {
       if (figure.tag === "chart") {
         // chart definition
-        if (data.version && compareVersions(String(data.version), "18.5.1") <= 0) {
-          map[figure.data.chartId] = figure.data;
-        } else {
-          map[figure.id] = figure.data;
-        }
+
+        map[figure.id] = figure.data;
       }
     });
   }

tests/collaborative/collaborative_history.test.ts

@@ -2,7 +2,6 @@ import { Model } from "../../src";
 import { DEFAULT_REVISION_ID, MESSAGE_VERSION } from "../../src/constants";
 import { toZone } from "../../src/helpers";
 import { CommandResult, UpdateCellCommand } from "../../src/types";
-import { LineChartDefinition } from "../../src/types/chart/line_chart";
 import { StateUpdateMessage } from "../../src/types/collaborative/transport_service";
 import { MockTransportService } from "../__mocks__/transport_service";
 import {
@@ -425,92 +424,6 @@ describe("Collaborative local history", () => {
     expect(getCell(model, "A1")?.format).toBeUndefined();
   });
 
-  test("Update chart revisions contain the full definition", () => {
-    const initialMessages: StateUpdateMessage[] = [
-      {
-        type: "REMOTE_REVISION",
-        version: MESSAGE_VERSION,
-        nextRevisionId: "1",
-        clientId: "bob",
-        commands: [
-          {
-            type: "UPDATE_CHART",
-            figureId: "fig1",
-            chartId: "chart1",
-            //@ts-ignore the old command would handle a partial definition
-            definition: { dataSets: [{ dataRange: "A1:A3" }] },
-          },
-          {
-            type: "CREATE_CHART",
-            sheetId: "sheet1",
-            figureId: "fig2",
-            chartId: "chart2",
-            col: 0,
-            row: 0,
-            offset: {
-              x: 0,
-              y: 0,
-            },
-            size: {
-              width: 100,
-              height: 100,
-            },
-            definition: {
-              title: { text: "" },
-              dataSets: [{ dataRange: "A1", yAxisId: "y" }],
-              type: "bar",
-              stacked: false,
-              dataSetsHaveTitle: false,
-              legendPosition: "none",
-            },
-          },
-          {
-            type: "UPDATE_CHART",
-            figureId: "fig2",
-            chartId: "chart2",
-            //@ts-ignore the old command would handle a partial definition
-            definition: { dataSets: [{ dataRange: "B1:B3" }] },
-          },
-        ],
-        serverRevisionId: "initial_revision",
-      },
-    ];
-    const data = {
-      revisionId: "initial_revision",
-      version: "18.5.1",
-      sheets: [
-        {
-          id: "sheet1",
-          figures: [
-            {
-              id: "fig1",
-              tag: "chart",
-              width: 400,
-              height: 300,
-              x: 100,
-              y: 100,
-              data: {
-                chartId: "chart1",
-                type: "line",
-                dataSetsHaveTitle: false,
-                dataSets: [{ dataRange: "Sheet1!B26:B35" }, { dataRange: "Sheet1!C26:C35" }],
-                legendPosition: "top",
-                title: "Line",
-                stacked: false,
-                cumulative: false,
-              },
-            },
-          ],
-        },
-      ],
-    };
-    const model = new Model(data, {}, initialMessages);
-    const definition1 = model.getters.getChartDefinition("chart1") as LineChartDefinition;
-    expect(definition1.dataSets).toEqual([{ dataRange: "A1:A3" }]);
-    const definition2 = model.getters.getChartDefinition("chart2") as LineChartDefinition;
-    expect(definition2.dataSets).toEqual([{ dataRange: "B1:B3" }]);
-  });
-
   test("Undo/redo your own change only", () => {
     setCellContent(alice, "A1", "hello in A1");
     setCellContent(bob, "B2", "hello in B2");

tests/model/model_import_export.test.ts

@@ -5,6 +5,7 @@ import {
   DEFAULT_CELL_WIDTH,
   DEFAULT_REVISION_ID,
   FORBIDDEN_SHEETNAME_CHARS,
+  MESSAGE_VERSION,
 } from "../../src/constants";
 import { toCartesian, toZone } from "../../src/helpers";
 import { DEFAULT_TABLE_CONFIG } from "../../src/helpers/table_presets";
@@ -16,6 +17,8 @@ import {
   DEFAULT_LOCALES,
   IconSetRule,
 } from "../../src/types";
+import { LineChartDefinition } from "../../src/types/chart";
+import { StateUpdateMessage } from "../../src/types/collaborative/transport_service";
 import {
   activateSheet,
   resizeColumns,
@@ -1130,3 +1133,88 @@ test("Can import spreadsheet with only version", () => {
   // We expect the model to be loaded without traceback
   expect(true).toBeTruthy();
 });
+
+test("Update chart revisions contain the full definition pre 18.5.1", () => {
+  const initialMessages: StateUpdateMessage[] = [
+    {
+      type: "REMOTE_REVISION",
+      version: MESSAGE_VERSION,
+      nextRevisionId: "1",
+      clientId: "bob",
+      commands: [
+        {
+          type: "UPDATE_CHART",
+          figureId: "fig1",
+          chartId: "fig1",
+          //@ts-ignore the old command would handle a partial definition
+          definition: { dataSets: [{ dataRange: "A1:A3" }] },
+        },
+        {
+          type: "CREATE_CHART",
+          sheetId: "sheet1",
+          figureId: "fig2",
+          chartId: "fig2",
+          col: 0,
+          row: 0,
+          offset: {
+            x: 0,
+            y: 0,
+          },
+          size: {
+            width: 100,
+            height: 100,
+          },
+          definition: {
+            title: { text: "" },
+            dataSets: [{ dataRange: "A1", yAxisId: "y" }],
+            type: "bar",
+            stacked: false,
+            dataSetsHaveTitle: false,
+            legendPosition: "none",
+          },
+        },
+        {
+          type: "UPDATE_CHART",
+          figureId: "fig2",
+          chartId: "fig2",
+          //@ts-ignore the old command would handle a partial definition
+          definition: { dataSets: [{ dataRange: "B1:B3" }] },
+        },
+      ],
+      serverRevisionId: "initial_revision",
+    },
+  ];
+  const data = {
+    revisionId: "initial_revision",
+    version: "18.4.3",
+    sheets: [
+      {
+        id: "sheet1",
+        figures: [
+          {
+            id: "fig1",
+            tag: "chart",
+            width: 400,
+            height: 300,
+            x: 100,
+            y: 100,
+            data: {
+              type: "line",
+              dataSetsHaveTitle: false,
+              dataSets: [{ dataRange: "Sheet1!B26:B35" }, { dataRange: "Sheet1!C26:C35" }],
+              legendPosition: "top",
+              title: "Line",
+              stacked: false,
+              cumulative: false,
+            },
+          },
+        ],
+      },
+    ],
+  };
+  const model = new Model(data, {}, initialMessages);
+  const definition1 = model.getters.getChartDefinition("fig1") as LineChartDefinition;
+  expect(definition1.dataSets).toEqual([{ dataRange: "A1:A3" }]);
+  const definition2 = model.getters.getChartDefinition("fig2") as LineChartDefinition;
+  expect(definition2.dataSets).toEqual([{ dataRange: "B1:B3" }]);
+});