-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Missing company card error handling for not success bank connection #70668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Missing company card error handling for not success bank connection #70668
Conversation
|
@shawnborton updated: |
|
That feels better, thoughts @Expensify/design ? |
|
LGTM 👍 |
|
Waiting on @nkuoch to make some changes on BE side to test end-2-end |
|
Backend PR in review. |
@nkuoch could you please specify how the error will be set inside Onyx - will be cool to get an example. thansk! |
# Conflicts: # src/pages/workspace/companyCards/BankConnection/index.tsx
|
https://github.com/Expensify/Web-Expensify/pull/48782 will change the error key from |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@ZhenjaHorbach 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] |
| if (!url) { | ||
| return; | ||
| } | ||
| // eslint-disable-next-line react-compiler/react-compiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was old code - but some eslint rules start to fire error: - in case we do not plan to change the whole flow - i think we are fine to leave it here.
83:9 error Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) react-compiler/react-compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh
Okay then
Thanks for the explanation!
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2025-10-03.11.32.28.movAndroid: mWeb Chrome2025-10-03.11.32.28.moviOS: HybridApp2025-10-03.11.03.33.moviOS: mWeb Safari2025-10-02.14.39.20.movMacOS: Chrome / Safari2025-10-02.14.23.21.movMacOS: Desktop2025-10-02.14.23.21.mov |
|
I don't think it's related to this PR 2025-10-03.11.05.46.mov |
|
But overall, the changes look good! |
I think that's correct - when you choose plaid - we set bank to Others option - and when you cancel flow - you landed on correct screen with other selected - which mean that after error - you already started a new flow (Plaid) |
Oh |
|
LGTM! |
|
Looks great, merging! Design reviewed here: #70668 (comment) |
|
✋ 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/robertjchen in version: 9.2.27-0 🚀
|
|
@narefyev91 @nkuoch Is there a way for testers to emulate the failed connection on step 3? |
|
@narefyev91 @robertjchen Can you please test it internally? QA team is not able to get the new modal. explorer_26hLTi1jI8.mp4 |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.27-6 🚀
|







Explanation of Change
We bring the user back to NewDot/close the connection popup window and show an error page explaining that there was an issue connecting cards and to try again. We should make sure it works this way for all card feed connection types.
Fixed Issues
$ #70144
PROPOSAL:
For C+ test - it's not possible to emulate locally failed connection. You could ask @nkuoch to help with that - or add mock logic like this
const newFeed = feed_123;
const workspaceAccountID = 18441278
Tests
Offline tests
No changes
QA Steps
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))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
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov