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] evaluation: behavior for invalid range arguments #3492

Closed

Conversation

fw-bot
Copy link
Collaborator

@fw-bot fw-bot commented Jan 23, 2024

Description:

When user provides an invalid sheet name in range arguments of a function, it performs computations on
data of specified range from active sheet by default and returns misleading results.

This PR changes the behavior to throw an error instead of performing any kind of computations.

Task: : 3619144

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: #3361

@robodoo
Copy link
Collaborator

robodoo commented Jan 23, 2024

@fw-bot
Copy link
Collaborator Author

fw-bot commented Jan 23, 2024

@khpa-odoo @pro-odoo cherrypicking of pull request #3361 failed.

stdout:

Auto-merging src/plugins/ui/evaluation.ts
CONFLICT (content): Merge conflict in src/plugins/ui/evaluation.ts
Auto-merging tests/formulas/compiler.test.ts

stderr:

09:49:50.507251 git.c:463               trace: built-in: git cherry-pick 1eb59b3bb0b482585ab2b638e6a1c3b931173ee0
error: could not apply 1eb59b3bb... [FIX] evaluation: behavior for invalid range arguments
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

@khpa-odoo khpa-odoo force-pushed the saas-15.2-15.0-fix-invalid-range-args-khpa-UVEK-fw branch from 307bb8a to 5aa4a8e Compare January 23, 2024 10:25
@khpa-odoo
Copy link
Contributor

@pro-odoo Resolved the conflict 👍

Comment on lines 193 to 194
if (getters.getSheet(range.sheetId)) {
cell = getters.getCell(range.sheetId, range.zone.left, range.zone.top);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because we used the getter getSheet in the fix for 15.0 and saas-15.1. It checks for the same condition and throws the error as well. So I made this change for both consistency and optimization. However, I can undo it if you want me to : )
@pro-odoo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you can undo this change. It's easier to know why we try to get the sheet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! @pro-odoo

When user provides an invalid sheet name in range arguments of a function,
it performs computations on data of specified range from active sheet by default
and returns misleading results.

This commit changes the behavior to throw an error instead of performing any
kind of computations.

Task ID : 3619144

X-original-commit: 5eedbd0
@khpa-odoo khpa-odoo force-pushed the saas-15.2-15.0-fix-invalid-range-args-khpa-UVEK-fw branch from 5aa4a8e to 2a2c07f Compare January 25, 2024 09:23
@pro-odoo
Copy link
Collaborator

robodoo r+

robodoo pushed a commit that referenced this pull request Jan 25, 2024
When user provides an invalid sheet name in range arguments of a function,
it performs computations on data of specified range from active sheet by default
and returns misleading results.

This commit changes the behavior to throw an error instead of performing any
kind of computations.

Task ID : 3619144

closes #3492

X-original-commit: 5eedbd0
Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>
@robodoo robodoo closed this Jan 25, 2024
@fw-bot fw-bot deleted the saas-15.2-15.0-fix-invalid-range-args-khpa-UVEK-fw branch February 8, 2024 10:46
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