Handle error in workspace's bank account page #15394
Conversation
|
Current state: |
melvin-bot
bot
requested review from
mananjadhav
and removed request for
a team
|
@mananjadhav @youssef-lr 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] |
| <OfflineWithFeedback | ||
| pendingAction={pendingAction} | ||
| errors={errors} | ||
| shouldShowErrorMessage | ||
| onClose={() => { | ||
| PaymentMethods.clearDeletePaymentMethodError(ONYXKEYS.BANK_ACCOUNT_LIST, achData.bankAccountID); | ||
| }} | ||
| > |
There was a problem hiding this comment.
This looks like a lot of changes because of the change of level of indentation
…ect-workspace-bankaccount
aldo-expensify
changed the title
aldo-expensify
requested review from
s77rt
and removed request for
mananjadhav
|
Taking PR screenshots... |
| @@ -266,7 +266,6 @@ function openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep) { | |||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | |||
| key: ONYXKEYS.REIMBURSEMENT_ACCOUNT, | |||
| value: { | |||
| errors: null, | |||
There was a problem hiding this comment.
We re-load the reimbursementAccount data each time we enter the "Connect bank account" page of a workspace. If there is an error, errors: null, was clearing the errors, not allowing the user to clear it with the X and not giving time to read it.
| <OfflineWithFeedback | ||
| pendingAction={pendingAction} | ||
| errors={errors} | ||
| shouldShowErrorMessage | ||
| onClose={BankAccounts.resetReimbursementAccount} |
There was a problem hiding this comment.
Wrapped in OfflineWithFeedback to:
- Show error
- Add strikethrough if offline and
pendingAction = 'delete'
| @@ -115,6 +120,7 @@ class WorkspacesListPage extends Component { | |||
| * @returns {Array} the menu item list | |||
| */ | |||
| getWorkspaces() { | |||
| const reimbursementAccountBrickRoadIndicator = !_.isEmpty(this.props.reimbursementAccount.errors) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''; | |||
There was a problem hiding this comment.
The reimbursementAccount is not per workspace. There is a single one. All workspaces will appear with a red dot if there is an error in reimbursementAccount.errors
There was a problem hiding this comment.
Just to be sure, if you link a bank account with any workspace, it will be linked with all of them?
There was a problem hiding this comment.
That seems to be how it is currently modelled. You can have N workspaces, but can only have one reimbursementAccount, so it is shared across workspaces.
|
@s77rt can you give me a hand checking the current behaviour in I guess the part that makes sense to check out in |
I don't think this exists on
This behaviour exists on main too since it's linked to the backend response. |
…m:Expensify/App into aldo_fix-disconnect-workspace-bankaccount
| const errors = lodashGet(props.reimbursementAccount, 'errors', {}); | ||
| const pendingAction = lodashGet(props.reimbursementAccount, 'pendingAction', null); | ||
| return ( | ||
| <ScreenWrapper includeSafeAreaPaddingBottom={false}> | ||
| <FullPageNotFoundView shouldShow={_.isEmpty(props.policy)}> | ||
| <HeaderWithCloseButton | ||
| title={props.translate('workspace.common.bankAccount')} | ||
| subtitle={props.policyName} | ||
| onCloseButtonPress={Navigation.dismissModal} | ||
| onBackButtonPress={Navigation.goBack} | ||
| shouldShowGetAssistanceButton | ||
| guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_BANK_ACCOUNT} | ||
| shouldShowBackButton | ||
| /> | ||
| <ScrollView style={styles.flex1}> | ||
| <Section | ||
| title={props.translate('workspace.bankAccount.almostDone')} | ||
| icon={Illustrations.BankArrow} | ||
| > |
There was a problem hiding this comment.
This looks like lots of changes but it is really just an indentation level change + these new variables errors and pendingAction
|
Updated the PR / resolved conflicts. @s77rt can you review this again 🙏 ? As you have seen, there are things that don't work great when you play with two devices at the same time, but these things also don't work in Another option for making this easier for review is to split the PR in smaller PR adding small stuff at a time. For example.. add the red brick path stuff, add the errors in the Continue/start over screen/ etc. @youssef-lr this is ready for review. |
|
Will review asap. |
|
I have verified tests still pass 👍 No VBBAno-vbba.mp4VBAA In Progress (with error)vbba-in-progress-with-error.mp4VBBA In Progressvbba-in-progress.mp4VBBA Open (with error)vbba-open-with-error.mp4VBBA Openvbba-open.mp4 |
melvin-bot
bot
requested review from
jasperhuangg and
s77rt
and removed request for
a team
|
@s77rt @jasperhuangg 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] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@aldo-expensify let us know if you plan to QA or should we take some platforms |
|
🚀 Deployed to staging by https://github.com/jasperhuangg in version: 1.3.3-0 🚀
|
|
@mvtglobally I can take care of the QA if needed, did you already do it? |
|
@aldo-expensify it would be great if you can. It sounds complex. 😜 |
QA StepsError setup: I'm using the applause testing account Under this setup, trying to delete the VBBA will throw an error. With an account that has a workspace and no VBBA:
Having a bank account in progress (completed
|
|
QA passed ✅ |
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.3-2 🚀
|
| title={props.translate('workspace.bankAccount.almostDone')} | ||
| icon={Illustrations.BankArrow} | ||
| > | ||
| <OfflineWithFeedback |
There was a problem hiding this comment.
Hi all! Just dropping a note that this PR caused issue - #18517
Wrapping with OfflineWithFeedback crosses out all text. In bank account page, it doesn't makes sense to have the strikethrough because the component is static, that is not showing a user input (chat message, workspace etc).
There was a problem hiding this comment.
Nice catch, but with removing the pending action we may lose some other intended strikethough styles?
https://github.com/Expensify/App/pull/15394/files#r1119420530
Details
Handle errors when disconnecting VBBA from workspace and when restarting VBBA setup
Refactored code deciding to show
ContinueBankAccountSetupmaking it more consistent across different cases (refresh with the workspace's VBBA page open, open for the first time the workspace's VBBA page, open for a second time the workspace's VBBA page, open the VBBA page directly using a URL)Fixed Issues
$ #14999
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Error setup: When needed, to easily force the backend to throw an error when using the command
RestartBankAccountSetup, hardcode thebankAccountIDsent in the request to0here:App/src/libs/actions/ReimbursementAccount/resetFreePlanBankAccount.js
Line 22 in 5ef05fc
The flows that end with an error in the view with the options "Continue with setup" / "Start over" should look like this:
With an account that has a workspace and no VBBA:
Settings > Workspace > Connect with bank account: Should show buttons "Connect online with Plaid" / "Connect manually" and the url should be/bank-account/newSettings > Workspace > Connect with bank account, close RHN, go toSettings > Workspace > Connect with bank accountagain: Should show buttons "Connect online with Plaid" / "Connect manually" and the url should be/bank-account/new/bank-account/newHaving a bank account in progress (completed
Company informationstep and reachedPersonal information):Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Continue with setup” should take you to the right step with the right url (step in the url)Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Refresh the page, you should get the same view, same options and same url/bank-account.Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Start over” should take you to the start with options "Connect online with Plaid" / "Connect manually", and the url should be ‘/bank-account/new‘Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Start over” throws error, Check reloading, closing the RHN and opening bank account config again, clearing the errorHaving a bank account in OPEN state
Settings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Clicking “Disconnect bank account” should take you to the start where you can pick "Connect online with Plaid" / "Connect manually" and the url should be/bank-account/new.Settings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Refresh the page, you should get the same view, same options and same urlSettings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Close the RHN, got back toSettings > Workspace > Connect with bank accountyou should get the same view, same options and same url.Settings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Click “Disconnect bank account”, after loading, it should show an error message with an "X" to dismiss. Verify the red block path and verify that clearing the error works.Having a bank account in PENDING state (waiting to enter verification numbers):
Login, go to
Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Continue with setup” should take you to the step to enter transactions with the url/bank-account/validate.Login, go to
Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Refresh the page, you should get the same view, same options and same urlVerify that no errors appear in the JS console
Offline tests
The add VBBA flow requires internet to work, so the offline support is very limited. You can see that there is a loading spinner when you restart, we can't proceed optimistically without the response from the server.
If you disable the internet connection and click a button like "Disconnect bank account", you will get a page telling you that you need a connection.
QA Steps
Feel free to ping @aldo-expensify to take over the lengthy QA
Error setup: When needed, you will need to:
Under this setup, trying to delete the VBBA will throw an error.
With an account that has a workspace and no VBBA:
Settings > Workspace > Connect with bank account: Should show buttons "Connect online with Plaid" / "Connect manually" and the url should be/bank-account/newSettings > Workspace > Connect with bank account, close RHN, go toSettings > Workspace > Connect with bank accountagain: Should show buttons "Connect online with Plaid" / "Connect manually" and the url should be/bank-account/new/bank-account/newHaving a bank account in progress (completed
Company informationstep and reachedPersonal information):Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Continue with setup” should take you to the right step with the right url (step in the url)Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Refresh the page, you should get the same view, same options and same url/bank-account.Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Start over” should take you to the start with options "Connect online with Plaid" / "Connect manually", and the url should be ‘/bank-account/new‘Having a bank account in OPEN state
Settings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Clicking “Disconnect bank account” should take you to the start where you can pick "Connect online with Plaid" / "Connect manually" and the url should be/bank-account/new.Settings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Refresh the page, you should get the same view, same options and same urlSettings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Close the RHN, got back toSettings > Workspace > Connect with bank accountyou should get the same view, same options and same url.Settings > Workspace > Connect with bank account: Should show the option “Disconnect bank account”, the url is ‘/bank-account/enabled‘. Click “Disconnect bank account”, after loading, it should show an error message with an "X" to dismiss. Verify the red block path and verify that clearing the error works.Having a bank account in PENDING state (waiting to enter verification numbers):
Login, go to
Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Clicking “Continue with setup” should take you to the step to enter transactions with the url/bank-account/validate.Login, go to
Settings > Workspace > Connect with bank account: Should show options “Continue with setup” / “Start over”, the url is/bank-account(no step). Refresh the page, you should get the same view, same options and same urlVerify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Screen.Recording.2023-02-27.at.4.04.37.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-02-27.at.4.56.50.PM.mov
Mobile Web - Safari
Screen.Recording.2023-02-27.at.4.55.45.PM.mov
Desktop
Screen.Recording.2023-02-27.at.4.57.34.PM.mov
iOS
Screen.Recording.2023-02-27.at.4.54.52.PM.mov
Android
Screen.Recording.2023-02-27.at.4.53.42.PM.mov