Revert "[CP Staging] Revert "Add translations for report field formula type"" #71908
Conversation
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index 8a3b4e92..eb445192 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -4810,7 +4810,7 @@ const translations = {
textAlternateText: 'Fügen Sie ein Feld für die freie Texteingabe hinzu.',
dateAlternateText: 'Fügen Sie einen Kalender zur Datumauswahl hinzu.',
dropdownAlternateText: 'Fügen Sie eine Liste von Optionen zur Auswahl hinzu.',
- formulaAlternateText: 'Fügen Sie ein Formularfeld hinzu.',
+ formulaAlternateText: 'Ein Formularfeld hinzufügen.',
nameInputSubtitle: 'Wählen Sie einen Namen für das Berichtsfeld.',
typeInputSubtitle: 'Wählen Sie aus, welche Art von Berichtsfeld verwendet werden soll.',
initialValueInputSubtitle: 'Geben Sie einen Startwert ein, der im Berichtsfeld angezeigt werden soll.',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index 9aa0ba3a..46079d74 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -4823,7 +4823,7 @@ const translations = {
textAlternateText: 'Ajoutez un champ pour la saisie de texte libre.',
dateAlternateText: 'Ajouter un calendrier pour la sélection de la date.',
dropdownAlternateText: "Ajouter une liste d'options à choisir.",
- formulaAlternateText: 'Ajouter un champ de formule.',
+ formulaAlternateText: 'Ajoutez un champ de formule.',
nameInputSubtitle: 'Choisissez un nom pour le champ du rapport.',
typeInputSubtitle: 'Choisissez le type de champ de rapport à utiliser.',
initialValueInputSubtitle: 'Entrez une valeur de départ à afficher dans le champ du rapport.',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| subtitle={translate('workspace.reportFields.typeInputSubtitle')} | ||
| rightLabel={translate('common.required')} | ||
| onTypeSelected={(type) => formRef.current?.resetForm({...inputValues, type, initialValue: type === CONST.REPORT_FIELD_TYPES.DATE ? defaultDate : ''})} | ||
| onTypeSelected={(type) => { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
This inline callback function creates a new function instance on every render, which can cause unnecessary re-renders of the TypeSelector component.
Consider extracting this callback to a separate memoized function:
const handleTypeSelected = useCallback((type) => {
let initialValue;
if (type === CONST.REPORT_FIELD_TYPES.DATE) {
initialValue = defaultDate;
} else if (type === CONST.REPORT_FIELD_TYPES.FORMULA) {
initialValue = '{report:id}';
} else {
initialValue = '';
}
formRef.current?.resetForm({
...inputValues,
type,
initialValue,
});
}, [inputValues, defaultDate]);
// Then use: onTypeSelected={handleTypeSelected}There was a problem hiding this comment.
Sounds like a good idea.
There was a problem hiding this comment.
This code existed and improving this is out of scope of this change, don't want to break random stuff
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Oh cool, thanks for working on re-applying this. There was an issue created, and I didn't realize your initial PR had been reverted, so we're working on a fix here FIX: Undefined when saving Text type report field using Formula.
We're making it so that it's not possible to create a formula field directly, you have to create a text field, and when you edit the initial value to include a formula, it will update the type to formula. Is that handled here, and do the tests cover all the tests in the issues that caused the revert?
Should we hold that other App PR for this?
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
The code looks good, so I'm happy with it if we have handled the testing properly. Also, let's get a C+ review.
| subtitle={translate('workspace.reportFields.typeInputSubtitle')} | ||
| rightLabel={translate('common.required')} | ||
| onTypeSelected={(type) => formRef.current?.resetForm({...inputValues, type, initialValue: type === CONST.REPORT_FIELD_TYPES.DATE ? defaultDate : ''})} | ||
| onTypeSelected={(type) => { |
There was a problem hiding this comment.
Sounds like a good idea.
|
LGTM |
. Thank you for your hard work!
No, this allows you to add a formula and it sets a default formula for you.
Yes, except the ones that were not bugs
Probably since it seems they are solving the same problem? |
Ah right, I remember now. Sounds good and thanks for explaining. I let the other PR know about this one. |
| style={styles.flex1} | ||
| enabledWhenOffline | ||
| isSubmitButtonVisible={isTextFieldType} | ||
| isSubmitButtonVisible={isTextFieldType || isFormulaFieldType} |
There was a problem hiding this comment.
NAB: i think this condition is redundant. The outer condition already ensures that only text or formula fields are true before rendering the FormProvider.
we can make it as true:
isSubmitButtonVisible
|
I am not sure about this but i noticed that we have a validation function for the text and list types that limits the max input ... should we also consider adding the formula case into the validation function what do you think @iwiznia @neil-marcellini |
|
|
||
| const fieldValue = reportField.value ?? reportField.defaultValue; | ||
| const isFieldDisabled = isReportFieldDisabled(report, reportField, policy); | ||
| const isFieldDisabled = isReportFieldDisabled(report, reportField, policy) || reportField.type === CONST.REPORT_FIELD_TYPES.FORMULA; |
There was a problem hiding this comment.
I have a question, I was thinking we can add this check already inaside the isReportFieldDisabled function. Is there a reason or scenario to keep it external?
There was a problem hiding this comment.
Could be nice, depends where else we use this function though
There was a problem hiding this comment.
Ah yes, that seems to work. Looked at the code and seems like it can be there.
|
I left a few comments, but the core flow works as expected, thus I am proceeding with the checklist ... |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-10-24.at.20.37.17.movAndroid: mWeb ChromeScreen.Recording.2025-10-24.at.20.42.37.moviOS: HybridAppScreen.Recording.2025-10-24.at.20.37.17.moviOS: mWeb SafariScreen.Recording.2025-10-24.at.20.41.12.movMacOS: Chrome / SafariScreen.Recording.2025-10-24.at.20.29.39.movMacOS: DesktopScreen.Recording.2025-10-24.at.20.44.56.mov |
Good catch, yes we should include formulas in that max length condition. |
|
|
||
| const fieldValue = reportField.value ?? reportField.defaultValue; | ||
| const isFieldDisabled = isReportFieldDisabled(report, reportField, policy); | ||
| const isFieldDisabled = isReportFieldDisabled(report, reportField, policy) || reportField.type === CONST.REPORT_FIELD_TYPES.FORMULA; |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The sortedPolicyReportFields useMemo hook is using entire objects (policy and report) as dependencies instead of specific properties.
Use specific properties as dependencies to avoid unnecessary recalculations:
const sortedPolicyReportFields = useMemo<PolicyReportField[]>((): PolicyReportField[] => {
const fields = getAvailableReportFields(report, Object.values(policy?.fieldList ?? {}));
return fields.filter((field) => field.target === report?.type).sort(({orderWeight: firstOrderWeight}, {orderWeight: secondOrderWeight}) => firstOrderWeight - secondOrderWeight);
}, [policy?.fieldList, report?.type, report?.reportID]); // Use specific properties
|
LGTM |
|
🎯 @abzokhattab, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #73524. |
|
@iwiznia Before last commit: 
After the commit: 
|
|
This change in the behavior is because of this condition After the last change, now the Thus, the question now is which behavior is right? should we just keep it hidden like others? |
|
LGTM after the fix |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Good to go, thanks!
neil-marcellini
deleted the
revert-71900-revert-71790-ionatan_addformulareportfieldtype
branch
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.2.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.2.39-3 🚀
|
Explanation of Change
Adding translations for report field of type formula
Fixed Issues
Context https://expensify.slack.com/archives/C01GTK53T8Q/p1759422862335019
See #71900
Tests
reportspage in the workspace editor in newDot{report:id}Offline tests
No
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop