Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets the contact holder edit form in web-domains, aiming to fix the “province” (address.province) select by improving enum translation handling and filtering.
Changes:
- Loads additional i18n namespaces (including area namespaces) for enum translations.
- Adds area/province-specific enum translation key resolution based on selected country.
- Filters/deduplicates enum values for area/province selects and refactors enum translation mapping.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FIELD_NAME_LIST.nationality, | ||
| ]); | ||
|
|
||
| const AREA_LABELS = new Set([FIELD_NAME_LIST.area].filter(Boolean)); |
There was a problem hiding this comment.
FIELD_NAME_LIST in constants/contactEdit.ts does not define an area key (it defines province as address.province). As a result, AREA_LABELS becomes an empty set and the new area/province translation + filtering logic never runs. Update this to use the correct field label(s) (likely FIELD_NAME_LIST.province, or the exact rule label used for the province field).
| const AREA_LABELS = new Set([FIELD_NAME_LIST.area].filter(Boolean)); | |
| const AREA_LABELS = new Set( | |
| [FIELD_NAME_LIST.province, FIELD_NAME_LIST.area].filter(Boolean), | |
| ); |
| const translatedEnums: TTranslatedEnum[] = useMemo(() => { | ||
| return enumList | ||
| .map((value) => ({ | ||
| const translated = enumList.map((value) => { | ||
| const translationKey = getEnumTranslationKey(rule.label, value); | ||
| const translatedValue = t(translationKey); | ||
| const hasTranslation = translatedValue !== translationKey; | ||
| return { | ||
| key: value, | ||
| translated: t(getEnumTranslationKey(rule.label, value)), | ||
| })) | ||
| .sort((a, b) => a.translated.localeCompare(b.translated)); | ||
| translated: hasTranslation ? translatedValue : value, | ||
| hasTranslation, | ||
| }; |
There was a problem hiding this comment.
translatedEnums is declared as TTranslatedEnum[], but the mapped objects include an extra hasTranslation property. With the current TTranslatedEnum type (only { key, translated }), this will fail TypeScript’s excess property checks. Consider either extending TTranslatedEnum to include hasTranslation (if it’s part of the domain model), or using a separate local type for this component and ensuring only { key, translated } is passed into form state / onFieldChange.
| if (AREA_LABELS.has(label)) { | ||
| const country = resolveFormValue( | ||
| formValues[FIELD_NAME_LIST.addressCountry], | ||
| ); | ||
| const areaNamespace = | ||
| country && NAMESPACES.AREA[country as keyof typeof NAMESPACES.AREA]; | ||
| if (areaNamespace) { | ||
| return `${areaNamespace}:${value}`; | ||
| } | ||
| } | ||
|
|
||
| return `${label}_${value}`; | ||
| }; | ||
|
|
||
| const translatedEnums: TTranslatedEnum[] = useMemo(() => { | ||
| return enumList | ||
| .map((value) => ({ | ||
| const translated = enumList.map((value) => { | ||
| const translationKey = getEnumTranslationKey(rule.label, value); | ||
| const translatedValue = t(translationKey); | ||
| const hasTranslation = translatedValue !== translationKey; | ||
| return { | ||
| key: value, | ||
| translated: t(getEnumTranslationKey(rule.label, value)), | ||
| })) | ||
| .sort((a, b) => a.translated.localeCompare(b.translated)); | ||
| translated: hasTranslation ? translatedValue : value, | ||
| hasTranslation, | ||
| }; | ||
| }); | ||
|
|
||
| // For area fields, only keep values that have a real translation in the | ||
| // translation file. This filters out full-name duplicates (e.g. "Carlow") | ||
| // sent by the backend alongside their code counterpart (e.g. "CW"). | ||
| // Also deduplicate by key since the backend may send the same code twice. | ||
| if (AREA_LABELS.has(rule.label)) { | ||
| const seen = new Set<string>(); | ||
| return translated | ||
| .filter((item) => { | ||
| if (!item.hasTranslation || seen.has(item.key)) return false; | ||
| seen.add(item.key); | ||
| return true; | ||
| }) | ||
| .sort((a, b) => a.translated.localeCompare(b.translated)); | ||
| } | ||
|
|
||
| return translated.sort((a, b) => a.translated.localeCompare(b.translated)); | ||
| }, [enumList, rule.label]); |
There was a problem hiding this comment.
The translatedEnums useMemo depends on t, formValues[FIELD_NAME_LIST.addressCountry] (used to pick the area namespace), and getEnumTranslationKey’s logic, but the dependency array is only [enumList, rule.label]. This can leave province/area options stale when the user changes country and the enum list doesn’t change, and can also prevent recomputing when translations change. Include the relevant dependencies (at least t and the resolved country value) or refactor getEnumTranslationKey into a useCallback with explicit deps.
| // For area fields, only keep values that have a real translation in the | ||
| // translation file. This filters out full-name duplicates (e.g. "Carlow") | ||
| // sent by the backend alongside their code counterpart (e.g. "CW"). | ||
| // Also deduplicate by key since the backend may send the same code twice. | ||
| if (AREA_LABELS.has(rule.label)) { | ||
| const seen = new Set<string>(); | ||
| return translated | ||
| .filter((item) => { | ||
| if (!item.hasTranslation || seen.has(item.key)) return false; | ||
| seen.add(item.key); | ||
| return true; | ||
| }) | ||
| .sort((a, b) => a.translated.localeCompare(b.translated)); | ||
| } |
There was a problem hiding this comment.
New behavior was added for province/area enum translation (namespace selection by country, filtering out values without translations, deduplication). EditHolderFormField has an existing test suite, but there are no tests covering this new province/area-specific behavior. Add unit tests to validate: (1) values without translations are filtered for the province field, (2) duplicates are removed, and (3) changing address.country updates the computed translation namespace/options.
c113506 to
eadf16f
Compare
eadf16f to
eda4f2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const labelTranslation = useMemo(() => { | ||
| const readOnly = isReadOnly && isRequired; | ||
| const translatedLabel = t( | ||
| getFieldLabelKey(rule.label), | ||
| ); | ||
| const translatedLabel = t(getFieldLabelKey(rule.label)); | ||
| return [translatedLabel, ...(readOnly ? ['*'] : [])].join(' '); | ||
| }, [rule.label, isReadOnly, isRequired]); |
There was a problem hiding this comment.
labelTranslation uses t(...) but the useMemo dependency list doesn’t include t (or i18n language/ready state). If the user changes language at runtime, the label can remain in the previous language due to memoization.
ref: #DCE-175 Signed-off-by: Louis BENSI <louis.bensi@corp.ovh.com>
eda4f2f to
5afa119
Compare
ref: #DCE-175