[FIX] Filters: fix filter id on sheet duplication #2631
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The internal state of the filter plugin requires that the id in the table mapping matches the id of the related FilterTable object. Task: 3384840
rrahir
force-pushed
the
16.0-fix-filter-table-duplication-rar
branch
from
cbc4bfc to
271a583
Compare
LucasLefevre
approved these changes
Jun 26, 2023
| const tables: Record<FilterTableId, FilterTable | undefined> = {}; | ||
| for (const filterTable of Object.values(this.tables[cmd.sheetId] || {})) { | ||
| if (filterTable) { | ||
| const newFilterTable = deepCopy(filterTable); |
There was a problem hiding this comment.
Ah putain, deepCopy doesn't deep copy... 🤦♂️
|
robodoo r- |
|
actually, since it generates a uuid inside a core plugin, collaborative users are not synchronized. |
|
Hmm looks like the id is never shared cross-user, nor exported. |
|
robodoo r+ |
robodoo
pushed a commit
that referenced
this pull request
Jun 26, 2023
The internal state of the filter plugin requires that the id in the table mapping matches the id of the related FilterTable object. closes #2631 Task: 3384840 Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
14 tasks
This was referenced Jun 26, 2023
14 tasks
rrahir
added a commit
that referenced
this pull request
Feb 7, 2024
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/ Task: 3728009
robodoo
pushed a commit
that referenced
this pull request
Feb 7, 2024
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]>
fw-bot
pushed a commit
that referenced
this pull request
Feb 7, 2024
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/ Task: 3728009 X-original-commit: be0fda3
14 tasks
fw-bot
pushed a commit
that referenced
this pull request
Feb 7, 2024
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/ Task: 3728009 X-original-commit: be0fda3
14 tasks
14 tasks
rrahir
added a commit
that referenced
this pull request
Feb 7, 2024
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/ Task: 3728009 X-original-commit: be0fda3
robodoo
pushed a commit
that referenced
this pull request
Feb 7, 2024
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 #3631 Task: 3728009 X-original-commit: be0fda3 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
robodoo
pushed a commit
that referenced
this pull request
Feb 7, 2024
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 #3630 Task: 3728009 X-original-commit: be0fda3 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
robodoo
pushed a commit
that referenced
this pull request
Feb 7, 2024
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 #3632 Task: 3728009 X-original-commit: be0fda3 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
14 tasks
rrahir
added a commit
that referenced
this pull request
Feb 7, 2024
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/ Task: 3728009 X-original-commit: 5a5c475
robodoo
pushed a commit
that referenced
this pull request
Feb 7, 2024
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 #3634 Task: 3728009 X-original-commit: 5a5c475 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
14 tasks
rrahir
added a commit
that referenced
this pull request
Feb 8, 2024
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/ Task: 3728009 X-original-commit: 0a2a454
robodoo
pushed a commit
that referenced
this pull request
Feb 8, 2024
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 #3636 Task: 3728009 X-original-commit: 0a2a454 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
fw-bot
pushed a commit
that referenced
this pull request
Feb 8, 2024
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/ Task: 3728009 X-original-commit: 8f13884
14 tasks
fw-bot
pushed a commit
that referenced
this pull request
Feb 8, 2024
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/ Task: 3728009 X-original-commit: 8f13884
14 tasks
robodoo
pushed a commit
that referenced
this pull request
Feb 8, 2024
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 #3638 Task: 3728009 X-original-commit: 8f13884 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
robodoo
pushed a commit
that referenced
this pull request
Feb 8, 2024
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 #3637 Task: 3728009 X-original-commit: 8f13884 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The internal state of the filter plugin requires that the id in the table mapping matches the id of the related FilterTable object.
Task: 3384840
Description:
description of this task, what is implemented and why it is implemented that way.
Odoo task ID : 3384840
review checklist