Skip to content
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

[No QA] Improve VBA deep link routing #3711

Merged
merged 4 commits into from
Jun 23, 2021
Merged

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jun 21, 2021

Details

  • This PR adds better support for deep link routing + general navigation in the VBA flow.
  • Users can receive emails with links to specific steps in the VBA flow and this will allow them to advance to that step automatically.
  • Routes are also now updated to reflect the current step we are on in the achData

I'll create a follow up issue to address the comment left in the code. Basically, since we are allowing deep links to drop the user into a step - it's also possible for them to see a step that they can't possibly action successfully if they are unintentionally directed there. e.g. if they are not currently setting up a VBA and deep link to the RequestorStep it won't be possible for them to advance. This is an edge case since they should (theoretically) always have an account "in setup" if we are handing off a deep link.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/167246

Tests

  1. Navigate to /bank-account and verify you see this screen and that the url does not have any sub-route

2021-06-22_06-11-18

2. Next, we will be following instructions here to create a "PENDING" account here - but only complete the "Company Information" step 3. Manually add the account

2021-06-22_06-11-52

4. Verify the "Company Information" step has the `/company` sub route

2021-06-22_06-13-45

5. Advance to the RequestorStep and verify the url changes to `/requestor`

2021-06-22_06-16-37

6. Enter the subroute `/company` and verify you can return the to company step to edit the information there (**note:** some fields will not be available to edit)

2021-06-22_06-17-59

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Jun 21, 2021
@marcaaron marcaaron requested a review from a team as a code owner June 21, 2021 19:38
@MelvinBot MelvinBot requested review from MariaHCD and removed request for a team June 21, 2021 19:39
Copy link
Contributor

@MariaHCD MariaHCD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I noticed that the Tests and WebQA sections are empty. Is that intentional?

Comment on lines 99 to 101
// When the step changes we will navigate to update the route params. This is mostly cosmestic as we only use
// the route params when the component first mounts to jump to a specific route instead of picking up where the
// user left off in the flow.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// When the step changes we will navigate to update the route params. This is mostly cosmestic as we only use
// the route params when the component first mounts to jump to a specific route instead of picking up where the
// user left off in the flow.
// When the step changes we will navigate to update the route params. This is mostly cosmetic as we only use
// the route params when the component first mounts to jump to a specific route instead of picking up where the
// user left off in the flow.

@@ -75,7 +76,30 @@ const defaultProps = {

class ReimbursementAccountPage extends React.Component {
componentDidMount() {
fetchFreePlanVerifiedBankAccount();
// We can specify a specific step to navigate to by using route params when the component mounts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We can specify a specific step to navigate to by using route params when the component mounts.
// We can specify a step to navigate to by using route params when the component mounts.

NAB

@marcaaron
Copy link
Contributor Author

Looks good! I noticed that the Tests and WebQA sections are empty. Is that intentional?

Sorry meant to mark this as No QA. Will ask QA to test the VBA flow once it's complete. But will add some test steps.

@marcaaron marcaaron changed the title Improve VBA deep link routing [No QA] Improve VBA deep link routing Jun 22, 2021
@marcaaron
Copy link
Contributor Author

Updated with test steps + fixed comments.

@MariaHCD MariaHCD merged commit 7ea9941 into main Jun 23, 2021
@MariaHCD MariaHCD deleted the marcaaron-handleDeepLink branch June 23, 2021 10:38
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.73-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.74-0🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

const currentStep = lodashGet(
this.props,
'reimbursementAccount.achData.currentStep',
CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcaaron I have a question: Why do we need to add the default value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while for me - but it would seem to imply that there's no reimbursementAccount data to pull the currentStep from. The "bank account" step is the first step in this flow so that's why we fall back to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marcaaron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants