Skip to content

Commit

Permalink
Fix onboarding library integration
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Gudahtt committed Nov 10, 2020
1 parent 5434d8d commit 6e123d1
Show file tree
Hide file tree
Showing 11 changed files with 101 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export default class ImportWithSeedPhrase extends PureComponent {
onSubmit: PropTypes.func.isRequired,
setSeedPhraseBackedUp: PropTypes.func,
initializeThreeBox: PropTypes.func,
completeOnboarding: PropTypes.func,
}

state = {
Expand Down Expand Up @@ -129,7 +128,6 @@ export default class ImportWithSeedPhrase extends PureComponent {
onSubmit,
setSeedPhraseBackedUp,
initializeThreeBox,
completeOnboarding,
} = this.props

try {
Expand All @@ -143,7 +141,6 @@ export default class ImportWithSeedPhrase extends PureComponent {
})

setSeedPhraseBackedUp(true).then(async () => {
await completeOnboarding()
initializeThreeBox()
history.push(INITIALIZE_END_OF_FLOW_ROUTE)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { connect } from 'react-redux'
import {
setSeedPhraseBackedUp,
initializeThreeBox,
setCompletedOnboarding,
} from '../../../../store/actions'
import ImportWithSeedPhrase from './import-with-seed-phrase.component'

Expand All @@ -11,7 +10,6 @@ const mapDispatchToProps = (dispatch) => {
setSeedPhraseBackedUp: (seedPhraseBackupState) =>
dispatch(setSeedPhraseBackedUp(seedPhraseBackupState)),
initializeThreeBox: () => dispatch(initializeThreeBox()),
completeOnboarding: () => dispatch(setCompletedOnboarding()),
}
}

Expand Down
31 changes: 25 additions & 6 deletions ui/app/pages/first-time-flow/end-of-flow/end-of-flow.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,52 @@ 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',
action: 'Onboarding Complete',
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
Expand Down
Original file line number Diff line number Diff line change
@@ -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 = {
Expand All @@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ describe('End of Flow Screen', function () {
history: {
push: sinon.spy(),
},
setCompletedOnboarding: sinon.spy(),
}

beforeEach(function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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 <Redirect to={{ pathname: DEFAULT_ROUTE }} />
}

if (seedPhraseBackedUp !== null) {
return <Redirect to={{ pathname: INITIALIZE_END_OF_FLOW_ROUTE }} />
}

if (isUnlocked) {
return <Redirect to={{ pathname: LOCK_ROUTE }} />
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(<FirstTimeFlowSwitch.WrappedComponent />)
it('redirects to /welcome route with null props', function () {
const props = {
completedOnboarding: null,
isInitialized: null,
isUnlocked: null,
seedPhraseBackedUp: null,
}
const wrapper = mountWithRouter(
<FirstTimeFlowSwitch.WrappedComponent {...props} />,
)
assert.equal(
wrapper
.find('Lifecycle')
Expand All @@ -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(
<FirstTimeFlowSwitch.WrappedComponent {...props} />,
)

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(
<FirstTimeFlowSwitch.WrappedComponent {...props} />,
)

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(
Expand All @@ -56,6 +100,7 @@ describe('FirstTimeFlowSwitch', function () {
completedOnboarding: false,
isUnlocked: false,
isInitialized: false,
seedPhraseBackedUp: null,
}

const wrapper = mountWithRouter(
Expand All @@ -75,6 +120,7 @@ describe('FirstTimeFlowSwitch', function () {
completedOnboarding: false,
isUnlocked: false,
isInitialized: true,
seedPhraseBackedUp: null,
}

const wrapper = mountWithRouter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export default class ConfirmSeedPhrase extends PureComponent {
seedPhrase: PropTypes.string,
initializeThreeBox: PropTypes.func,
setSeedPhraseBackedUp: PropTypes.func,
completeOnboarding: PropTypes.func,
}

state = {
Expand Down Expand Up @@ -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

Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { connect } from 'react-redux'
import {
setSeedPhraseBackedUp,
initializeThreeBox,
setCompletedOnboarding,
} from '../../../../store/actions'
import ConfirmSeedPhrase from './confirm-seed-phrase.component'

Expand All @@ -11,7 +10,6 @@ const mapDispatchToProps = (dispatch) => {
setSeedPhraseBackedUp: (seedPhraseBackupState) =>
dispatch(setSeedPhraseBackedUp(seedPhraseBackupState)),
initializeThreeBox: () => dispatch(initializeThreeBox()),
completeOnboarding: () => dispatch(setCompletedOnboarding()),
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ describe('ConfirmSeedPhrase Component', function () {
history: { push: pushSpy },
setSeedPhraseBackedUp: () => Promise.resolve(),
initializeThreeBox: initialize3BoxSpy,
completeOnboarding: sinon.spy(),
},
{
metricsEvent: metricsEventSpy,
Expand Down

0 comments on commit 6e123d1

Please sign in to comment.