From be0fda3780f326ebc5a275396f0b48384a1e3c0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= Date: Tue, 6 Feb 2024 10:59:02 +0100 Subject: [PATCH] [FIX] DataFilter: Fix overlapping filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 odoo/o-spreadsheet#3629 Task: 3728009 Signed-off-by: Lucas Lefèvre (lul) --- src/migrations/data.ts | 40 +++++++++++++++++++- tests/__snapshots__/model.test.ts.snap | 2 +- tests/components/filter_icon_overlay.test.ts | 19 ++++++++++ tests/plugins/import_export.test.ts | 20 ++++++++-- 4 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 tests/components/filter_icon_overlay.test.ts diff --git a/src/migrations/data.ts b/src/migrations/data.ts index 29edde107d..a44a62e500 100644 --- a/src/migrations/data.ts +++ b/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; + }, + }, ]; /** diff --git a/tests/__snapshots__/model.test.ts.snap b/tests/__snapshots__/model.test.ts.snap index 7c7a8e674b..1dfab50209 100644 --- a/tests/__snapshots__/model.test.ts.snap +++ b/tests/__snapshots__/model.test.ts.snap @@ -36,6 +36,6 @@ Object { ], "styles": Object {}, "uniqueFigureIds": true, - "version": 12, + "version": 12.5, } `; diff --git a/tests/components/filter_icon_overlay.test.ts b/tests/components/filter_icon_overlay.test.ts new file mode 100644 index 0000000000..6cb0ec070c --- /dev/null +++ b/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); + }); +}); diff --git a/tests/plugins/import_export.test.ts b/tests/plugins/import_export.test.ts index 8f63954cf5..f2f032c617 100644 --- a/tests/plugins/import_export.test.ts +++ b/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", () => {