From 6e123d14f9b5a13330e123033c257122af1f1139 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 6 Nov 2020 22:35:00 -0330 Subject: [PATCH] Fix onboarding library integration 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. --- .../import-with-seed-phrase.component.js | 3 -- .../import-with-seed-phrase.container.js | 2 - .../end-of-flow/end-of-flow.component.js | 31 +++++++++--- .../end-of-flow/end-of-flow.container.js | 9 +++- .../end-of-flow/tests/end-of-flow.test.js | 1 + .../first-time-flow-switch.component.js | 13 ++++- .../first-time-flow-switch.container.js | 8 ++- .../tests/first-time-flow-switch.test.js | 50 ++++++++++++++++++- .../confirm-seed-phrase.component.js | 6 --- .../confirm-seed-phrase.container.js | 2 - .../confirm-seed-phrase-component.test.js | 1 - 11 files changed, 101 insertions(+), 25 deletions(-) diff --git a/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js b/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js index 1e17e8f08320..b9de02575854 100644 --- a/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js +++ b/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js @@ -21,7 +21,6 @@ export default class ImportWithSeedPhrase extends PureComponent { onSubmit: PropTypes.func.isRequired, setSeedPhraseBackedUp: PropTypes.func, initializeThreeBox: PropTypes.func, - completeOnboarding: PropTypes.func, } state = { @@ -129,7 +128,6 @@ export default class ImportWithSeedPhrase extends PureComponent { onSubmit, setSeedPhraseBackedUp, initializeThreeBox, - completeOnboarding, } = this.props try { @@ -143,7 +141,6 @@ export default class ImportWithSeedPhrase extends PureComponent { }) setSeedPhraseBackedUp(true).then(async () => { - await completeOnboarding() initializeThreeBox() history.push(INITIALIZE_END_OF_FLOW_ROUTE) }) diff --git a/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.container.js b/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.container.js index 25dafdb4cd71..7da5af5a1aa8 100644 --- a/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.container.js +++ b/ui/app/pages/first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.container.js @@ -2,7 +2,6 @@ import { connect } from 'react-redux' import { setSeedPhraseBackedUp, initializeThreeBox, - setCompletedOnboarding, } from '../../../../store/actions' import ImportWithSeedPhrase from './import-with-seed-phrase.component' @@ -11,7 +10,6 @@ const mapDispatchToProps = (dispatch) => { setSeedPhraseBackedUp: (seedPhraseBackupState) => dispatch(setSeedPhraseBackedUp(seedPhraseBackupState)), initializeThreeBox: () => dispatch(initializeThreeBox()), - completeOnboarding: () => dispatch(setCompletedOnboarding()), } } diff --git a/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.component.js b/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.component.js index ea92cf6e9453..826bf094e81e 100644 --- a/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.component.js +++ b/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.component.js @@ -15,19 +15,24 @@ export default class EndOfFlowScreen extends PureComponent { static propTypes = { history: PropTypes.object, completionMetaMetricsName: PropTypes.string, + setCompletedOnboarding: PropTypes.function, onboardingInitiator: PropTypes.exact({ location: PropTypes.string, tabId: PropTypes.number, }), } - onComplete = async () => { - const { - history, - completionMetaMetricsName, - onboardingInitiator, - } = this.props + async _beforeUnload() { + await this._onOnboardingComplete() + } + + _removeBeforeUnload() { + window.removeEventListener('beforeunload', this._beforeUnload) + } + async _onOnboardingComplete() { + const { setCompletedOnboarding, completionMetaMetricsName } = this.props + await setCompletedOnboarding() this.context.metricsEvent({ eventOpts: { category: 'Onboarding', @@ -35,13 +40,27 @@ export default class EndOfFlowScreen extends PureComponent { name: completionMetaMetricsName, }, }) + } + onComplete = async () => { + const { history, onboardingInitiator } = this.props + + this._removeBeforeUnload() + await this._onOnboardingComplete() if (onboardingInitiator) { await returnToOnboardingInitiator(onboardingInitiator) } history.push(DEFAULT_ROUTE) } + componentDidMount() { + window.addEventListener('beforeunload', this._beforeUnload.bind(this)) + } + + componentWillUnmount = () => { + this._removeBeforeUnload() + } + render() { const { t } = this.context const { onboardingInitiator } = this.props diff --git a/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.container.js b/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.container.js index b741b501a32f..5b0d00c17ffe 100644 --- a/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.container.js +++ b/ui/app/pages/first-time-flow/end-of-flow/end-of-flow.container.js @@ -1,5 +1,6 @@ import { connect } from 'react-redux' import { getOnboardingInitiator } from '../../../selectors' +import { setCompletedOnboarding } from '../../../store/actions' import EndOfFlow from './end-of-flow.component' const firstTimeFlowTypeNameMap = { @@ -18,4 +19,10 @@ const mapStateToProps = (state) => { } } -export default connect(mapStateToProps)(EndOfFlow) +const mapDispatchToProps = (dispatch) => { + return { + setCompletedOnboarding: () => dispatch(setCompletedOnboarding()), + } +} + +export default connect(mapStateToProps, mapDispatchToProps)(EndOfFlow) diff --git a/ui/app/pages/first-time-flow/end-of-flow/tests/end-of-flow.test.js b/ui/app/pages/first-time-flow/end-of-flow/tests/end-of-flow.test.js index 24a6c23b462e..ad7b0e7e112c 100644 --- a/ui/app/pages/first-time-flow/end-of-flow/tests/end-of-flow.test.js +++ b/ui/app/pages/first-time-flow/end-of-flow/tests/end-of-flow.test.js @@ -12,6 +12,7 @@ describe('End of Flow Screen', function () { history: { push: sinon.spy(), }, + setCompletedOnboarding: sinon.spy(), } beforeEach(function () { diff --git a/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.component.js b/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.component.js index 54503bf1d199..6caad3c0032b 100644 --- a/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.component.js +++ b/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.component.js @@ -4,6 +4,7 @@ import { Redirect } from 'react-router-dom' import { DEFAULT_ROUTE, LOCK_ROUTE, + INITIALIZE_END_OF_FLOW_ROUTE, INITIALIZE_WELCOME_ROUTE, INITIALIZE_UNLOCK_ROUTE, } from '../../../helpers/constants/routes' @@ -13,15 +14,25 @@ export default class FirstTimeFlowSwitch extends PureComponent { completedOnboarding: PropTypes.bool, isInitialized: PropTypes.bool, isUnlocked: PropTypes.bool, + seedPhraseBackedUp: PropTypes.bool, } render() { - const { completedOnboarding, isInitialized, isUnlocked } = this.props + const { + completedOnboarding, + isInitialized, + isUnlocked, + seedPhraseBackedUp, + } = this.props if (completedOnboarding) { return } + if (seedPhraseBackedUp !== null) { + return + } + if (isUnlocked) { return } diff --git a/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.container.js b/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.container.js index 1b35418b546f..92423f3dcdf6 100644 --- a/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.container.js +++ b/ui/app/pages/first-time-flow/first-time-flow-switch/first-time-flow-switch.container.js @@ -2,12 +2,18 @@ import { connect } from 'react-redux' import FirstTimeFlowSwitch from './first-time-flow-switch.component' const mapStateToProps = ({ metamask }) => { - const { completedOnboarding, isInitialized, isUnlocked } = metamask + const { + completedOnboarding, + isInitialized, + isUnlocked, + seedPhraseBackedUp, + } = metamask return { completedOnboarding, isInitialized, isUnlocked, + seedPhraseBackedUp, } } diff --git a/ui/app/pages/first-time-flow/first-time-flow-switch/tests/first-time-flow-switch.test.js b/ui/app/pages/first-time-flow/first-time-flow-switch/tests/first-time-flow-switch.test.js index 98a0f110391b..3ec0d1a61404 100644 --- a/ui/app/pages/first-time-flow/first-time-flow-switch/tests/first-time-flow-switch.test.js +++ b/ui/app/pages/first-time-flow/first-time-flow-switch/tests/first-time-flow-switch.test.js @@ -6,12 +6,21 @@ import { LOCK_ROUTE, INITIALIZE_WELCOME_ROUTE, INITIALIZE_UNLOCK_ROUTE, + INITIALIZE_END_OF_FLOW_ROUTE, } from '../../../../helpers/constants/routes' import FirstTimeFlowSwitch from '..' describe('FirstTimeFlowSwitch', function () { - it('redirects to /welcome route with no props', function () { - const wrapper = mountWithRouter() + it('redirects to /welcome route with null props', function () { + const props = { + completedOnboarding: null, + isInitialized: null, + isUnlocked: null, + seedPhraseBackedUp: null, + } + const wrapper = mountWithRouter( + , + ) assert.equal( wrapper .find('Lifecycle') @@ -35,10 +44,45 @@ describe('FirstTimeFlowSwitch', function () { ) }) + it('redirects to end of flow route when seedPhraseBackedUp is true', function () { + const props = { + completedOnboarding: false, + seedPhraseBackedUp: true, + } + const wrapper = mountWithRouter( + , + ) + + assert.equal( + wrapper + .find('Lifecycle') + .find({ to: { pathname: INITIALIZE_END_OF_FLOW_ROUTE } }).length, + 1, + ) + }) + + it('redirects to end of flow route when seedPhraseBackedUp is false', function () { + const props = { + completedOnboarding: false, + seedPhraseBackedUp: false, + } + const wrapper = mountWithRouter( + , + ) + + assert.equal( + wrapper + .find('Lifecycle') + .find({ to: { pathname: INITIALIZE_END_OF_FLOW_ROUTE } }).length, + 1, + ) + }) + it('redirects to /lock route when isUnlocked is true ', function () { const props = { completedOnboarding: false, isUnlocked: true, + seedPhraseBackedUp: null, } const wrapper = mountWithRouter( @@ -56,6 +100,7 @@ describe('FirstTimeFlowSwitch', function () { completedOnboarding: false, isUnlocked: false, isInitialized: false, + seedPhraseBackedUp: null, } const wrapper = mountWithRouter( @@ -75,6 +120,7 @@ describe('FirstTimeFlowSwitch', function () { completedOnboarding: false, isUnlocked: false, isInitialized: true, + seedPhraseBackedUp: null, } const wrapper = mountWithRouter( diff --git a/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.component.js b/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.component.js index 1c2cf25c767a..3494e5f0a7a5 100644 --- a/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.component.js +++ b/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.component.js @@ -26,7 +26,6 @@ export default class ConfirmSeedPhrase extends PureComponent { seedPhrase: PropTypes.string, initializeThreeBox: PropTypes.func, setSeedPhraseBackedUp: PropTypes.func, - completeOnboarding: PropTypes.func, } state = { @@ -70,10 +69,6 @@ export default class ConfirmSeedPhrase extends PureComponent { exportAsFile('', this.props.seedPhrase, 'text/plain') } - setOnboardingCompleted = async () => { - await this.props.completeOnboarding() - } - handleSubmit = async () => { const { history, setSeedPhraseBackedUp, initializeThreeBox } = this.props @@ -92,7 +87,6 @@ export default class ConfirmSeedPhrase extends PureComponent { setSeedPhraseBackedUp(true).then(async () => { initializeThreeBox() - this.setOnboardingCompleted() history.push(INITIALIZE_END_OF_FLOW_ROUTE) }) } catch (error) { diff --git a/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.container.js b/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.container.js index bd6cef274e04..646314ecd9fa 100644 --- a/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.container.js +++ b/ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.container.js @@ -2,7 +2,6 @@ import { connect } from 'react-redux' import { setSeedPhraseBackedUp, initializeThreeBox, - setCompletedOnboarding, } from '../../../../store/actions' import ConfirmSeedPhrase from './confirm-seed-phrase.component' @@ -11,7 +10,6 @@ const mapDispatchToProps = (dispatch) => { setSeedPhraseBackedUp: (seedPhraseBackupState) => dispatch(setSeedPhraseBackedUp(seedPhraseBackupState)), initializeThreeBox: () => dispatch(initializeThreeBox()), - completeOnboarding: () => dispatch(setCompletedOnboarding()), } } diff --git a/ui/app/pages/first-time-flow/seed-phrase/tests/confirm-seed-phrase-component.test.js b/ui/app/pages/first-time-flow/seed-phrase/tests/confirm-seed-phrase-component.test.js index b8f12e527ad5..a5a0d1c918c9 100644 --- a/ui/app/pages/first-time-flow/seed-phrase/tests/confirm-seed-phrase-component.test.js +++ b/ui/app/pages/first-time-flow/seed-phrase/tests/confirm-seed-phrase-component.test.js @@ -154,7 +154,6 @@ describe('ConfirmSeedPhrase Component', function () { history: { push: pushSpy }, setSeedPhraseBackedUp: () => Promise.resolve(), initializeThreeBox: initialize3BoxSpy, - completeOnboarding: sinon.spy(), }, { metricsEvent: metricsEventSpy,