Adds transition for disabled wallets #11626

NejcZdovc · 2017-10-23T06:24:53Z

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.

Resolves #11611
Resolves #11614

Auditors:

Plan A

launch 0.18.36 and enable payments via about:preferences#payments
visit several websites and ensure they're appearing under the ledger table
disable payments via the "On/Off" switch under about:preferences#payments
switch to this branch
go back into about:preferences#payments and enable payments
ledger table should be the same as it was on 0.18.36

Plan B

launch 0.18.36 and enable payments via about:preferences#payments
disable payments via the "On/Off" switch under about:preferences#payments
switch to this branch
go back into about:preferences#payments and enable payments
transition overlay should be triggered

Plan C

go to preferences and try to toggle payments on/off multiple times
there shouldn't be any errors in the log
wallet should be created when you leave it on

Plan D

launch 0.18.36 and enable payments via about:preferences#payments
visit several websites and ensure they're appearing under the ledger table
disable payments via the "On/Off" switch under about:preferences#payments
switch to this branch
go to preferences and try to toggle payments on/off multiple times
wallet should be converted when you leave it on

Reviewer Checklist:

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

NejcZdovc · 2017-10-23T08:29:40Z

when this is merged and new build is done, we need to check if we didn't regressed in #11506

bbondy · 2017-10-23T13:23:12Z

app/browser/api/ledger.js

@@ -2212,7 +2226,7 @@ const migration = (state) => {
        }
      })
    } catch (err) {
-      console.log('Error migrating file', err.toString())
+      console.log(err.toString())


nit console.error here so it has callstack too

will check the whole ledger.js file and replace it where needed

bbondy · 2017-10-23T13:27:55Z

app/browser/api/ledger.js

@@ -2327,7 +2364,7 @@ const transitionWalletToBat = () => {

  try {
    client.transition(newPaymentId, (err, properties) => {
-      if (err) {
+      if (err || newClient == null) {


I think you want !newClient here because otherwise newClient's false could be used in the else.

bbondy · 2017-10-23T13:32:14Z

app/common/state/migrationState.js

+
+  hasBeenNotified: (state) => {
+    state = validateState(state)
+    return state.getIn(['migrations', 'batMercuryTimestamp']) !== state.getIn(['migrations', 'btc2BatNotifiedTimestamp'])


Pls add comment here that this only happens on initial state, I think that's the case from reading the code.

bbondy · 2017-10-23T13:32:17Z

app/common/state/migrationState.js

+
+  hasUpgradedWallet: (state) => {
+    state = validateState(state)
+    return state.getIn(['migrations', 'batMercuryTimestamp']) !== state.getIn(['migrations', 'btc2BatTimestamp'])


Pls add comment here that this only happens on initial state, I think that's the case from reading the code.

Resolves brave#11611 Auditors: Test Plan:

Resolves brave#11614 Auditors: Test Plan:

codecov-io · 2017-10-23T20:08:19Z

Codecov Report

Merging #11626 into master will increase coverage by 0.07%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master   #11626      +/-   ##
==========================================
+ Coverage   52.26%   52.33%   +0.07%     
==========================================
  Files         268      269       +1     
  Lines       25311    25377      +66     
  Branches     4034     4042       +8     
==========================================
+ Hits        13228    13281      +53     
- Misses      12083    12096      +13

Flag	Coverage Δ
#unittest	`52.33% <76.47%> (+0.07%)`	⬆️

Impacted Files	Coverage Δ
app/browser/api/ledger.js	`31.3% <59.18%> (+0.29%)`	⬆️
app/browser/reducers/ledgerReducer.js	`43.37% <88.88%> (-0.4%)`	⬇️
app/common/state/migrationState.js	`93.33% <93.18%> (ø)`

Adds transition for disabled wallets

NejcZdovc · 2017-10-23T20:18:44Z

master 696ef66
0.21 bcedf7b
0.20 a1a88e1
0.19 d86f227

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Oct 23, 2017

NejcZdovc added this to the 0.19.x Hotfix 2 (Release channel) milestone Oct 23, 2017

NejcZdovc self-assigned this Oct 23, 2017

NejcZdovc force-pushed the hotfix/#11611-disabled branch from 8bedda4 to d037100 Compare October 23, 2017 07:49

NejcZdovc removed the PR/work-in-progress ⚒ label Oct 23, 2017

NejcZdovc requested a review from bbondy October 23, 2017 13:13

NejcZdovc mentioned this pull request Oct 23, 2017

Handle all bad state scenarios for the ledger #11566

Closed

3 tasks

bbondy reviewed Oct 23, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#11611-disabled branch from 37b6568 to 87bd992 Compare October 23, 2017 13:56

bbondy approved these changes Oct 23, 2017

View reviewed changes

NejcZdovc added 3 commits October 23, 2017 21:11

Adds transition for disabled wallets

3ec8d88

Resolves brave#11611 Auditors: Test Plan:

Fixes publishers migration with disabled wallet

f0538bf

Resolves brave#11614 Auditors: Test Plan:

Fixes overlay

9c8f3b5

NejcZdovc force-pushed the hotfix/#11611-disabled branch from 87bd992 to 9c8f3b5 Compare October 23, 2017 19:12

NejcZdovc merged commit 696ef66 into brave:master Oct 23, 2017

NejcZdovc added a commit that referenced this pull request Oct 23, 2017

Merge pull request #11626 from NejcZdovc/hotfix/#11611-disabled

bcedf7b

Adds transition for disabled wallets

NejcZdovc added a commit that referenced this pull request Oct 23, 2017

Merge pull request #11626 from NejcZdovc/hotfix/#11611-disabled

a1a88e1

Adds transition for disabled wallets

NejcZdovc added a commit that referenced this pull request Oct 23, 2017

Merge pull request #11626 from NejcZdovc/hotfix/#11611-disabled

d86f227

Adds transition for disabled wallets

This was referenced Oct 24, 2017

Publishers lost if upgrading from 0.18 with wallet disabled #11614

Closed

Transition when users wallet is disabled #11611

Closed

LaurenWags mentioned this pull request Oct 26, 2017

busyP errors when toggling payments off/on and leaving off #11684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds transition for disabled wallets #11626

Adds transition for disabled wallets #11626

NejcZdovc commented Oct 23, 2017 •

edited

Loading

NejcZdovc commented Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

codecov-io commented Oct 23, 2017 •

edited

Loading

NejcZdovc commented Oct 23, 2017

Adds transition for disabled wallets #11626

Adds transition for disabled wallets #11626

Conversation

NejcZdovc commented Oct 23, 2017 • edited Loading

NejcZdovc commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 23, 2017 • edited Loading

Codecov Report

NejcZdovc commented Oct 23, 2017

NejcZdovc commented Oct 23, 2017 •

edited

Loading

codecov-io commented Oct 23, 2017 •

edited

Loading