Implemented notification for user to backup Brave wallet #13504

jasonrsadler · 2018-03-19T20:34:00Z

Resolves #13425
Unblocks #4847

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

FOR QA ONLY:

Start Brave in staging environment and short interval timers from a console:
LEDGER_ENVIRONMENT=staging LEDGER_NOTIFICATION=true npm start
Keep Note of counter on console
Enable Payments
Add funds to wallet
Counter will be count up to notification show (in ms)
After 2 minutes, notification will display. Dismiss notification
After additional 5 minutes (from first notification show), notification will display. Dismiss notification
Each additional 1 minutes after 2nd notification is shown, notification will display.
Notification will continue to be displayed 1 once a minute until a backup or recovery action occurs or notifications are opted out.
Notifications will not display if a backup or recovery action has occurred.

IN PRODUCTION:
Notifications will be displayed if the user has not backed up their keys and has not opted out:
after 7 days,
after another 14 days,
once a month thereafter
If user turns off ledger notifications or backs up their wallet (or recovers a wallet) then this notification will not show.

Backup or Recovery action consists of:
Preferences->Payments->Advanced Settings

Click 'Backup Your Wallet'. Click 'Copy To Clipboard button'
Click 'Backup Your Wallet'. Click 'Print Key'
Click 'Backup Your Wallet'. Click 'Save Recovery File...' and save the file to disk
Click 'Recover Your Wallet'. Enter a valid recovery key and click 'Recover'
Click 'Recover Your Wallet'. Click 'Import Recovery Key'. Import a valid recovery file

unittest -- --grep="showBackupKeys" for simulated

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2018-03-19T21:28:59Z

Codecov Report

Merging #13504 into master will increase coverage by 0.02%.
The diff coverage is 77.06%.

@@            Coverage Diff             @@
##           master   #13504      +/-   ##
==========================================
+ Coverage   56.54%   56.57%   +0.02%     
==========================================
  Files         283      284       +1     
  Lines       29036    29100      +64     
  Branches     4824     4845      +21     
==========================================
+ Hits        16419    16463      +44     
- Misses      12617    12637      +20

Flag	Coverage Δ
#unittest	`56.57% <77.06%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
app/sessionStore.js	`85.71% <ø> (ø)`	⬆️
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
app/common/lib/ledgerUtil.js	`95.7% <ø> (ø)`	⬆️
js/actions/appActions.js	`18.83% <0%> (-0.11%)`	⬇️
app/locale.js	`35.77% <100%> (ø)`	⬆️
app/common/state/aboutPreferencesState.js	`100% <100%> (ø)`
app/browser/reducers/ledgerReducer.js	`44.7% <50%> (-0.3%)`	⬇️
...rer/components/preferences/payment/ledgerBackup.js	`36.58% <50%> (-0.26%)`	⬇️
app/browser/api/ledger.js	`63.15% <55.55%> (-0.15%)`	⬇️
app/browser/api/ledgerNotifications.js	`72.33% <70.37%> (+2.45%)`	⬆️
... and 9 more

NejcZdovc · 2018-03-19T22:44:20Z

app/browser/api/ledger.js

