Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@alitoshmatov 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c03ea2a4d
ℹ️ 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".
|
@alitoshmatov Hey, it's not yet ready for review - I will add some more fixes and tests. We had a bunch of issues yesterday that we thought were blockers but ultimately got demoted. I thought we will want to merge it ASAP but since we have some time now I will add all the fixes here. |
|
@dominictb will be reviewer here because he is the dedicated C+ reviewer for this project |
|
🚧 @joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@dominictb Ready for your review :) |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Tested and all four cases work for me 👍
|
@dominictb could you please proceed with the review here? |
|
Seems like Screen.Recording.2026-01-28.at.14.36.13-compressed.mov |
|
For #79323, I think neither the expected behavior nor the fix is complete. Other steps also have the same issue, not just I think the real issue is not about
Once decided to add a completely new BBA, we should reset the progress; not to continue what's left with the partial BBA that was initially selected. Screen.Recording.2026-01-28.at.14.53.33.mov@joekaufmanexpensify might confirm here. |
|
|
||
| const selectPaymentMethod = useCallback( | ||
| (paymentMethod?: PaymentMethod, policy?: Policy) => { | ||
| const canLinkExistingBusinessBankAccount = getEligibleExistingBusinessBankAccounts(bankAccountList, policy?.outputCurrency, true).length > 0; |
There was a problem hiding this comment.
Do we need to move this into the memoized function? Can we move it outside like before?
There was a problem hiding this comment.
We need the policy that comes as argument provided to selectPaymentMethod
| if (policyID === undefined) { | ||
| return; | ||
| } | ||
|
|
Hm, I think this is the real issue. Once you select a PENDING bank account, we shouldn't show the select acount page if you go back from the test transactions step. We should link the PENDING bank account to the workspace. You can't then add a different bank account unless you delete the pending bank account. This had been working before when I tested it on adhoc, but maybe something has changed, cc @MrMuzyk |
|
Thanks. @MrMuzyk If so we need to fix for other steps too (we currently only fix for VALIDATION step). Basically once linked, we can never go back to existing BBA page. |
|
I'll look into the comments. If something changed it must've changed on BE side as PR wasn't update at all over last 2 weeks. |
|
This bug you've described has the very same cause as this one . We've had a long chat about it and in result there will be a new field added to |
If account is PENDING then it should always end up on VALIDATION step afaik -> https://github.com/Expensify/App/blob/main/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx#L226 So I don't think we need a safeguard for other steps. Unless Im missing something...? 🤔 |
|
Ah okay 👍 Got it now. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-02-01.at.18.40.38.movScreen.Recording.2026-02-01.at.18.41.37-compressed.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-01.at.18.30.12.movScreen.Recording.2026-02-01.at.18.33.41.mov |
|
Tested well.
@MrMuzyk Do you think we should block this PR for that backend change or would you handle that in a separate PR later? As I tested today, that bug is not fixed yet. |
Actually if this is already tested, approved and can get merged lets do that 🙏 I'll open a separate PR with the fix for that one last issue |
|
@dominictb Small correction. Ive linked the wrong issue in this comment. Correct issue I wanted to link is - #79312. Im working on separate PR which will also address this issue you've mentioned |
|
✋ 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/srikarparsi in version: 9.3.11-19 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
Explanation of Change
Fixes for multiple issues found during testing
Fixed Issues
$ #79317
$ #79318
$ #79323
$ #79252
PROPOSAL:
Tests
Same as QA steps
Offline tests
QA Steps
Scenario A:
Preconditions:
Create two USD workspaces - A and B.
Workspace A is set up with business bank account.
Scenario B:
Preconditions:
Create a USD workspace.
Create a non-USD workspace.
USD workspace is set up with business bank account.
Scenario C:
Precondition:
Create two USD workspaces - A and B.
Add a pending bank account to Workspace A.
Scenario D:
Precondition:
User has created a USD workspace and add business bank account.
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
Screen.Recording.2026-01-13.at.13.07.58.mov
nonusd65912.mp4
rhp.mp4
bulkpay.mp4