Name the required deposit currency in the waiting-for-submitter-account next step - #94786
Name the required deposit currency in the waiting-for-submitter-account next step#94786ShridharGoel wants to merge 14 commits into
Conversation
…nt next step When the backend next step includes requiredDepositCurrency (set when a submitter's deposit account is the wrong currency for direct reimbursement), render "add a USD bank account" instead of the generic "add a bank account". Falls back to the generic copy when the field is absent.
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@Eskalifer1 Please 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 335a281f6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@trjExpensify Can you help with running the translations workflow? |
|
Hi! I will review it on Monday |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: The diff is too large to include in this comment (98KB), so I've created a gist for you: 📋 View the translation diff here 📋 Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Nice lil' improvement. 👍
|
@Eskalifer1 Hi, did you get the chance to complete the review? |
| const account = requiredDepositCurrency ? `Cuenta bancaria en ${requiredDepositCurrency}` : 'cuenta bancaria'; | ||
| switch (actorType) { | ||
| case CONST.NEXT_STEP.ACTOR_TYPE.CURRENT_USER: | ||
| return `Esperando a que <strong>tú</strong> añadas una cuenta bancaria.`; | ||
| return `Esperando a que <strong>tú</strong> añadas una ${account}.`; |
There was a problem hiding this comment.
In some languages, the word “bank” is now capitalized; I think this should be corrected, as in this example here
|
Hi @ShridharGoel lmk when this will be ready! Thank you! |
|
Hi @ShridharGoel Gentle bump here! |
|
Continuing now. |
I think web only should be fine (assuming you'll be checking on mobile) |
# Conflicts: # src/languages/de.ts # src/languages/es.ts # src/languages/fr.ts # src/languages/ja.ts # src/languages/nl.ts # src/languages/pl.ts # src/languages/pt-BR.ts # src/languages/zh-hans.ts
|
@Eskalifer1 Updated |
|
I have a question. Why are we changing only one thing in |
|
@codex review |
It was part of the translation diff, looks like it runs around the whole block where some change was made. It added types to the parameters and few other extra changes. I'm wondering whether we should just keep those changes or remove them manually. I'll revert the tag change though, not sure why the workflow generated that as well. Thanks for pointing it out. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd205ecf2d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return `<next-step>${translate(`nextStep.message.${nextStep.messageKey}`, actor, actorType, eta, etaType)}</next-step>`; | ||
| const message = | ||
| nextStep.messageKey === CONST.NEXT_STEP.MESSAGE_KEY.WAITING_FOR_SUBMITTER_ACCOUNT | ||
| ? translate('nextStep.message.waitingForSubmitterAccount', actor, actorType, eta, etaType, nextStep.requiredDepositCurrency) |
There was a problem hiding this comment.
When WAITING_FOR_SUBMITTER_ACCOUNT includes requiredDepositCurrency, this new string is passed straight into the translation and then rendered as HTML by MoneyReportHeaderStatusBar's RenderHTML path. Unlike the actor name escaped just above, an unexpected API/Onyx value containing markup would be parsed as HTML instead of displayed as a currency code, so escape or strictly validate it before interpolation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think that's valid. Any ideas @ShridharGoel?
There was a problem hiding this comment.
It's better to add the check, already updated.
|
Most of what Codex writes about is something we didn’t touch in this PR; I think within the scope of this PR, we only need to update what pertains to this PR. Maybe we need help from someone on the internal team to figure out what to do in this situation, since the translations were generated for purposes they weren’t intended for—what do you think about that @ShridharGoel? |
Restore existing next-step copy while keeping currency-aware bank account messages in every locale.
|
Actually it's better to keep the related changes only. Can you check now? |
API values enter RenderHTML through translated next-step copy. Encode the currency so markup is displayed as text instead of parsed.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #93938 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Checklist is completed here |
|
@Eskalifer1 I think if you approve again after the time of completing the checklist then the check will pass. |
Eskalifer1
left a comment
Eskalifer1
left a comment
There was a problem hiding this comment.
LGTM!
(Trying to fix checklist failing)
Thank you! It worked! |
|
Hi @ShridharGoel, NAB, but let's also resolve that one |
|
@Eskalifer1 Isn't that already handled? |
|
Updated in the description. |
|
I see, you meant updating the description only. Done now, thanks. |
4 participants
Explanation of Change
When a workspace reimburses directly into a deposit account (e.g. a USD Bancorp bank account), the submitter's deposit account must be in the matching currency. When it isn't, the report's next step is "Waiting for [submitter] to add a bank account" — but it never said which currency was required.
The backend now sends a
requiredDepositCurrencyvalue on the next step for this case. This PR renders it, so the message reads "Waiting for [submitter] to add a USD bank account" instead of the generic copy. When the field is absent (e.g. the submitter has no deposit account at all), the copy stays generic.Changes:
requiredDepositCurrencyto theReportNextStepOnyx type.buildNextStepMessagepasses it to thewaitingForSubmitterAccounttranslation.This is the NewDot half of the backend work in Auth#22421, which sets the field.
Fixed Issues
$ #93938, https://github.com/Expensify/Expensify/issues/644032
PROPOSAL:
Tests
Offline tests
QA Steps
Same as the Tests above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
MacOS: Chrome / Safari