[HOLD for payment 2023-02-01] [$250] There's no back button at bank offline blocking view reported by @thesahindia

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Disconnect the internet connection
2. Navigate to settings > Workspaces
3. Select a workspace
4. Click on "connect bank account"

Expected Result:

There should be a back button

Actual Result:

There's no back button

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.27-3

Reproducible in staging?: y

Reproducible in production?: y

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/201705335-a707a598-e65e-42cb-9b4e-6f2556c555a9.mov

Expensify/Expensify Issue URL:

Issue reported by: @thesahindia

Slack conversation:
https://expensify.slack.com/archives/C049HHMV9SM/p1668369917906139

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[Puneet-here]

Proposal

We need to show the back button when the user is offline we can make the changes as suggested at ReimbursementAccountLoadingIndicator

         <HeaderWithCloseButton
             title={props.translate('reimbursementAccountLoadingAnimation.oneMoment')}
             onCloseButtonPress={Navigation.dismissModal}
+            shouldShowBackButton={props.network.isOffline}
+            onBackButtonPress={Navigation.goBack}
         />
         <FullPageOfflineBlockingView>
             {props.isSubmittingVerificationsData ? (
@@ -49,4 +53,7 @@ const ReimbursementAccountLoadingIndicator = props => (
 ReimbursementAccountLoadingIndicator.propTypes = propTypes;
 ReimbursementAccountLoadingIndicator.displayName = 'ReimbursementAccountLoadingIndicator';
 
-export default withLocalize(ReimbursementAccountLoadingIndicator);
+export default compose(
+    withNetwork(),
+    withLocalize,
+)(ReimbursementAccountLoadingIndicator);

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[melvin-bot]

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[melvin-bot]

Triggered auto assignment to @AndrewGable (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[dylanexpensify]

Upwork: https://www.upwork.com/jobs/~012907523c42742c6b

[eVoloshchak]

@Puneet-here's proposal looks good to me

🎀👀🎀 C+ reviewed!
cc: @AndrewGable

This means back button will be hidden only while a network request is in progress, so is there a reason to hide it at all?
Being able to go back while loading is nice

[AndrewGable]

Agree that user should always be able to go back (I think). I do not think we should hide back button at all.

cc @marcaaron who might have navigation ideas...

[marcaaron]

Agree. This just looks implemented incorrectly to me.

[AndrewGable]

@Puneet-here - Can you amend your proposal with this in mind? Then I will assign and I look forward to reviewing the PR.

[Puneet-here]

Updated proposal

Pass shouldShowBackButton and onBackButtonPress at

App/src/components/ReimbursementAccountLoadingIndicator.js

Line 23 in 6b8c0dc

<HeaderWithCloseButton

         <HeaderWithCloseButton
             title={props.translate('reimbursementAccountLoadingAnimation.oneMoment')}
             onCloseButtonPress={Navigation.dismissModal}
+            shouldShowBackButton
+            onBackButtonPress={Navigation.goBack}
         />

[melvin-bot]

@AndrewGable, @eVoloshchak, @dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[dylanexpensify]

reviewing today!

[marcaaron]

On second thought... I am looking at this again... and not really sure if we need to do this. If you're offline you can't really move through the different steps of the bank account flow anyway. cc @MariaHCD who might know more (or who would know).

[thesahindia]

I believe there should be a back button. If a user clicks at "connect bank account" in workspace (when offline) they will see the offline blocking view. Now if they wanna do something else, they will try to go back but there is no back button.

[ctkochan22]

So this offline blocker is introduced in ReimbursementAccountPage.js and its used between steps as well.

When not in a loading state, the is a back button send the user to the previous setup step. So instead of jumping back to the last page like Navigation.goBack(), we need to go to the previous setup step in the flow, regardless of where they were before this. For example, if on the "Personal Information" step, if you click the back button, it does not go to the last navigation step. It instead goes to Company step.

Introducing a back button here with the logic Navigation.goBack() will be strange for the user when:

• Here the back button takes them back to the company step
  
• And here, the back button goes back to the previous page which is the workspace settings.
  

Instead, we can do a few things.

1. We could not offer the back button during the offline/loading periods
2. Extract and consolidate the go back logic from BankStep, RequestorStep, etc. Pass that extracted go back method into ReimbursementAccountLoadingIndicator and the substep, so that they will all work consistently.

I think the 2nd option may be the way to go? @nkuoch @MariaHCD curious as to your thoughts

[eVoloshchak]

@ctkochan22, that's a good catch!
I think we should go with the second step
@Puneet-here, could you update your proposal please?

[melvin-bot]

This issue has not been updated in over 15 days. @nkuoch, @dylanexpensify eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[JmillsExpensify]

What's going on with this issue? Do we still have a PR in review?

[nkuoch]

Yes, ended up up creating a single PR with a refactoring and fixing several issues of the flow, was ooo at some point and it's a bit long to review because the PR is pretty big. But it's currently in final review and I'm confident it will be merged really soon.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.58-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix and refactor VBBA setup flow #13236

If no regressions arise, payment will be issued on 2023-02-01. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@nkuoch] The PR that introduced the bug has been identified. Link to the PR: N/A - Bug has always been there
• [@nkuoch] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• [@nkuoch] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A - It was an improvement more than a bug
• [@dylanexpensify] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[dylanexpensify]

Working on this today!

[dylanexpensify]

RT here

[thesahindia]

@dylanexpensify, this is eligible for reporting compensation. We will need an upwork job here.

[dylanexpensify]

Ah yes, will do now

[dylanexpensify]

job here, also invited you!

[thesahindia]

Applied, thanks!

[dylanexpensify]

offer sent!

[thesahindia]

@dylanexpensify, accepted!

[dylanexpensify]

paid! Closing this out @nkuoch reopen if something is missing!