[FIX] localize and canonicalize content

[matho-odoo]

Task: 6089166

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

[robodoo]

[hokolomopo]

Commit format is wrong, otherwise LGTM :)

[LucasLefevre]

why remove it from here and copy the exact same code in the children ?

[matho-odoo]

One children use "canonicalizeContent" and the other "canonicalizeNumberContent"

[LucasLefevre]

it's very weird and it's impossible to understand and know why by reading the code.

I found out it's because of dates which are not canonical on UPDATE_CELL (and then core detects the format).
If we want to be consistent and to always use canonical content on the wire, shouldn't we change that ?

At the very least, it deserves a comment.

canonicalizeNumberContent and canonicalizeContent should not both exist, only one them. It's very confusing, and you don't know which one should be used. But that's a refactoring for master.

[anhe-odoo]

Maybe add a comment to explain why you used two different canonicalize method (here and in standalone_composer_store)

[LucasLefevre]

composer.setCurrentContent is supposed to take localized content, since it later takes this._currentContent and run canonicalizeNumberContent on it

[rrahir]

Think it's what it does ^^

[matho-odoo]

I change it to localizedValue, Lucas is right :)
(and on top of that, value was already canonical)

[rrahir]

I read the sentence backwards, I think ><

[LucasLefevre]

testing Claude review skill:

Commits

7c6ed6dd7 removes .map(canonicalizeContent…) but leaves the preceding .filter(...) without a trailing ; (relying on ASI); ef2dd7a63 adds it back. Cosmetic — not a blocker, but ideally squashed.

ef2dd7a63 bundles unrelated pivot_composer_helpers.ts / auto_complete_registry.ts typing changes (adding getters to the this type) alongside CF localization work. Defensible since it's required by data_validation_auto_complete.ts in the same commit.

Findings

src/registries/data_validation_registry.ts:747 — nit

getNumberCriterionlocalizedValues has a casing typo (localizedValues → LocalizedValues) and the "Number" name is misleading — it's also used for isValueInList, which stores dates/text too. Rename to getCriterionLocalizedValues.

src/registries/data_validation_registry.ts:147 — nit

getPreview: (criterion: TextContainsCriterion, …) is used for the textIs entry; the inner field type should be TextIsCriterion. Same shape, but the wrong-name annotation is confusing.

src/registries/data_validation_registry.ts:109,131,153 — nit

criterion.values[0]?.toString() — values is string[], so both ?. and .toString() are redundant. Just use criterion.values[0].

src/registries/data_validation_registry.ts:685,696 — warning

isValueInList preview/error now uses locale.formulaArgSeparator as separator (e.g. ; in FR). Display only, but it's a UX change from the old hardcoded ", " — worth a one-line comment or confirming it matches the spec for the task.

src/components/side_panel/data_validation/dv_criterion_form/dv_input/dv_input.ts:63,70 — nit

localizeContent(...) ?? "" — localizeContent is typed (string, Locale) => string and never returns nullish. Drop the fallback or change the helper's contract.

src/components/side_panel/data_validation/dv_criterion_form/dv_input/dv_input.ts:60-65 — warning

onWillUpdateProps overwrites state.textInput whenever the parent's props.value changes. If the parent re-renders for any reason while the user has uncommitted input in the field, their text is lost. In practice the parent only re-renders the value on confirm, so it's fine today — but worth gating on nextProps.value !== canonicalizeContent(this.state.textInput, locale) to be safe.

tests — coverage gap (warning)

No test exercises the CF editor color-scale/icon-set localized input path added in ef2dd7a63 (typing 1,5 in FR locale should canonicalize to 1.5 in the rule). Worth a focused component test.