Set transition back to false when not needed to avoid being stuck in limbo #11763

bsclifton · 2017-11-02T04:56:14Z

This commit also fixes a leak that the tests had (which was due to tests running and including bat-publisher).

Fixes #11566
Fixes #11757

Auditors: @NejcZdovc, @bbondy

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.

Test Plan:

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

bsclifton · 2017-11-02T04:56:39Z

app/browser/api/ledger.js

-const checkBtcBatMigrated = (state, status) => {
-  if (!status) {
+const checkBtcBatMigrated = (state, paymentsEnabled) => {
+  if (!paymentsEnabled) {


renamed the variable to be more meaningful (and also match other places in the file)

bsclifton · 2017-11-02T04:57:48Z

app/browser/api/ledger.js

@@ -2373,6 +2373,8 @@ const checkBtcBatMigrated = (state, status) => {
  if (!isNewInstall && !hasUpgradedWallet) {
    state = migrationState.setTransitionStatus(state, true)
    module.exports.transitionWalletToBat()
+  } else {
+    state = migrationState.setTransitionStatus(state, false)


This is the big bug fix- if it's an existing install and hasn't upgraded yet, it needs to start (or in the case where you quit, restart) the transition process. Otherwise, it should hide the transition because that is no longer valid

bsclifton · 2017-11-02T04:58:22Z

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

@@ -170,7 +171,7 @@ class EnabledContent extends ImmutableComponent {
  render () {
    const ledgerData = this.props.ledgerData
    const walletStatusText = walletStatus(ledgerData)
-    const inTransition = ledgerData.getIn(['migration', 'btc2BatTransitionPending']) === true
+    const inTransition = migrationState.inTransition(ledgerData)


simple cleanup to put in helper method

HOWEVER... notice that it says migration instead of migrations. This was a bug I caught with the unit tests. I'm not sure how this ever returned true?

This is failing for me and only see disabled screen on payments when upgrading from 0.18.x. This is the error:

(BLESSED_EXTENSION context for mnojpmjdmbbfmejpflffifhffcmidifd) http://localhost:8080/gen/aboutPages.entry.js:17915: Uncaught AssertionError: state must contain an Immutable.Map of migrations{AssertionError: state must contain an Immutable.Map of migrations at validateState (http://localhost:8080/gen/aboutPages.entry.js:173611:10) at Object.inTransition (http://localhost:8080/gen/aboutPages.entry.js:173626:13) at EnabledContent.render (http://localhost:8080/gen/aboutPages.entry.js:102839:41) at http://localhost:8080/gen/aboutPages.entry.js:78075:21 at measureLifeCyclePerf (http://localhost:8080/gen/aboutPages.entry.js:77355:12) at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (http://localhost:8080/gen/aboutPages.entry.js:78074:25) at ReactCompositeComponentWrapper._renderValidatedComponent (http://localhost:8080/gen/aboutPages.entry.js:78101:32) at ReactCompositeComponentWrapper.performInitialMount (http://localhost:8080/gen/aboutPages.entry.js:77641:30) at ReactCompositeComponentWrapper.mountComponent (http://localhost:8080/gen/aboutPages.entry.js:77537:21) at Object.mountComponent (http://localhost:8080/gen/aboutPages.entry.js:25250:35)}

This is working because this is not state but ledgerData which is generated here

browser-laptop/app/browser/tabs.js

Line 243 in f52a109

.set('migration', migration)

We can't use state helper here

OK I can fix this- we are OK to use the state helper because the path is identical, but it might be sloppy to do so (in case things change). The problem in our case is the validation that is done, which throws the error.

I'll change it back to the hardcoded path since the state helper shouldn't be used to evaluate ledgerData (even if the path matches)

wait- nevermind, I see the problem... fixing

bsclifton · 2017-11-02T05:00:23Z

test/unit/app/browser/api/ledgerTest.js

+      },
+      Synopsis: batPublisher.Synopsis
+    }
+    mockery.registerMock('bat-publisher', lp)


This is basically what fixes the tests. Without bat-publisher being mocked, I was getting the leak error on my machine

bsclifton · 2017-11-02T05:14:21Z

Marking as work-in-progress... the tests are having another issue now

bsclifton · 2017-11-02T05:40:45Z

removing WIP label- I confirmed this does fix the unit tests 😄 👍
https://travis-ci.org/brave/browser-laptop/jobs/296108034

Ready for review! 😄

codecov-io · 2017-11-02T05:54:43Z

Codecov Report

Merging #11763 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #11763      +/-   ##
==========================================
- Coverage   52.76%   52.61%   -0.15%     
==========================================
  Files         271      271              
  Lines       25713    25715       +2     
  Branches     4104     4104              
==========================================
- Hits        13567    13530      -37     
- Misses      12146    12185      +39

Flag	Coverage Δ
#unittest	`52.61% <100%> (-0.15%)`	⬇️

Impacted Files	Coverage Δ
app/common/state/migrationState.js	`93.87% <100%> (+0.54%)`	⬆️
app/browser/api/ledger.js	`38.1% <100%> (-1.67%)`	⬇️
js/stores/appStoreRenderer.js	`91.17% <0%> (-8.83%)`	⬇️
app/renderer/components/reduxComponent.js	`84.37% <0%> (-6.25%)`	⬇️
js/stores/windowStore.js	`26.71% <0%> (-0.3%)`	⬇️

NejcZdovc

Just tested out PR and transition overlay is not triggered for me when I upgrade from 0.18 wallet. Probably because of that state error

bsclifton · 2017-11-06T16:28:09Z

@NejcZdovc fixed, ready for re-review

@NejcZdovc

…limbo This commit also fixes a leak that the tests had (which was due to tests running and including bat-publisher). Fixes #11566 Fixes #11757 Auditors: @NejcZdovc, @bbondy

NejcZdovc

++

Set transition back to false when not needed to avoid being stuck in limbo

NejcZdovc · 2017-11-06T18:39:49Z

master 88974f2
0.21 e81dfce
0.20 5af92b4
0.19 54fecec

bsclifton added this to the 0.19.x Hotfix 4 (Release channel) milestone Nov 2, 2017

bsclifton self-assigned this Nov 2, 2017

bsclifton requested review from bbondy and NejcZdovc November 2, 2017 04:56

bsclifton commented Nov 2, 2017

View reviewed changes

bsclifton added the PR/work-in-progress ⚒ label Nov 2, 2017

bsclifton removed the PR/work-in-progress ⚒ label Nov 2, 2017

bsclifton added the feature/rewards label Nov 2, 2017

NejcZdovc suggested changes Nov 6, 2017

View reviewed changes

Set transition back to false when not needed to avoid being stuck in …

d5298e0

…limbo This commit also fixes a leak that the tests had (which was due to tests running and including bat-publisher). Fixes #11566 Fixes #11757 Auditors: @NejcZdovc, @bbondy

NejcZdovc approved these changes Nov 6, 2017

View reviewed changes

NejcZdovc merged commit 88974f2 into brave:master Nov 6, 2017

NejcZdovc added a commit that referenced this pull request Nov 6, 2017

Merge pull request #11763 from bsclifton/ledger-bad-state-fix

5af92b4

Set transition back to false when not needed to avoid being stuck in limbo

NejcZdovc added a commit that referenced this pull request Nov 6, 2017

Merge pull request #11763 from bsclifton/ledger-bad-state-fix

e81dfce

Set transition back to false when not needed to avoid being stuck in limbo

NejcZdovc added a commit that referenced this pull request Nov 6, 2017

Merge pull request #11763 from bsclifton/ledger-bad-state-fix

54fecec

Set transition back to false when not needed to avoid being stuck in limbo

bbondy modified the milestones: 0.19.x Hotfix 5 (Release channel), 0.19.x Hotfix 4 (Release channel) Nov 8, 2017

bsclifton deleted the ledger-bad-state-fix branch November 22, 2017 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set transition back to false when not needed to avoid being stuck in limbo #11763

Set transition back to false when not needed to avoid being stuck in limbo #11763

bsclifton commented Nov 2, 2017

bsclifton Nov 2, 2017

bsclifton Nov 2, 2017

bsclifton Nov 2, 2017

bsclifton Nov 2, 2017

NejcZdovc Nov 6, 2017

NejcZdovc Nov 6, 2017 •

edited

Loading

bsclifton Nov 6, 2017

bsclifton Nov 6, 2017

bsclifton Nov 2, 2017

bsclifton commented Nov 2, 2017

bsclifton commented Nov 2, 2017 •

edited

Loading

codecov-io commented Nov 2, 2017 •

edited

Loading

NejcZdovc left a comment

bsclifton commented Nov 6, 2017

NejcZdovc left a comment

NejcZdovc commented Nov 6, 2017

Set transition back to false when not needed to avoid being stuck in limbo #11763

Set transition back to false when not needed to avoid being stuck in limbo #11763

Conversation

bsclifton commented Nov 2, 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

NejcZdovc Nov 6, 2017 • edited Loading

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

bsclifton commented Nov 2, 2017

bsclifton commented Nov 2, 2017 • edited Loading

codecov-io commented Nov 2, 2017 • edited Loading

Codecov Report

NejcZdovc left a comment

Choose a reason for hiding this comment

bsclifton commented Nov 6, 2017

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Nov 6, 2017

NejcZdovc Nov 6, 2017 •

edited

Loading

bsclifton commented Nov 2, 2017 •

edited

Loading

codecov-io commented Nov 2, 2017 •

edited

Loading