Repository navigation
[CP Staging] Show "removed" instead of "set to """ when a text report field is cleared - #104551
Conversation
…leared Co-authored-by: daledah <[email protected]>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
MelvinBot
changed the title
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
🦜 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/el.ts b/src/languages/el.ts
index e5ce5b44..e982bb98 100644
--- a/src/languages/el.ts
+++ b/src/languages/el.ts
@@ -10752,7 +10752,7 @@ ${reportName}`,
changeField: (oldValue, newValue, fieldName) => `άλλαξε το ${fieldName} σε «${newValue}» (προηγουμένως «${oldValue}»)`,
changeFieldEmpty: (newValue, fieldName) => `ορίστε το ${fieldName} σε «${newValue}»`,
changeFieldRemoved: (oldValue, fieldName) => `αφαίρεσε το ${fieldName} (προηγουμένως «${oldValue}»)`,
- changeFieldCleared: (fieldName) => `αφαίρεσε το ${fieldName}`,
+ changeFieldCleared: (fieldName: string) => `καταργήθηκε το ${fieldName}`,
changeReportPolicy: (toPolicyName: string, fromPolicyName?: string) => {
if (!toPolicyName) {
return `άλλαξε τον χώρο εργασίας${fromPolicyName ? `(προηγουμένως ${fromPolicyName})` : ''}`;
diff --git a/src/languages/es.ts b/src/languages/es.ts
index 4a7148d4..766459f9 100644
--- a/src/languages/es.ts
+++ b/src/languages/es.ts
@@ -10354,7 +10354,7 @@ ${reportName}`,
changeField: (oldValue, newValue, fieldName) => `cambió ${fieldName} a "${newValue}" (previamente "${oldValue}")`,
changeFieldEmpty: (newValue, fieldName) => `estableció ${fieldName} a ${newValue}`,
changeFieldRemoved: (oldValue, fieldName) => `eliminó ${fieldName} (previamente "${oldValue}")`,
- changeFieldCleared: (fieldName) => `eliminó ${fieldName}`,
+ changeFieldCleared: (fieldName: string) => `eliminó ${fieldName}`,
changeReportPolicy: (toPolicyName, fromPolicyName) => {
if (!toPolicyName) {
return `cambió el espacio de trabajo${fromPolicyName ? ` (previamente ${fromPolicyName})` : ''}`;
diff --git a/src/languages/it.ts b/src/languages/it.ts
index 4dae6d7d..caebbae9 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -10500,7 +10500,7 @@ Aggiungi altre regole di spesa per proteggere il flusso di cassa aziendale.`,
changeField: (oldValue: string | undefined, newValue: string, fieldName: string) => `ha modificato ${fieldName} in "${newValue}" (in precedenza "${oldValue}")`,
changeFieldEmpty: (newValue: string, fieldName: string) => `imposta ${fieldName} su "${newValue}"`,
changeFieldRemoved: (oldValue: string, fieldName: string) => `ha rimosso ${fieldName} (in precedenza "${oldValue}")`,
- changeFieldCleared: (fieldName: string) => `ha rimosso ${fieldName}`,
+ changeFieldCleared: (fieldName: string) => `rimosso ${fieldName}`,
changeReportPolicy: (toPolicyName: string, fromPolicyName?: string) => {
if (!toPolicyName) {
return `ha modificato il workspace${fromPolicyName ? `(precedentemente ${fromPolicyName})` : ''}`;
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index eb588fad..b147222d 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -10346,7 +10346,7 @@ ${reportName}`,
changeField: (oldValue: string | undefined, newValue: string, fieldName: string) => `${fieldName} を「${newValue}」（以前は「${oldValue}」）に変更しました`,
changeFieldEmpty: (newValue: string, fieldName: string) => `${fieldName} を「${newValue}」に設定`,
changeFieldRemoved: (oldValue: string, fieldName: string) => `${fieldName} を削除しました（以前は「${oldValue}」）`,
- changeFieldCleared: (fieldName: string) => `${fieldName} を削除しました`,
+ changeFieldCleared: (fieldName: string) => `${fieldName}を削除しました`,
changeReportPolicy: (toPolicyName: string, fromPolicyName?: string) => {
if (!toPolicyName) {
return `ワークスペース${fromPolicyName ? `（以前は${fromPolicyName}）` : ''}を変更しました`;
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index f8f7cd52..c2e70842 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -10470,7 +10470,7 @@ er bestedingsregels toe om de kasstroom van het bedrijf te beschermen.`,
changeField: (oldValue: string | undefined, newValue: string, fieldName: string) => `heeft ${fieldName} gewijzigd in „${newValue}” (voorheen „${oldValue}”)`,
changeFieldEmpty: (newValue: string, fieldName: string) => `stel ${fieldName} in op "${newValue}"`,
changeFieldRemoved: (oldValue: string, fieldName: string) => `heeft ${fieldName} verwijderd (voorheen „${oldValue}”)`,
- changeFieldCleared: (fieldName: string) => `heeft ${fieldName} verwijderd`,
+ changeFieldCleared: (fieldName: string) => `${fieldName} verwijderd`,
changeReportPolicy: (toPolicyName: string, fromPolicyName?: string) => {
if (!toPolicyName) {
return `heeft de werkruimte${fromPolicyName ? `(voorheen ${fromPolicyName})` : ''} gewijzigd`;
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 6bf3c6ab..ed7e49a0 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -10065,7 +10065,7 @@ ${reportName}`,
changeField: (oldValue: string | undefined, newValue: string, fieldName: string) => `将${fieldName}更改为“${newValue}”（先前为“${oldValue}”）`,
changeFieldEmpty: (newValue: string, fieldName: string) => `将 ${fieldName} 设置为“${newValue}”`,
changeFieldRemoved: (oldValue: string, fieldName: string) => `已移除${fieldName}（先前为“${oldValue}”）`,
- changeFieldCleared: (fieldName: string) => `已移除${fieldName}`,
+ changeFieldCleared: (fieldName: string) => `已移除 ${fieldName}`,
changeReportPolicy: (toPolicyName: string, fromPolicyName?: string) => {
if (!toPolicyName) {
return `更改了工作区${fromPolicyName ? `（原为 ${fromPolicyName}）` : ''}`;
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
|
The failing check is unrelated to this PR, so I didn't push a fix. Please rerun the failed Analysis: In Evidence: This PR changes the language files, |
|
🤖 Android test steps verification results: The broken
|
|
@MelvinBot please open this PR and fill your checklists |
|
🤖 Web test steps verification results: I confirmed the report field setup and that a new expense's report shows the field's workspace initial value. I could not finish the clear-and-press-Enter step, so I could not capture the system message: the web test tool cannot send an Enter keypress. This is a test tooling limit, not a product result. The PR's Tests section is still a TODO, so these steps come from the linked issue's reproduction steps.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
I marked this PR ready for review and updated the PR Author Checklist. I checked only items that are true or don't apply. The What I checked: the items that don't apply to this change. It changes only system message text in What you still need to do:
Possible gap: On Android, clearing a field that showed only its initial value posts |
| */ | ||
| function buildOptimisticChangeFieldAction(reportField: PolicyReportField, previousReportField: PolicyReportField, currentUserAccountID: number): OptimisticChangeFieldAction { | ||
| // A field that was never saved on the report shows its workspace default, so that is the value the user changed | ||
| const previousValue = previousReportField.value ?? previousReportField.defaultValue; |
There was a problem hiding this comment.
@MelvinBot the ?? previousReportField.defaultValue fallback in buildOptimisticChangeFieldAction is what causes the flicker. I logged it on web:
- Client sends its own id via
reportFieldsActionIDs, so the server reuses the samereportActionIDand overwritesoriginalMessage. - Optimistic:
oldValue: 'init text'→removed Text (previously "init text"). - ~760ms later the server's copy lands with
oldValuegone →removed Text.
|
I didn't push a fix because no code change is needed. Since my last comment, the PR head is unchanged ( Why it's unrelated: The runner killed a Jest worker ( Details
|
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2026-10-21.at.18.36.28.mov |
|
No code change is needed, so I didn't push anything. The failure won't clear until someone with Actions access reruns the failed
To fix it, open the failed job and select Re-run failed jobs. |
The server reuses the client's reportActionID and overwrites originalMessage without the default as oldValue, so the fallback made the message flicker from 'removed text (previously ...)' to 'removed text'. Co-authored-by: daledah <[email protected]>
|
@daledah @AndrewGable One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Agreed. I removed the
Typecheck passed, and |
…in the CHANGEFIELD test Co-authored-by: daledah <[email protected]>
|
Fixed the failing Cause: The new Fix: Replaced the two reads with one Verification: ESLint no longer reports these errors on the file. I couldn't run Jest in this environment, so CI will confirm the test still passes.
|
grgia
changed the title
…FieldMessage [CP Staging] Show "removed" instead of "set to """ when a text report field is cleared (cherry picked from commit 0a1f77f) (cherry-picked to staging by grgia)
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🔁 Filed a Staging retest request for deploy blockers #104507 after this PR was cherry-picked to staging. |
|
🚀 Cherry-picked to staging by https://github.com/grgia in version: 9.5.15-2 🚀
|
|
No help site update is needed. This bug fix only changes the in-app system message shown after you clear a text report field, and no help article describes that message. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/grgia in version: 9.5.16-0 🚀
|
|
No help site update is needed. This bug fix only changes the in-app system message shown after someone clears a text report field, and no article under |
|
🚀 Cherry-picked to staging by https://github.com/grgia in version: 9.5.18-0 🚀
|
|
No help site update needed. This PR fixes the wording of a report-history system message when someone clears a report field, and no article in |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.18-1 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Clearing a text report field that only showed its workspace initial value produced the system message
set text to "". The report had no saved value for the field, so theCHANGEFIELDaction had an emptyoldValue. The message builder used the "removed" wording only whenoldValuewas non-empty. With both values empty, it fell through tochangeFieldEmpty(set {field} to "{value}").This PR:
getMessageOfOldDotReportActionto check for an emptynewValuefirst. It showsremoved {field} (previously "{oldValue}")when there is an old value, and a newchangeFieldClearedmessage,removed {field}, when there isn't. A cleared field can no longer render asset {field} to "".changeFieldClearedto every locale, plus unit tests.buildOptimisticChangeFieldActionas it is onmain:oldValueis the report's saved value. Falling back to the field'sdefaultValuemade the optimistic message readremoved text (previously "<initial value>"), then flicker toremoved textwhen the server's action, which reuses the samereportActionID, replaced it. Clearing a never-saved field now showsremoved textfrom the start.Fixed Issues
$ #104507
PROPOSAL: #104507 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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
MelvinBot test run on standalone Android NewDot. Clearing a text report field that showed its initial value "hello" posts
removed textinstead ofset text to "".Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
AI Tests
npm run typecheck: passednpm run lint-changed: passednpm run fmt(oxfmt): no formatting changesnpm test -- tests/unit/ReportActionsUtilsTest.ts tests/unit/ReportUtilsTest.ts: 2009/2009 passed (includes 3 new tests)generateTranslations.ts --dry-run(ts-nodeis not installed in this environment)defaultValuefallback:npm run typecheckpassed and the two test files above passed 2009/2009. Lint and oxfmt could not run in that environment; the change restores the lines frommain.