Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FW][FIX] DataFilter: Fix overlapping filters #3634

Closed

Conversation

fw-bot
Copy link
Collaborator

@fw-bot fw-bot commented 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.

// 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.

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

Description:

description of this task, what is implemented and why it is implemented that way.

Task: : TASK_ID

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

Forward-Port-Of: #3632
Forward-Port-Of: #3629

@robodoo
Copy link
Collaborator

robodoo commented Feb 7, 2024

@fw-bot
Copy link
Collaborator Author

fw-bot commented Feb 7, 2024

@rrahir @LucasLefevre cherrypicking of pull request #3629 failed.

stdout:

Auto-merging src/migrations/data.ts
CONFLICT (content): Merge conflict in src/migrations/data.ts
Auto-merging tests/plugins/import_export.test.ts
CONFLICT (content): Merge conflict in tests/plugins/import_export.test.ts

stderr:

17:56:24.912604 git.c:463               trace: built-in: git cherry-pick cdae531c8d798246f079c40f47efa311119ce131
error: could not apply cdae531c8... [FIX] DataFilter: Fix overlapping filters
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
----------
status:

Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).

In the former case, you may want to edit this PR message as well.

More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port

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
@rrahir rrahir force-pushed the saas-16.4-16.0-fix-datafilter-dup-def-rar-LaEH-fw branch from 7d4b620 to 0c5158a Compare February 7, 2024 17:28
@rrahir
Copy link
Collaborator

rrahir commented Feb 7, 2024

@robodoo r+

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]>
@robodoo robodoo closed this Feb 7, 2024
@fw-bot fw-bot deleted the saas-16.4-16.0-fix-datafilter-dup-def-rar-LaEH-fw branch February 21, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants