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] Selection: exclude hidden rows/cols in selection statistics #3010

Conversation

fw-bot
Copy link
Collaborator

@fw-bot fw-bot commented Oct 16, 2023

Description:

Previously, the Selection Statistics displayed calculations for all rows and columns, even when some were hidden.

This commit addresses the issue by considering only the cells that are not hidden in the statistics calculation.

Task: : 3508872

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 (_lt("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: #2966

@robodoo
Copy link
Collaborator

robodoo commented Oct 16, 2023

@fw-bot
Copy link
Collaborator Author

fw-bot commented Oct 16, 2023

@dhrp-odoo @LucasLefevre cherrypicking of pull request #2966 failed.

stdout:

Auto-merging src/plugins/ui_stateful/selection.ts
CONFLICT (content): Merge conflict in src/plugins/ui_stateful/selection.ts
Auto-merging tests/plugins/core.test.ts

stderr:

09:21:51.366786 git.c:463               trace: built-in: git cherry-pick 7d1eb564903cc4f27591149c626acb2949a67c9b
error: could not apply 7d1eb5649... [FIX] Selection: exclude hidden rows/cols in selection statistics
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

@dhrp-odoo dhrp-odoo force-pushed the saas-16.1-16.0-fix-hidden-rows-in-statistic-fn-dhrp-D791-fw branch from 02cfba1 to 88f3dd5 Compare October 16, 2023 13:07
@dhrp-odoo
Copy link
Contributor

Hi @LucasLefevre,

I have resolved the merged conflicts and made some changes to the code.

The reason for these changes is that in version 16.1, we returned an evaluated cell by mapping over zones, which doesn't have any info such as an ID or any other identifier to determine the column and row for filtering hidden rows.

Can you review these changes once again?

Thanks :)

src/plugins/ui_stateful/selection.ts Outdated Show resolved Hide resolved
src/plugins/ui_stateful/selection.ts Outdated Show resolved Hide resolved
@dhrp-odoo dhrp-odoo force-pushed the saas-16.1-16.0-fix-hidden-rows-in-statistic-fn-dhrp-D791-fw branch from 88f3dd5 to ead0f56 Compare October 17, 2023 07:03
Comment on lines 429 to 430
for (let col = zone.left; col <= zone.right; col++) {
for (let row = zone.top; row <= zone.bottom; row++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestion to avoid one nested level and with a more declarative approach.
LGTM otherwise :)

Suggested change
for (let col = zone.left; col <= zone.right; col++) {
for (let row = zone.top; row <= zone.bottom; row++) {
for (const { col, row } of positions(zone)) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LucasLefevre, I have implemented the suggested changes. Thanks :)

Previously, the Selection Statistics displayed calculations for all
rows and columns, even when some were hidden.

This commit addresses the issue by considering only the cells that
are not hidden in the statistics calculation.

Task: 3508872
X-original-commit: 56d9f38
@dhrp-odoo dhrp-odoo force-pushed the saas-16.1-16.0-fix-hidden-rows-in-statistic-fn-dhrp-D791-fw branch from ead0f56 to 4a34868 Compare October 20, 2023 11:09
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

robodoo r+
Thanks !

robodoo pushed a commit that referenced this pull request Oct 20, 2023
Previously, the Selection Statistics displayed calculations for all
rows and columns, even when some were hidden.

This commit addresses the issue by considering only the cells that
are not hidden in the statistics calculation.

closes #3010

Task: 3508872
X-original-commit: 56d9f38
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
@robodoo robodoo temporarily deployed to merge October 20, 2023 11:41 Inactive
@robodoo robodoo closed this Oct 20, 2023
@fw-bot fw-bot deleted the saas-16.1-16.0-fix-hidden-rows-in-statistic-fn-dhrp-D791-fw branch November 3, 2023 11: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.

None yet

4 participants