From 5337bf6413d09aa11af27072f026e3e101611833 Mon Sep 17 00:00:00 2001 From: Jaco Greeff Date: Wed, 22 Feb 2017 10:43:02 +0100 Subject: [PATCH] Default account selection update (#4609) * Default accounts setting - account provider * RPC support for default accounts * Updating JS code * Rename whitelist to addresses * Set the defaults using default, allowing for null/full lists * Update failing tests (after merge) * Fix merge with wrong rpc call names * One account needs to be selected --- js/src/modals/DappPermissions/store.js | 51 ++++++++++++--------- js/src/modals/DappPermissions/store.spec.js | 20 ++++++-- js/src/views/ParityBar/accountStore.js | 13 ++---- js/src/views/ParityBar/accountStore.spec.js | 8 ++-- js/src/views/ParityBar/parityBar.test.js | 3 +- 5 files changed, 53 insertions(+), 42 deletions(-) diff --git a/js/src/modals/DappPermissions/store.js b/js/src/modals/DappPermissions/store.js index 3239343c5..0476879a5 100644 --- a/js/src/modals/DappPermissions/store.js +++ b/js/src/modals/DappPermissions/store.js @@ -20,6 +20,7 @@ export default class Store { @observable accounts = []; @observable modalOpen = false; @observable whitelist = []; + @observable whitelistDefault = null; constructor (api) { this._api = api; @@ -29,17 +30,14 @@ export default class Store { @action closeModal = () => { transaction(() => { - let addresses = null; const checkedAccounts = this.accounts.filter((account) => account.checked); - - if (checkedAccounts.length) { - addresses = checkedAccounts.filter((account) => account.default) - .concat(checkedAccounts.filter((account) => !account.default)) - .map((account) => account.address); - } + const defaultAddress = (this.accounts.find((account) => account.default) || {}).address; + const addresses = checkedAccounts.length === this.accounts.length + ? null + : checkedAccounts.map((account) => account.address); this.modalOpen = false; - this.updateWhitelist(addresses); + this.updateWhitelist(addresses, defaultAddress); }); } @@ -53,8 +51,8 @@ export default class Store { checked: this.whitelist ? this.whitelist.includes(account.address) : true, - default: this.whitelist - ? this.whitelist[0] === account.address + default: this.whitelistDefault + ? this.whitelistDefault === account.address : index === 0, description: account.meta.description, name: account.name @@ -66,8 +64,10 @@ export default class Store { @action selectAccount = (address) => { transaction(() => { + const isSingleAccount = this.accounts.filter((account) => account.checked).length === 1; + this.accounts = this.accounts.map((account) => { - if (account.address === address) { + if (account.address === address && (!isSingleAccount || !account.checked)) { account.checked = !account.checked; account.default = false; } @@ -96,26 +96,35 @@ export default class Store { }); } - @action setWhitelist = (whitelist) => { - this.whitelist = whitelist; + @action setWhitelist = (whitelist, whitelistDefault) => { + transaction(() => { + this.whitelist = whitelist; + this.whitelistDefault = whitelistDefault; + }); } loadWhitelist () { - return this._api.parity - .getNewDappsAddresses() - .then((whitelist) => { - this.setWhitelist(whitelist); + return Promise + .all([ + this._api.parity.getNewDappsAddresses(), + this._api.parity.getNewDappsDefaultAddress() + ]) + .then(([whitelist, whitelistDefault]) => { + this.setWhitelist(whitelist, whitelistDefault); }) .catch((error) => { console.warn('loadWhitelist', error); }); } - updateWhitelist (whitelist) { - return this._api.parity - .setNewDappsAddresses(whitelist) + updateWhitelist (whitelist, whitelistDefault = null) { + return Promise + .all([ + this._api.parity.setNewDappsAddresses(whitelist), + this._api.parity.setNewDappsDefaultAddress(whitelistDefault) + ]) .then(() => { - this.setWhitelist(whitelist); + this.setWhitelist(whitelist, whitelistDefault); }) .catch((error) => { console.warn('updateWhitelist', error); diff --git a/js/src/modals/DappPermissions/store.spec.js b/js/src/modals/DappPermissions/store.spec.js index 20d927484..800c8a315 100644 --- a/js/src/modals/DappPermissions/store.spec.js +++ b/js/src/modals/DappPermissions/store.spec.js @@ -32,14 +32,16 @@ function create () { api = { parity: { getNewDappsAddresses: sinon.stub().resolves(WHITELIST), - setNewDappsAddresses: sinon.stub().resolves(true) + getNewDappsDefaultAddress: sinon.stub().resolves(WHITELIST[0]), + setNewDappsAddresses: sinon.stub().resolves(true), + setNewDappsDefaultAddress: sinon.stub().resolves(true) } }; store = new Store(api); } -describe('modals/DappPermissions/store', () => { +describe.only('modals/DappPermissions/store', () => { beforeEach(() => { create(); }); @@ -80,11 +82,11 @@ describe('modals/DappPermissions/store', () => { }); it('calls setNewDappsAddresses', () => { - expect(api.parity.setNewDappsAddresses).to.have.been.calledOnce; + expect(api.parity.setNewDappsAddresses).to.have.been.calledWith(['456', '789']); }); - it('has the default account in first position', () => { - expect(api.parity.setNewDappsAddresses).to.have.been.calledWith(['789', '456']); + it('calls into setNewDappsDefaultAddress', () => { + expect(api.parity.setNewDappsDefaultAddress).to.have.been.calledWith('789'); }); }); @@ -107,6 +109,14 @@ describe('modals/DappPermissions/store', () => { expect(store.accounts.find((account) => account.address === '456').default).to.be.false; expect(store.accounts.find((account) => account.address === '123').default).to.be.true; }); + + it('does not deselect the last account', () => { + store.selectAccount('123'); + store.selectAccount('456'); + console.log(store.accounts.map((account) => ({ address: account.address, checked: account.checked }))); + expect(store.accounts.find((account) => account.address === '456').default).to.be.true; + expect(store.accounts.find((account) => account.address === '456').checked).to.be.true; + }); }); describe('setDefaultAccount', () => { diff --git a/js/src/views/ParityBar/accountStore.js b/js/src/views/ParityBar/accountStore.js index fef00a142..4c2736864 100644 --- a/js/src/views/ParityBar/accountStore.js +++ b/js/src/views/ParityBar/accountStore.js @@ -50,18 +50,11 @@ export default class AccountStore { this.isLoading = isLoading; } - makeDefaultAccount = (address) => { - const accounts = [address].concat( - this.accounts - .filter((account) => account.address !== address) - .map((account) => account.address) - ); - - // Have optimistic UI: https://www.smashingmagazine.com/2016/11/true-lies-of-optimistic-user-interfaces/?utm_source=codropscollective - this.setDefaultAccount(address); + makeDefaultAccount = (defaultAddress) => { + this.setDefaultAccount(defaultAddress); return this._api.parity - .setNewDappsAddresses(accounts) + .setNewDappsDefaultAddress(defaultAddress) .catch((error) => { console.warn('makeDefaultAccount', error); }); diff --git a/js/src/views/ParityBar/accountStore.spec.js b/js/src/views/ParityBar/accountStore.spec.js index 8b2a9a41a..16cebf9e1 100644 --- a/js/src/views/ParityBar/accountStore.spec.js +++ b/js/src/views/ParityBar/accountStore.spec.js @@ -18,7 +18,7 @@ import sinon from 'sinon'; import AccountStore from './accountStore'; -import { ACCOUNT_DEFAULT, ACCOUNT_FIRST, ACCOUNT_NEW, createApi } from './parityBar.test.js'; +import { ACCOUNT_DEFAULT, ACCOUNT_NEW, createApi } from './parityBar.test.js'; let api; let store; @@ -104,10 +104,8 @@ describe('views/ParityBar/AccountStore', () => { return store.makeDefaultAccount(ACCOUNT_NEW); }); - it('calls into parity_setNewDappsAddresses (with ordering)', () => { - expect(api.parity.setNewDappsAddresses).to.have.been.calledWith([ - ACCOUNT_NEW, ACCOUNT_FIRST, ACCOUNT_DEFAULT - ]); + it('calls into parity_setNewDappsDefaultAddress', () => { + expect(api.parity.setNewDappsDefaultAddress).to.have.been.calledWith(ACCOUNT_NEW); }); }); }); diff --git a/js/src/views/ParityBar/parityBar.test.js b/js/src/views/ParityBar/parityBar.test.js index 97c6e6251..f390424e1 100644 --- a/js/src/views/ParityBar/parityBar.test.js +++ b/js/src/views/ParityBar/parityBar.test.js @@ -37,7 +37,8 @@ function createApi () { defaultAccount: sinon.stub().resolves(ACCOUNT_DEFAULT), allAccountsInfo: sinon.stub().resolves(ACCOUNTS), getNewDappsAddresses: sinon.stub().resolves(null), - setNewDappsAddresses: sinon.stub().resolves(true) + setNewDappsAddresses: sinon.stub().resolves(true), + setNewDappsDefaultAddress: sinon.stub().resolves(true) } };