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

Fix onboarding library integration #9835

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Fix onboarding library integration #9835

merged 1 commit into from
Nov 10, 2020

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Nov 9, 2020

Fixes: #9792

Explanation:

The bug with our onboarding library integration was introduced in #8873 because of a change in when completeOnboarding was called. We hadn't realized at the time that the onboarding integration relied upon the onboarding completing event to know when the onboarding state should be cleared. Because onboarding is now marked as completed earlier, the state was cleared just as it was intended to be used.

The onboarding completed event has been moved back to where it was before: after the user exits the "end of flow" page.

The original problem that #8873 was addressing was a routing issue, where the user would be redirected back to the seed phrase confirmation page despite already having confirmed their seed phrase. This was fixed in a different way here, by updating the routing in the first time flow switch to skip straight to the end of flow page if the seed phrase has already been confirmed.

This does involve one user-facing change in behavior; if the user opens any MetaMask UI before navigating away from the end-of-flow screen, they will still be considered mid-onboarding so it'll redirect to the end-of-flow screen. But we do mark onboarding as completed if the user closes the tab/window while on the end of flow screen, which was another goal of #8873.

Manual testing steps:

To test that the onboarding integration works:

  • Open a browser without MetaMask installed
  • Navigate to https://metamask.github.io/test-dapp/
  • Press the "CLICK HERE TO INSTALL METAMASK" button
  • Install MetaMask
  • Proceed through onboarding to the last page
  • See that the message "All Done" will close this tab and direct back to https://metamask.github.io/test-dapp/ is shown below the "All done" button

To test that the user is redirected back to the end-of-flow page if the user opens a tab while the end of flow page is open:

  • Install MetaMask
  • Proceed through onboarding to the last page
  • Open the MetaMask popup while on the last page of onboarding
  • See that a new tab is opened, which navigates to the last page of onboarding

To test that onboarding is marked as completed when the end of flow page is closed:

  • Install MetaMask
  • Proceed through onboarding to the last page
  • Close the tab
  • Open the MetaMask popup, and see that the home screen is shown

@metamaskbot
Copy link
Collaborator

Builds ready [6985c67]
Page Load Metrics (474 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3510552209
domContentLoaded3747554728239
load3767574748239
domInteractive3747544728239

@Gudahtt
Copy link
Member Author

Gudahtt commented Nov 9, 2020

This depends upon #9830

Base automatically changed from track-seed-phrase-backup to develop November 10, 2020 16:04
@Gudahtt Gudahtt marked this pull request as ready for review November 10, 2020 16:05
@Gudahtt Gudahtt requested a review from a team as a code owner November 10, 2020 16:05
@Gudahtt Gudahtt requested a review from darkwing November 10, 2020 16:05
The bug with our onboarding library integration was introduced in #8873
because of a change in when `completeOnboarding` was called. We hadn't
realized at the time that the onboarding integration relied upon the
onboarding completing event to know when the onboarding state should
be cleared. Because onboarding is now marked as completed earlier, the
state was cleared just as it was intended to be used.

The onboarding completed event has been moved back to where it was
before: after the user exits the "end of flow" page.

The original problem that #8873 was addressing was a routing issue,
where the user would be redirected back to the seed phrase confirmation
page despite already having confirmed their seed phrase. This was fixed
in a different way here, by updating the routing in the first time flow
switch to skip straight to the end of flow page if the seed phrase has
already been confirmed.

This does involve one user-facing change in behavior; if the user opens
any MetaMask UI before navigating away from the end-of-flow screen,
they will still be considered mid-onboarding so it'll redirect to the
end-of-flow screen. But we do mark onboarding as completed if the user
closes the tab/window while on the end of flow screen, which was
another goal of #8873.
@metamaskbot
Copy link
Collaborator

Builds ready [6e123d1]
Page Load Metrics (374 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298540115
domContentLoaded2546203739948
load25562137410048
domInteractive2546193729948

@darkwing
Copy link
Contributor

All flows worked exactly as described!

@Gudahtt Gudahtt merged commit 552ea13 into develop Nov 10, 2020
@Gudahtt Gudahtt deleted the fix-onboarding branch November 10, 2020 21:27
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding library redirect broken on last page of onboarding
3 participants