@@ -1063,6 +1063,9 @@ const backupKeys = (state, backupAction) => {

  if (backupAction === 'print') {
    tabs.create({url: appUrlUtil.aboutUrls.get('about:printkeys')})
+
+    // we do not check whether the user actually printed the backup word list
+    ledgerState.setBackupStatus(state, true)


We need to assign this call to the state, so that we actually save this change:

ledgerState.setBackupStatus(state, true) -> state = ledgerState.setBackupStatus(state, true)

NejcZdovc · 2018-03-19T22:44:27Z

app/browser/api/ledger.js

@@ -1079,8 +1082,10 @@ const backupKeys = (state, backupAction) => {
    if (file) {
      try {
        fs.writeFileSync(file, message)
+        ledgerState.setBackupStatus(state, true)


same as above

NejcZdovc · 2018-03-19T22:44:31Z

app/browser/api/ledger.js

      } catch (e) {
        console.error('Problem saving backup keys')
+        ledgerState.setBackupStatus(state, false)


same as above

jasonrsadler · 2018-04-01T13:45:50Z

@NejcZdovc , how can I trigger notifications in development?

NejcZdovc · 2018-04-04T10:43:56Z

@jasonrsadler which notifications?

NejcZdovc · 2018-04-04T10:47:42Z

Oh I know which one. You can do it like this

appActions.createTabRequested({
  url: 'about:preferences#payments?ledgerBackupOverlayVisible',
  windowId: xx
})

Where xx is current window ID

jasonrsadler · 2018-04-05T02:35:02Z

Yep. Got that. I meant the notification bar that comes down at the top of the webview?

NejcZdovc · 2018-04-05T04:05:59Z

@jasonrsadler check out ledgerNotifications.js, more specifically https://github.com/brave/browser-laptop/blob/master/app/browser/api/ledgerNotifications.js#L216

jasonrsadler · 2018-04-05T18:26:20Z

Got it!

Thanks @NejcZdovc

jasonrsadler · 2018-04-06T17:24:19Z

app/browser/api/ledgerNotifications.js

@@ -230,7 +230,7 @@ const showEnabledNotifications = (state) => {
      showReviewPublishers(now + ledgerUtil.milliseconds.day)
    }
  } else if (now - bootStamp > 7 * ledgerUtil.milliseconds.day) {
-    if (noBackupKeys(state) && shouldShowNotificationBackupKeys()) {
+    if (noBackupKeys(state) && shouldShowNotificationBackupKeys() && hasFunds(state)) {


@mrose17, during our coding session, we created hasFunds but the linter picked it up as unused. My assumption was the notification should be shown based on the original criteria and also (added above) if the user has actual funds in the wallet. Can you confirm this? Thanks.

this is an interesting question: my thinking is that we don't need hasFunds, it's a good idea for folks to keep a balance, regardless. wdyt?

mrose17

thanks for including the security review issue. looks good to me. let's see what the security folks say.

mrose17 · 2018-04-09T23:51:53Z

app/browser/api/ledgerNotifications.js

@@ -230,7 +230,7 @@ const showEnabledNotifications = (state) => {
      showReviewPublishers(now + ledgerUtil.milliseconds.day)
    }
  } else if (now - bootStamp > 7 * ledgerUtil.milliseconds.day) {
-    if (noBackupKeys(state) && shouldShowNotificationBackupKeys()) {
+    if (noBackupKeys(state) && shouldShowNotificationBackupKeys() && hasFunds(state)) {


this is an interesting question: my thinking is that we don't need hasFunds, it's a good idea for folks to keep a balance, regardless. wdyt?

jasonrsadler · 2018-04-11T03:11:36Z

"this is an interesting question: my thinking is that we don't need hasFunds, it's a good idea for folks to keep a balance, regardless. wdyt?"

I could go either way. My initial thinking:
The user has downloaded and is trying the browser out. 7 days goes by and they've played around with it. Enabled payments and all that good stuff. But they haven't added any funds. They get a notification to backup their wallet with no funds in it and they say "Sure, I'll do it. Just don't remind me again. There's really nothing of value (yet) to backup". So later, they decide they want to add funds to go out and completely forgot about backing up the wallet because it just didn't seem that important at the time. Now that they have funds, they may take it more seriously if they get the notification only after they've put funds into it.

mrose17 · 2018-04-11T03:12:52Z

@jasonrsadler - agreed. why not DM mandar and get his take on it?

jasonrsadler · 2018-04-11T14:41:51Z

Per @mandar-brave - if the user has never added funds then don't show. Once the user has funds or has a zero balance but has added funds in the past then show.
Also suggested frequency from payments enable and user clicking to remind later: 7 days, 14 days, then once a month from that point until backup or opt out.

The question from the linked issue:
For existing users, since we have no way of knowing whether a backup has occurred or not, show it anyway and allow for opt out.

mrose17 · 2018-04-11T14:42:18Z

great! please make it so!

diracdeltas · 2018-04-11T16:55:28Z

can we make sure brave-intl/bat-client#52 gets merged before this so that the backup notification shows the new code words? otherwise users may see some offensive words.

Dismissing approval, per Jason's comment in https://github.com/brave/internal/issues/266#issue-313154893

jasonrsadler · 2018-04-11T17:28:49Z

@diracdeltas @NejcZdovc @mrose17 Blocked on brave-intl/bat-client#52

jasonrsadler · 2018-04-13T15:42:41Z

@NejcZdovc, is ledger.js -> onWalletProperties a good place to set a flag to tell us if the user has ever funded the wallet?

diracdeltas · 2018-04-13T17:37:18Z

js/constants/appConfig.js

+    'payments.notification-backup-keys-count': 0, // number of times user has been notified to backup wallet
+    // the intervals in days that the user should be reminded to backup wallet
+    // if an out of bounds index is selected, it should default to monthly
+    'payments.notification-backup-keys-interval': [7, 14],


the settings / defaultSettings object is supposed to only be used for settings that are exposed in about:preferences such that the user can modify them.

can we avoid adding more non-user-exposed payments flags/timestamps to settings? unless i'm misunderstanding why they were added here in the first place.

seems like they should go in appState.ledger.about or appState.ledger.info (https://github.com/brave/browser-laptop/pull/13504/files#diff-b4e400e260f324745d12616a8c9c1fc6R185)

I wondered about that myself. @mrose17 or @NejcZdovc would have to reply on that but I can move them.

yes, let's move this into the state

Changes were made

jasonrsadler · 2018-04-14T02:22:25Z

@diracdeltas, would you give this another look?

NejcZdovc · 2018-04-25T05:03:38Z

app/browser/api/ledgerNotifications.js

+      state = aboutPreferencesState.setPreferencesProp(state, 'backupNotifyTimestamp', nextTime)
+      module.exports.showBackupKeys(nextTime)
+    }
+    if (process.env.LEDGER_NOTIFICATION) {


process.env.LEDGER_NOTIFICATION -> process.env.LEDGER_VERBOSE

Are you sure you want to put this under verbose? It would be nice if QA saw just the time passed rather than filter through all the other messages. Also, when run in the future, you'll get the time thrown out to the console every 5 seconds.

that's a good point. Let's keep it like it is now

NejcZdovc · 2018-04-25T05:03:51Z

app/browser/api/ledgerNotifications.js

@@ -63,9 +72,12 @@ const init = () => {
 }

 const onInterval = (state) => {
+  if (process.env.LEDGER_NOTIFICATION) {


process.env.LEDGER_NOTIFICATION -> process.env.LEDGER_VERBOSE

same as above

NejcZdovc · 2018-04-25T05:05:31Z

js/actions/appActions.js

@@ -1967,6 +1967,13 @@ const appActions = {
      duration,
      revisited
    })
+  },
+
+  onLedgerBackupSuccess: function (success) {


Do we need to pass variable in, where in reducer we just set true and not use this variable?

NejcZdovc · 2018-04-25T05:06:31Z

app/common/state/ledgerState.js

@@ -360,6 +360,11 @@ const ledgerState = {
    return state.setIn(['about', 'preferences', 'updatedStamp'], date)
  },

+  getRecoveryStatus: (state) => {


Is this used?

bradleyrichter · 2018-04-25T21:52:57Z

@jasonrsadler WDYT?

[Turn Off Notifications] [Later] [Back Up Now]

jasonrsadler · 2018-04-25T22:03:29Z

@bradleyrichter I'm good with that. Thanks.

jasonrsadler · 2018-04-25T22:12:33Z

NejcZdovc · 2018-04-26T04:09:50Z

app/renderer/components/preferences/payment/ledgerBackup.js


 class LedgerBackupContent extends ImmutableComponent {
  copyToClipboard (text) {
    aboutActions.setClipboard(text)
+    appActions.onLedgerBackupSuccess(true)


@jasonrsadler this is the only thing that I found, so I will just fix it and then I will merge this PR

WIP -- ensure backup status is assigned to the state WIP -- merging sync Added appAction to show backup keys Updated notifications text Added condition for backup notification only if the user has funds WIP -- Notification tests WIP -- notification tests wip WIP -- wip on tests WIP -- added constants to appConfig and completed unittests Fixed linting Removed notification trigger from debug menu WIP -- Updating wallet backup notification per criteria Added tests. Removed redundant function. spacing clean up Moved payments settings from defaultSettings to payments field Created aboutPreferencesState and moved ledger functions. Added Preferences Prop helpers to aboutPreferencesState Moved configs to state. bumped bat-client version merge fix Updated state.md Remove redundant function and usage Removed notification interval from state. minor code corrections lint Updated for null checks and updated tests Modifications for testing Removed unused methods Fixed spacing and reverted launch.json. Implemented process var for test Updating for test plan. Corrected backup and recovery timestamp writes Set clipboard action backup to set backup flag Updated to use updatedStamp vice recoverySucceeded lint Updated tests to reflect state passing Updated text for notification Updated QA interval lint Adding context menu option to bookmark/history entries to add their respective sites to the ledger Changes per CR, hiding context menu item when not applicable Exposing addSiteVisit and shouldTrackTab for unit testing, add initial manual addition unit tests Adding more tests for addSiteVisit/addNewLocation under manual addition Updated text for dismiss button on notification

NejcZdovc

++ code looks good and works great. Thank you

Implemented notification for user to backup Brave wallet

NejcZdovc · 2018-04-26T04:52:01Z

master a2256bc
0.23 4942965
0.22 99711a9

bsclifton · 2018-05-15T17:32:40Z

Will work on reverting this- we decided to not include this in the 0.22.x-release3 build

@jasonrsadler

This reverts commit 99711a9. Auditors: @jasonrsadler

@jasonrsadler

This reverts commit 99711a9. Auditors: @jasonrsadler

bsclifton · 2018-05-15T18:33:10Z

Reverted:
0.22.x-release3 b74e887
0.23.x f495fd4
master - will need to PR

…p-remind"" This reverts commit b74e887.

bsclifton · 2018-05-15T20:56:18Z

Reverted the reverts after discussion and closed the PR to master which had the revert

NejcZdovc previously requested changes Mar 19, 2018

View reviewed changes

NejcZdovc self-requested a review April 4, 2018 10:44

NejcZdovc assigned jasonrsadler Apr 4, 2018

NejcZdovc added the feature/rewards label Apr 4, 2018

jasonrsadler commented Apr 6, 2018

View reviewed changes

NejcZdovc mentioned this pull request Apr 9, 2018

Add alert dialog for wallet backup invitation #4847

Closed

jasonrsadler changed the title ~~WIP -- issue #13425 -- TODO: appAction to bring up advanced settings~~ Implemented notification for user to backup Brave wallet Apr 11, 2018

jasonrsadler requested review from mrose17 and diracdeltas April 11, 2018 02:36

jasonrsadler added the PR/pending-review label Apr 11, 2018

mrose17 previously approved these changes Apr 11, 2018

View reviewed changes

jasonrsadler added the PR/blocked label Apr 11, 2018

diracdeltas reviewed Apr 13, 2018

View reviewed changes

NejcZdovc reviewed Apr 25, 2018

View reviewed changes

NejcZdovc requested review from ryanml and NejcZdovc April 25, 2018 05:29

NejcZdovc reviewed Apr 26, 2018

View reviewed changes

NejcZdovc approved these changes Apr 26, 2018

View reviewed changes

NejcZdovc merged commit a2256bc into brave:master Apr 26, 2018

NejcZdovc added a commit that referenced this pull request Apr 26, 2018

Merge pull request #13504 from jasonrsadler/gen-backup-remind

4942965

Implemented notification for user to backup Brave wallet

NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 26, 2018

NejcZdovc removed the PR/pending-review label Apr 26, 2018

NejcZdovc added a commit that referenced this pull request Apr 26, 2018

Merge pull request #13504 from jasonrsadler/gen-backup-remind

99711a9

Implemented notification for user to backup Brave wallet

jasonrsadler deleted the gen-backup-remind branch April 26, 2018 10:08

mrose17 mentioned this pull request May 15, 2018

Remind the user to generate a backup file #13425

Closed

bsclifton removed this from the 0.22.x Release 3 (Beta channel) milestone May 15, 2018

bsclifton added a commit that referenced this pull request May 15, 2018

Revert "Merge pull request #13504 from jasonrsadler/gen-backup-remind"

b74e887

This reverts commit 99711a9. Auditors: @jasonrsadler

bsclifton added a commit that referenced this pull request May 15, 2018

Revert "Merge pull request #13504 from jasonrsadler/gen-backup-remind"

f495fd4

This reverts commit 99711a9. Auditors: @jasonrsadler

bsclifton mentioned this pull request May 15, 2018

Revert "Merge pull request #13504 from jasonrsadler/gen-backup-remind" #14139

Closed

bsclifton added this to the 0.22.x Release 3 (Beta channel) milestone May 15, 2018

bsclifton added a commit that referenced this pull request May 15, 2018

Revert "Revert "Merge pull request #13504 from jasonrsadler/gen-backu…

96e7390

…p-remind"" This reverts commit b74e887.

LaurenWags mentioned this pull request Sep 17, 2018

implement notification/prompt to user to backup their wallet brave/brave-browser#1164

Closed

Implemented notification for user to backup Brave wallet #13504

Implemented notification for user to backup Brave wallet #13504

Conversation

jasonrsadler commented Mar 19, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

codecov-io commented Mar 19, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler commented Apr 1, 2018

NejcZdovc commented Apr 4, 2018

NejcZdovc commented Apr 4, 2018 • edited Loading

jasonrsadler commented Apr 5, 2018

NejcZdovc commented Apr 5, 2018

jasonrsadler commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrose17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler commented Apr 11, 2018 • edited Loading

mrose17 commented Apr 11, 2018

jasonrsadler commented Apr 11, 2018 • edited Loading

mrose17 commented Apr 11, 2018

diracdeltas commented Apr 11, 2018

jasonrsadler commented Apr 11, 2018 • edited Loading

jasonrsadler commented Apr 13, 2018

diracdeltas Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

jasonrsadler Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradleyrichter commented Apr 25, 2018

jasonrsadler commented Apr 25, 2018

jasonrsadler commented Apr 25, 2018

Choose a reason for hiding this comment

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 26, 2018

bsclifton commented May 15, 2018

bsclifton commented May 15, 2018

bsclifton commented May 15, 2018

jasonrsadler commented Mar 19, 2018 •

edited

Loading

codecov-io commented Mar 19, 2018 •

edited

Loading

NejcZdovc commented Apr 4, 2018 •

edited

Loading

jasonrsadler commented Apr 11, 2018 •

edited

Loading

jasonrsadler commented Apr 11, 2018 •

edited

Loading

jasonrsadler commented Apr 11, 2018 •

edited

Loading

diracdeltas Apr 13, 2018 •

edited

Loading

jasonrsadler Apr 13, 2018 •

edited

Loading

NejcZdovc Apr 25, 2018 •

edited

Loading

NejcZdovc Apr 25, 2018 •

edited

Loading