Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

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

Merged
merged 1 commit into from
Nov 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/browser/api/ledger.js
Original file line number Diff line number Diff line change
Expand Up @@ -2367,8 +2367,8 @@ const yoDawg = (stateState) => {
return stateState
}

const checkBtcBatMigrated = (state, status) => {
if (!status) {
const checkBtcBatMigrated = (state, paymentsEnabled) => {
if (!paymentsEnabled) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return state
}

Expand All @@ -2378,6 +2378,8 @@ const checkBtcBatMigrated = (state, status) => {
if (!isNewInstall && !hasUpgradedWallet) {
state = migrationState.setTransitionStatus(state, true)
module.exports.transitionWalletToBat()
} else {
state = migrationState.setTransitionStatus(state, false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return state
Expand Down Expand Up @@ -2513,6 +2515,7 @@ const getMethods = () => {
privateMethods = {
enable,
addVisit,
checkBtcBatMigrated,
clearVisitsByPublisher: function () {
visitsByPublisher = {}
},
Expand Down
5 changes: 5 additions & 0 deletions app/common/state/migrationState.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ const migrationState = {
return state.setIn(['migrations', 'btc2BatTransitionPending'], value)
},

inTransition: (state) => {
state = validateState(state)
return state.getIn(['migrations', 'btc2BatTransitionPending']) === true
},

setConversionTimestamp: (state, value) => {
state = validateState(state)
if (value == null) {
Expand Down
1 change: 1 addition & 0 deletions docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ AppStore
batMercuryTimestamp: integer, // when session is upgraded (and this new schema added)
btc2BatTimestamp: integer, // when call was made to backend to convert BTC => BAT
btc2BatNotifiedTimestamp: integer, // when user was shown "wallet upgraded" notification
btc2BatTransitionPending: boolean // true if user is being shown transition screen
},
menu: {
template: object // used on Windows and by our tests: template object with Menubar control
Expand Down
92 changes: 87 additions & 5 deletions test/unit/app/browser/api/ledgerTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const sinon = require('sinon')
const mockery = require('mockery')
const settings = require('../../../../../js/constants/settings')
const appActions = require('../../../../../js/actions/appActions')
const migrationState = require('../../../../../app/common/state/migrationState')
const batPublisher = require('bat-publisher')

const defaultAppState = Immutable.fromJS({
ledger: {},
Expand Down Expand Up @@ -135,6 +137,16 @@ describe('ledger api unit tests', function () {
ledgerClient.returns(lc)
mockery.registerMock('bat-client', ledgerClient)

// ledger publisher stubbing
const lp = {
ruleset: [],
getPublisherProps: function (publisher) {
return null
},
Synopsis: batPublisher.Synopsis
}
mockery.registerMock('bat-publisher', lp)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// once everything is stubbed, load the ledger
ledgerApi = require('../../../../../app/browser/api/ledger')
})
Expand All @@ -148,16 +160,16 @@ describe('ledger api unit tests', function () {
})

describe('initialize', function () {
let notificationsInitSpy
let notificationsInitStub
beforeEach(function () {
notificationsInitSpy = sinon.spy(ledgerApi.notifications, 'init')
notificationsInitStub = sinon.stub(ledgerApi.notifications, 'init')
})
afterEach(function () {
notificationsInitSpy.restore()
notificationsInitStub.restore()
})
it('calls notifications.init', function () {
ledgerApi.initialize(defaultAppState, true)
assert(notificationsInitSpy.calledOnce)
assert(notificationsInitStub.calledOnce)
})
})

Expand Down Expand Up @@ -277,8 +289,8 @@ describe('ledger api unit tests', function () {
ledgerApi.clearVisitsByPublisher()
})
afterEach(function () {
fakeClock.restore()
ledgerApi.setSynopsis(undefined)
fakeClock.restore()
})
it('records a visit when over the PAYMENTS_MINIMUM_VISIT_TIME threshold', function () {
const state = ledgerApi.initialize(stateWithLocation, true)
Expand Down Expand Up @@ -322,6 +334,76 @@ describe('ledger api unit tests', function () {
})
})

describe('checkBtcBatMigrated', function () {
let transitionWalletToBatStub
before(function () {
transitionWalletToBatStub = sinon.stub(ledgerApi, 'transitionWalletToBat')
})
after(function () {
transitionWalletToBatStub.restore()
})
describe('when not a new install and wallet has not been upgraded', function () {
let result
before(function () {
const notMigratedYet = defaultAppState.merge(Immutable.fromJS({
firstRunTimestamp: 12345,
migrations: {
batMercuryTimestamp: 34512,
btc2BatTimestamp: 34512,
btc2BatNotifiedTimestamp: 34512,
btc2BatTransitionPending: false
}
}))
assert.equal(migrationState.inTransition(notMigratedYet), false)
transitionWalletToBatStub.reset()
result = ledgerApi.checkBtcBatMigrated(notMigratedYet, true)
})
it('sets transition status to true', function () {
assert(migrationState.inTransition(result))
})
it('calls transitionWalletToBat', function () {
assert(transitionWalletToBatStub.calledOnce)
})
})

describe('when a transition is already being shown', function () {
it('sets transition to false if new install', function () {
const stuckOnMigrate = defaultAppState.merge(Immutable.fromJS({
firstRunTimestamp: 12345,
migrations: {
batMercuryTimestamp: 12345,
btc2BatTimestamp: 12345,
btc2BatNotifiedTimestamp: 12345,
btc2BatTransitionPending: true
}
}))
assert(migrationState.isNewInstall(stuckOnMigrate))
assert.equal(migrationState.hasUpgradedWallet(stuckOnMigrate), false)
assert(migrationState.inTransition(stuckOnMigrate))

const result = ledgerApi.checkBtcBatMigrated(stuckOnMigrate, true)
assert.equal(migrationState.inTransition(result), false)
})
it('sets transition to false if wallet has been upgraded', function () {
const stuckOnMigrate = defaultAppState.merge(Immutable.fromJS({
firstRunTimestamp: 12345,
migrations: {
batMercuryTimestamp: 34512,
btc2BatTimestamp: 54321,
btc2BatNotifiedTimestamp: 34512,
btc2BatTransitionPending: true
}
}))
assert.equal(migrationState.isNewInstall(stuckOnMigrate), false)
assert(migrationState.hasUpgradedWallet(stuckOnMigrate))
assert(migrationState.inTransition(stuckOnMigrate))

const result = ledgerApi.checkBtcBatMigrated(stuckOnMigrate, true)
assert.equal(migrationState.inTransition(result), false)
})
})
})

describe('transitionWalletToBat', function () {
describe('when client is not busy', function () {
before(function () {
Expand Down