[FIX] DataFilter: Fix overlapping filters

The fix proposed in PR #2631 was incomplete. While it prevented the
*new* duplication of filter tables on overlapping zones (due to a mismatch
inthe plugin internal mapping) it didn't fix the filters already
duplicated and overlapping.

This revision adds a new migration to clear the duplicated filters based
on the knowledge of how the bug could occur.

Before #2631, when duplicating a sheet containing a filter, the filter
plugin would:
- Deep copy the entry of `this.tables[cmd.sheetIdFrom]` - an Object
  mapping the id of a `FilterTable` with the corresponding
  `FilterTable` object
- create a new entry for `cmd.sheetIdTo` in `this.tables` with the deep
  copy

However, deep copy would create a new `FilterTable` object with a new
generated id (type uuid) which would mismatch with the internal mapping
of the copied object.

e.g.
```typescript
// in sheet1
table1 = {id1: filter1 instanceof FilterTable}
id1 === filter1.id

// in sheet2
table2 = deepCopy(table1)
table2 ===  {id1: filter2 instanceof FilterTable}
id1 !== filter2.id
```

When trying to then alter the copied sheet structure and such
modification altered the filters, the implementation did not account for
such situation and would end up duplicating the filter table in `sheet2`,
thus creating a sheet with filters with overlapping zones.

E.g.

``` typescript
tables["sheet2"] = {id1: filter2}
filter2.zone === "A1:A10"

deleteRow(5)

tables["sheet2"] = {id1: filter2, id3:filter3}
id1 !== filter2.id
id3 === filter3.id

// overlap in filte zones
filter2.zone === "A1:A10"
filter3.zone === "A1:A9"
```

Since Javascript guarantee `Object` property insertion order upon
iteration (for equivalent objects [1]), we can safely say that in such
situation the latest key of the object `tables["sheet2"]` is the truth
and we can delete the other keyslinked to filters with overlapping
zones.

One of the side-effects of these overlapping tables was a crash in the
owl component `FilterIconsOverlay` due to some duplicated keys. As such,
a regression test was added to cover this case.

[1] https://262.ecma-international.org/10.0/

closes #3629

Task: 3728009
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>

src/migrations/data.ts

@@ -4,7 +4,15 @@ import {
   FORBIDDEN_IN_EXCEL_REGEX,
   FORMULA_REF_IDENTIFIER,
 } from "../constants";
-import { deepCopy, getItemId, toXC, toZone, UuidGenerator } from "../helpers/index";
+import {
+  deepCopy,
+  getItemId,
+  overlap,
+  toXC,
+  toZone,
+  UuidGenerator,
+  zoneToXc,
+} from "../helpers/index";
 import { StateUpdateMessage } from "../types/collaborative/transport_service";
 import {
   CoreCommand,
@@ -14,6 +22,7 @@ import {
   SheetData,
   UID,
   WorkbookData,
+  Zone,
 } from "../types/index";
 import { XlsxReader } from "../xlsx/xlsx_reader";
 import { normalizeV9 } from "./legacy_tools";
@@ -23,7 +32,7 @@ import { normalizeV9 } from "./legacy_tools";
  * a breaking change is made in the way the state is handled, and an upgrade
  * function should be defined
  */
-export const CURRENT_VERSION = 12;
+export const CURRENT_VERSION = 12.5;
 const INITIAL_SHEET_ID = "Sheet1";
 
 /**
@@ -305,6 +314,33 @@ const MIGRATIONS: Migration[] = [
       return data;
     },
   },
+  {
+    description: "Fix datafilter duplication",
+    from: 12,
+    to: 12.5,
+    applyMigration(data: any): any {
+      for (let sheet of data.sheets || []) {
+        let knownDataFilterZones: Zone[] = [];
+        for (let filterTable of sheet.filterTables || []) {
+          const zone = toZone(filterTable.range);
+          // See commit message for the details
+          const intersectZoneIndex = knownDataFilterZones.findIndex((knownZone) =>
+            overlap(knownZone, zone)
+          );
+          if (intersectZoneIndex !== -1) {
+            knownDataFilterZones[intersectZoneIndex] = zone;
+          } else {
+            knownDataFilterZones.push(zone);
+          }
+        }
+
+        sheet.filterTables = knownDataFilterZones.map((zone) => ({
+          range: zoneToXc(zone),
+        }));
+      }
+      return data;
+    },
+  },
 ];
 
 /**

tests/__snapshots__/model.test.ts.snap

@@ -36,6 +36,6 @@ Object {
   ],
   "styles": Object {},
   "uniqueFigureIds": true,
-  "version": 12,
+  "version": 12.5,
 }
 `;

tests/components/filter_icon_overlay.test.ts

@@ -0,0 +1,19 @@
+import { Model } from "../../src";
+import { mountSpreadsheet } from "../test_helpers/helpers";
+
+describe("Filter Icon Overlay component", () => {
+  test("Overlapping filters are overwritten bythe latest inserted", async () => {
+    const model = new Model({
+      version: 12,
+      sheets: [
+        {
+          name: "Sheet1",
+          id: "sh1",
+          filterTables: [{ range: "A2:B3" }, { range: "A2:C4" }],
+        },
+      ],
+    });
+    const { fixture } = await mountSpreadsheet({ model });
+    expect(fixture.querySelectorAll(".o-filter-icon").length).toBe(3);
+  });
+});

tests/plugins/import_export.test.ts

@@ -33,7 +33,7 @@ describe("data", () => {
 });
 
 describe("Migrations", () => {
-  test("Can upgrade from 1 to 12", () => {
+  test("Can upgrade from 1 to 12.5", () => {
     const model = new Model({
       version: 1,
       sheets: [
@@ -53,7 +53,7 @@ describe("Migrations", () => {
       ],
     });
     const data = model.exportData();
-    expect(data.version).toBe(12);
+    expect(data.version).toBe(12.5);
     expect(data.sheets[0].id).toBeDefined();
     expect(data.sheets[0].figures).toBeDefined();
     expect(data.sheets[0].cells.A1!.content).toBe("=A1");
@@ -76,7 +76,7 @@ describe("Migrations", () => {
     });
     const data = model.exportData();
     const cells = data.sheets[0].cells;
-    expect(data.version).toBe(12);
+    expect(data.version).toBe(12.5);
     // formulas are de-normalized with version 9
     expect(cells.A1?.content).toBe("=A1");
     expect(cells.A2?.content).toBe("=1");
@@ -397,6 +397,20 @@ describe("Migrations", () => {
     expect(data.sheets[1].cells["A1"]?.format).toEqual(1);
     expect(data.sheets[1].cells["A2"]?.format).toEqual(2);
   });
+
+  test("migrate version 12.5: Fix Overlapping datafilters", () => {
+    const model = new Model({
+      version: 12,
+      sheets: [
+        {
+          id: "1",
+          filterTables: [{ range: "A1:B2" }, { range: "A1:C2" }],
+        },
+      ],
+    });
+    const data = model.exportData();
+    expect(data.sheets[0].filterTables).toEqual([{ range: "A1:C2" }]);
+  });
 });
 
 describe("Import", () => {