From 8853ab277a58b85ca6595527d21902ca02a05510 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= Date: Tue, 6 Feb 2024 09:59:02 +0000 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#3637 Task: 3728009 X-original-commit: 8f138849b19bef77528b1c81a014081a9568db46 Signed-off-by: Lucas Lefèvre (lul) Signed-off-by: Rémi Rahir (rar) --- src/migrations/data.ts | 30 +++++++++++++++++++- tests/components/filter_icon_overlay.test.ts | 19 +++++++++++++ tests/model/model_import_export.test.ts | 16 ++++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 tests/components/filter_icon_overlay.test.ts diff --git a/src/migrations/data.ts b/src/migrations/data.ts index fd099f3b1d..a82eb46257 100644 --- 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 { UuidGenerator, getItemId, toXC, toZone } from "../helpers/index"; +import { UuidGenerator, getItemId, overlap, toXC, toZone, zoneToXc } from "../helpers/index"; import { isValidLocale } from "../helpers/locale"; import { StateUpdateMessage } from "../types/collaborative/transport_service"; import { @@ -16,6 +16,7 @@ import { SheetData, UID, WorkbookData, + Zone, } from "../types/index"; import { XlsxReader } from "../xlsx/xlsx_reader"; import { normalizeV9 } from "./legacy_tools"; @@ -306,6 +307,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; + }, + }, { description: "Change Border description structure", from: 12, 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/model/model_import_export.test.ts b/tests/model/model_import_export.test.ts index c3baeb843d..8a8e78b153 100644 --- a/tests/model/model_import_export.test.ts +++ b/tests/model/model_import_export.test.ts @@ -30,7 +30,7 @@ describe("data", () => { }); describe("Migrations", () => { - test("Can upgrade from 1 to 12", () => { + test("Can upgrade from 1 to 13", () => { const model = new Model({ version: 1, sheets: [ @@ -402,6 +402,20 @@ describe("Migrations", () => { }); }); + 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" }]); + }); + test("migrate version 14: set locale of spreadsheet to en_US", () => { let model = new Model({ version: 13 }); let data = model.exportData();