Skip to content

Conversation

hokolomopo
Copy link
Contributor

Description

If we have a range with an invalid sheet name, the sheet name is not escaped with quotes when it contains special characters when converting the range back to a string.

Task: 5125762

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

If we have a range with an invalid sheet name, the sheet name is not
escaped with quotes when it contains special characters when converting
the range back to a string.

Task: 5125762
@robodoo
Copy link
Collaborator

robodoo commented Oct 15, 2025

Pull request status dashboard

const cfs = data.sheets[1].conditionalFormats;
const rule1 = cfs[0].rule as ColorScaleRule;
expect(cfs[0].ranges).toEqual(["=sheetName_!A1:A2"]);
expect(cfs[0].ranges).toEqual(["'=sheetName_'!A1:A2"]);
Copy link
Contributor Author

@hokolomopo hokolomopo Oct 15, 2025

Choose a reason for hiding this comment

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

This whole test is both totally broken, and it highlight some problematic behaviors:

Broken because:

  • in the spreadsheet data, it uses figure.type instead of figure.tag. So Charts are never actually created.
  • it creates a sheet with invalid characters in its name such as sheetName?, but the ranges of the charts/cfs are =sheetName!, an invalid sheet name that does not exist (notice the =, those are ranges and not formulas)

Strange behavior highlighted

  • cfs formulas are stored as string in the plugin, and exported as it in the data. So they never go though the createRange/toRangeString moulinette. So sheet name with wrong character are never escaped.

Not sure if we want/need to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rrahir didn't we talk about this test sometime ? I kinda remember something like that

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do recall that as well, I think we agreed to fix it in later versions (specifically when it came to introduce the carousel but I might be wrong) I don't recall what we put in place though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants