Adding periodic reminder modal for backing up recovery phrase #11021

ryanml · 2021-05-10T03:12:18Z

The goal is to display this reminder two days after wallet creation/import (or in the case of an update, two days from that point). After the user dismisses the reminder, it will subsequently appear every 90 days.

Manual Test Plan

In app/scripts/controllers/app-state.js, set REMINDER_CHECK_INTERVAL to equal time.SECOND * 30 (30 seconds), INITIAL_REMINDER_FREQUENCY to equal time.MINUTE * 2 (2 minutes), and FOLLOWUP_REMINDER_FREQUENCY to equal time.MINUTE * 4 (4 minutes)
Run a dev build from this branch
Create/Import a wallet
View Notifications and dismiss "what's new popup" (this notification should not appear when the "what's new popup" is open)
Confirm after 2 minutes that the reminder appears.
Dismiss the reminder
Confirm that after 4 minutes, the reminder appears agin.
Dismiss the reminder, re-confirm step 7

Appearance

(User has not backed up their seed phrase)

(User has backed up their seed phrase)

metamaskbot · 2021-05-10T03:53:24Z

Builds ready [1ee7b23]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (582 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	41	76	55	10	5
		domContentLoaded	358	685	581	83	40
		load	359	686	582	83	40
		domInteractive	357	684	580	83	40

ryanml · 2021-05-12T12:45:25Z

ui/components/ui/popover/popover.component.js

@@ -17,6 +17,7 @@ const Popover = ({
  showArrow,


Per the designs, there is no 'X' button (closure happens upon clicking "Got it!" at the bottom of the popup), and the title is centered.

metamaskbot · 2021-05-12T13:11:07Z

Builds ready [64ae808]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (555 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	42	78	59	9	4
		domContentLoaded	392	689	553	90	43
		load	393	690	555	90	43
		domInteractive	392	688	553	90	43

ryanml · 2021-05-12T13:14:55Z

shared/constants/time.js

@@ -0,0 +1,5 @@
+export const MILLISECOND = 1;


After merge I'll put up a change to use these across the codebase where applicable

darkwing · 2021-05-14T21:29:23Z

app/scripts/controllers/app-state.js

+    }
+
+    // Capture the current timestamp
+    const currentTime = new Date().getTime();


Nit: we could move this var declaration above if (recoveryPhraseReminderLastShown === 0) { and then use it on line 181.

good catch, i can update this

metamaskbot · 2021-05-15T00:38:37Z

Builds ready [2962484]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (578 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	71	54	8	4
		domContentLoaded	385	633	577	77	37
		load	386	635	578	77	37
		domInteractive	384	633	576	77	37

danjm · 2021-05-19T23:40:43Z

There might be a way that this can be simplified so that fewer background code additions are needed and no migration is required.

What I'm thinking is:

recoveryPhraseReminderLastShown can be exposed via state to the front end
Instead of shouldShowRecoveryPhraseReminder as a property, it could be a redux selector. This selector could compare the current time to the recoveryPhraseReminderLastShown time. If recoveryPhraseReminderLastShown is not yet defined, assume that the user has never been shown the reminder, and return true if (currentTime - 0) > 2 days. If recoveryPhraseReminderLastShown is defined, assume the user has seen the reminder before, and return true if (currentTime - 0) > 90 days
then, instead of calling setRecoveryPhraseReminderHasBeenShown from the front end when the reminder is closed, just call setRecoveryPhraseReminderLastShown with the current date/time

With this, I think we can greatly reduce the amount of background code added with this PR, and remove the need for a migration.

Let me know what you think.

ryanml · 2021-05-24T02:06:18Z

@danjm - I have refactored this PR per our discussion. cc: @darkwing

metamaskbot · 2021-05-24T03:22:51Z

Builds ready [2f73ba8]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (578 ± 30 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	65	56	8	4
		domContentLoaded	400	641	577	63	30
		load	401	642	578	63	30
		domInteractive	399	641	576	63	30

metamaskbot · 2021-05-24T18:18:32Z

Builds ready [664acf8]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (723 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	55	87	66	8	4
		domContentLoaded	435	945	722	125	60
		load	436	946	723	125	60
		domInteractive	434	945	721	125	60

danjm

My major suggestion involved the refactor to simplify controller logic. Those changes look good and this PR looks good overall to me. Nice work!

* Adding recurring recovery phrase reminder modal * Refactoring per PR feedback

ryanml self-assigned this May 10, 2021

ryanml force-pushed the backup-reminder-modal branch from 34a6225 to 41bf2da Compare May 10, 2021 03:24

ryanml dismissed a stale review via 1ee7b23 May 10, 2021 03:37

ryanml force-pushed the backup-reminder-modal branch from 41bf2da to 1ee7b23 Compare May 10, 2021 03:37

ryanml force-pushed the backup-reminder-modal branch from 1ee7b23 to aec9e88 Compare May 12, 2021 12:28

ryanml marked this pull request as ready for review May 12, 2021 12:36

ryanml requested a review from a team as a code owner May 12, 2021 12:36

ryanml requested a review from danjm May 12, 2021 12:36

ryanml force-pushed the backup-reminder-modal branch from cd0bb14 to 4d7e18e Compare May 12, 2021 12:38

ryanml commented May 12, 2021

View reviewed changes

ryanml force-pushed the backup-reminder-modal branch 2 times, most recently from d2f98f1 to 64ae808 Compare May 12, 2021 12:55

ryanml commented May 12, 2021

View reviewed changes

darkwing reviewed May 14, 2021

View reviewed changes

ryanml force-pushed the backup-reminder-modal branch from 64ae808 to 2962484 Compare May 15, 2021 00:23

ryanml requested a review from darkwing May 15, 2021 00:23

ryanml force-pushed the backup-reminder-modal branch from 2962484 to 603e651 Compare May 19, 2021 23:20

Adding recurring recovery phrase reminder modal

5649ea2

ryanml force-pushed the backup-reminder-modal branch from 603e651 to 91c5de6 Compare May 24, 2021 02:04

ryanml force-pushed the backup-reminder-modal branch 2 times, most recently from c4d1ca9 to 2f73ba8 Compare May 24, 2021 03:07

Refactoring per PR feedback

664acf8

ryanml force-pushed the backup-reminder-modal branch from 2f73ba8 to 664acf8 Compare May 24, 2021 18:00

danjm approved these changes May 31, 2021

View reviewed changes

ryanml merged commit 9932c40 into develop Jun 5, 2021

ryanml deleted the backup-reminder-modal branch June 5, 2021 06:33

ryanml added a commit that referenced this pull request Jun 22, 2021

Adding periodic reminder modal for backing up recovery phrase (#11021)

6a23866

* Adding recurring recovery phrase reminder modal * Refactoring per PR feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding periodic reminder modal for backing up recovery phrase #11021

Adding periodic reminder modal for backing up recovery phrase #11021

ryanml commented May 10, 2021 •

edited

metamaskbot commented May 10, 2021

ryanml May 12, 2021

metamaskbot commented May 12, 2021

ryanml May 12, 2021

darkwing May 14, 2021

ryanml May 14, 2021

ryanml May 15, 2021

metamaskbot commented May 15, 2021

danjm commented May 19, 2021 •

edited

ryanml commented May 24, 2021

metamaskbot commented May 24, 2021

metamaskbot commented May 24, 2021

danjm left a comment

Adding periodic reminder modal for backing up recovery phrase #11021

Adding periodic reminder modal for backing up recovery phrase #11021

Conversation

ryanml commented May 10, 2021 • edited

Manual Test Plan

Appearance

(User has not backed up their seed phrase)

(User has backed up their seed phrase)

metamaskbot commented May 10, 2021

ryanml May 12, 2021

Choose a reason for hiding this comment

metamaskbot commented May 12, 2021

ryanml May 12, 2021

Choose a reason for hiding this comment

darkwing May 14, 2021

Choose a reason for hiding this comment

ryanml May 14, 2021

Choose a reason for hiding this comment

ryanml May 15, 2021

Choose a reason for hiding this comment

metamaskbot commented May 15, 2021

danjm commented May 19, 2021 • edited

ryanml commented May 24, 2021

metamaskbot commented May 24, 2021

metamaskbot commented May 24, 2021

danjm left a comment

Choose a reason for hiding this comment

ryanml commented May 10, 2021 •

edited

danjm commented May 19, 2021 •

edited