Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@arosiclair I needed to reintroduce backTo param to fix this bug - #83308 (comment) - because the previous solution wasn't working for the wallet page and other locations from where we can enter the flow. We will remove it after the backTo -> dynamic routes project #73825 is finished. LMK if you're ok with that. I'm still working on improving the redirection (discussed here #83308 (comment)) in the meantime, @dukenv0307 could you please run another round of review? |
|
Looks better now Screen.Recording.2026-03-16.at.09.52.02.mov |
Yup makes sense to me 👍 |
|
@dukenv0307 I think I improved redirection enough, also fixed some issues with missing data when navigating back. Could you please run another round of the review? |
@dukenv0307 no we don't really have to - I changed the mechanism to navigate to each substep explicitly, so we don't have this redirection on the hook level. And when navigating explicitly we don't have the navigation problem. So even if we decide to change that we can always do it in the PR when we'll remove the backTo param (After dynamic navigation project is finished). |
|
okay, on it now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-20.at.09.31.39.movAndroid: mWeb ChromeScreen.Recording.2026-03-20.at.09.24.48.moviOS: HybridAppScreen.Recording.2026-03-20.at.09.27.15.moviOS: mWeb SafariScreen.Recording.2026-03-20.at.09.23.04.movMacOS: Chrome / SafariScreen.Recording.2026-03-20.at.09.18.54.mov |
|
@koko57 We got the conflict |
|
conflicts resolved |
arosiclair
left a comment
There was a problem hiding this comment.
Wow this was even bigger than I thought it would be. Just a few comments
| [SCREENS.REIMBURSEMENT_ACCOUNT_NON_USD]: { | ||
| animationTypeForReplace: 'pop', | ||
| }, |
There was a problem hiding this comment.
Is this change still necessary? We made some related changes in this PR recently: https://github.com/Expensify/App/pull/85531/changes
| const ownerKeys = reimbursementAccountDraft?.beneficialOwnerKeys ?? beneficialOwnersDraftValues.beneficialOwnerKeys; | ||
|
|
||
| useEffect(() => { | ||
| if (reimbursementAccountDraft?.beneficialOwnerKeys || beneficialOwnersDraftValues.beneficialOwnerKeys.length === 0) { |
There was a problem hiding this comment.
| if (reimbursementAccountDraft?.beneficialOwnerKeys || beneficialOwnersDraftValues.beneficialOwnerKeys.length === 0) { | |
| if (!reimbursementAccountDraft?.beneficialOwnerKeys || beneficialOwnersDraftValues.beneficialOwnerKeys.length === 0) { |
Is this right?
|
|
||
| type DraftValuesForSignerInfo = { | ||
| isUserDirector: boolean; | ||
| [key: string]: string | boolean; |
There was a problem hiding this comment.
The keys for this result object seem fixed. Can we add them all to the type?
|
|
||
| const isAUD = currency === CONST.CURRENCY.AUD; | ||
| const pages = useMemo(() => { | ||
| const filtered = isDocusignStepRequired ? allPages : allPages.filter((p) => p.pageName !== PAGE_NAME.DOCUSIGN); |
There was a problem hiding this comment.
| const filtered = isDocusignStepRequired ? allPages : allPages.filter((p) => p.pageName !== PAGE_NAME.DOCUSIGN); | |
| const filteredPages = isDocusignStepRequired ? allPages : allPages.filter((p) => p.pageName !== PAGE_NAME.DOCUSIGN); |
| const isAUD = currency === CONST.CURRENCY.AUD; | ||
| const pages = useMemo(() => { | ||
| const filtered = isDocusignStepRequired ? allPages : allPages.filter((p) => p.pageName !== PAGE_NAME.DOCUSIGN); | ||
| return filtered.map((p) => { |
There was a problem hiding this comment.
| return filtered.map((p) => { | |
| return filtered.map((page) => { |
Explanation of Change
Fixed Issues
$ #79048
PROPOSAL:
Tests
Prerequisites: being an admin of at least one workspace, the workspace should have global reimbursements supported currency (EUR, CAD, AUD or GBP)
Go to Workspace > Workflows
Click Add bank account
Verify that the Country and Currency is filled in with supported values (EU Countries, Canada, Austrania or UK)
Go through the flow, fill in all the fields
Click browser / native back button
Verify that you're redirected to the previous step, not out of the flow
Go to Wallet
Click Add bank account
On the Bank Purpose Page choose Make Payments
Choose one of the countries
Repeat steps 4-6 from the previous scenario
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-03-03.at.16.17.07.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-03-03.at.16.08.56.mp4