From c43374a553c28459be4cac116a72a4f34dddba72 Mon Sep 17 00:00:00 2001 From: Thomas Huang Date: Wed, 20 Mar 2019 20:26:48 -0500 Subject: [PATCH 1/8] Clear notices when setCompletedOnboarding is called --- app/scripts/metamask-controller.js | 1 + app/scripts/notice-controller.js | 17 ++++++++++++ .../app/controllers/notice-controller-test.js | 26 +++++++++++++++++++ test/unit/ui/app/actions.spec.js | 24 +++++++++++++++++ ui/app/actions.js | 21 ++++++++++++--- 5 files changed, 85 insertions(+), 4 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 540aee936..ce59caf83 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -473,6 +473,7 @@ module.exports = class MetamaskController extends EventEmitter { // notices checkNotices: noticeController.updateNoticesList.bind(noticeController), markNoticeRead: noticeController.markNoticeRead.bind(noticeController), + markAllNoticesRead: noticeController.markAllNoticesRead.bind(noticeController), approveProviderRequest: providerApprovalController.approveProviderRequest.bind(providerApprovalController), clearApprovedOrigins: providerApprovalController.clearApprovedOrigins.bind(providerApprovalController), diff --git a/app/scripts/notice-controller.js b/app/scripts/notice-controller.js index 6fe8b8cf0..d1a15ff08 100644 --- a/app/scripts/notice-controller.js +++ b/app/scripts/notice-controller.js @@ -58,6 +58,23 @@ module.exports = class NoticeController extends EventEmitter { } } + markAllNoticesRead (cb) { + cb = cb || function (err) { if (err) throw err } + try { + const noticeList = this.getNoticesList() + noticeList.forEach(notice => { + notice.read = true + notice.body = '' + }) + this.setNoticesList(noticeList) + const latestNotice = this.getNextUnreadNotice() + cb(null, latestNotice) + } catch (err) { + cb(err) + } + } + + async updateNoticesList () { const newNotices = await this._retrieveNoticeData() const oldNotices = this.getNoticesList() diff --git a/test/unit/app/controllers/notice-controller-test.js b/test/unit/app/controllers/notice-controller-test.js index 834c88f7b..95e3ea9bd 100644 --- a/test/unit/app/controllers/notice-controller-test.js +++ b/test/unit/app/controllers/notice-controller-test.js @@ -39,6 +39,32 @@ describe('notice-controller', function () { }) }) + describe('#markAllNoticesRead', () => { + it('marks all notices read', (done) => { + const testList = [{ + id: 0, + read: false, + title: 'Notice 1', + }, { + id: 1, + read: false, + title: 'Notice 2', + }, { + id: 2, + read: false, + title: 'Notice 3', + }] + + noticeController.setNoticesList(testList) + + noticeController.markAllNoticesRead() + + const unreadNotices = noticeController.getUnreadNotices() + assert.equal(unreadNotices.length, 0) + done() + }) + }) + describe('#getNextUnreadNotice', function () { it('should retrieve the latest unread notice', function (done) { var testList = [ diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index 8d7de8b02..d27521088 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -1308,6 +1308,30 @@ describe('Actions', () => { }) }) + describe.only('#setCompletedOnboarding', () => { + let markAllNoticesReadSpy, completeOnboardingSpy + + beforeEach(() => { + markAllNoticesReadSpy = sinon.stub(background, 'markAllNoticesRead') + completeOnboardingSpy = sinon.stub(background, 'completeOnboarding') + }) + + after(() => { + markAllNoticesReadSpy.restore() + completeOnboardingSpy.restore() + }) + + it('', (done) => { + const store = mockStore() + store.dispatch(actions.setCompletedOnboarding()) + .then(() => { + assert.equal(markAllNoticesReadSpy.callCount, 1) + assert.equal(completeOnboardingSpy.callCount, 1) + }) + done() + }) + }) + describe('#updateNetworkNonce', () => { let getTransactionCountSpy diff --git a/ui/app/actions.js b/ui/app/actions.js index d8363eba6..65070fc8c 100644 --- a/ui/app/actions.js +++ b/ui/app/actions.js @@ -2487,16 +2487,29 @@ function setCompletedOnboarding () { return dispatch => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { - background.completeOnboarding(err => { - dispatch(actions.hideLoadingIndication()) + background.markAllNoticesRead(err => { if (err) { dispatch(actions.displayWarning(err.message)) return reject(err) } - dispatch(actions.completeOnboarding()) - resolve() + dispatch(actions.clearNotices()) + resolve(false) + }) + }) + .then(() => { + return new Promise((resolve, reject) => { + background.completeOnboarding(err => { + if (err) { + dispatch(actions.displayWarning(err.message)) + return reject(err) + } + + dispatch(actions.completeOnboarding()) + dispatch(actions.hideLoadingIndication()) + resolve() + }) }) }) } From 07c974525859eba81471ec7591821ea010addf5a Mon Sep 17 00:00:00 2001 From: Thomas Huang Date: Wed, 20 Mar 2019 21:29:08 -0500 Subject: [PATCH 2/8] Remove exclusive test --- test/unit/ui/app/actions.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index d27521088..bfc6459b1 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -1308,7 +1308,7 @@ describe('Actions', () => { }) }) - describe.only('#setCompletedOnboarding', () => { + describe('#setCompletedOnboarding', () => { let markAllNoticesReadSpy, completeOnboardingSpy beforeEach(() => { From a64855ef4940f1a409b4a168a60d0f5f06836086 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 29 Mar 2019 12:03:31 +0800 Subject: [PATCH 3/8] notices - markAllNoticesRead - use async/await --- app/scripts/metamask-controller.js | 2 +- app/scripts/notice-controller.js | 23 ++++++++----------- .../app/controllers/notice-controller-test.js | 5 ++-- test/unit/ui/app/actions.spec.js | 11 ++++----- 4 files changed, 16 insertions(+), 25 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index ce59caf83..058d527c0 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -473,7 +473,7 @@ module.exports = class MetamaskController extends EventEmitter { // notices checkNotices: noticeController.updateNoticesList.bind(noticeController), markNoticeRead: noticeController.markNoticeRead.bind(noticeController), - markAllNoticesRead: noticeController.markAllNoticesRead.bind(noticeController), + markAllNoticesRead: nodeify(noticeController.markAllNoticesRead, noticeController), approveProviderRequest: providerApprovalController.approveProviderRequest.bind(providerApprovalController), clearApprovedOrigins: providerApprovalController.clearApprovedOrigins.bind(providerApprovalController), diff --git a/app/scripts/notice-controller.js b/app/scripts/notice-controller.js index d1a15ff08..050fca9c8 100644 --- a/app/scripts/notice-controller.js +++ b/app/scripts/notice-controller.js @@ -58,20 +58,15 @@ module.exports = class NoticeController extends EventEmitter { } } - markAllNoticesRead (cb) { - cb = cb || function (err) { if (err) throw err } - try { - const noticeList = this.getNoticesList() - noticeList.forEach(notice => { - notice.read = true - notice.body = '' - }) - this.setNoticesList(noticeList) - const latestNotice = this.getNextUnreadNotice() - cb(null, latestNotice) - } catch (err) { - cb(err) - } + async markAllNoticesRead () { + const noticeList = this.getNoticesList() + noticeList.forEach(notice => { + notice.read = true + notice.body = '' + }) + this.setNoticesList(noticeList) + const latestNotice = this.getNextUnreadNotice() + return latestNotice } diff --git a/test/unit/app/controllers/notice-controller-test.js b/test/unit/app/controllers/notice-controller-test.js index 95e3ea9bd..d550d86cd 100644 --- a/test/unit/app/controllers/notice-controller-test.js +++ b/test/unit/app/controllers/notice-controller-test.js @@ -40,7 +40,7 @@ describe('notice-controller', function () { }) describe('#markAllNoticesRead', () => { - it('marks all notices read', (done) => { + it('marks all notices read', async () => { const testList = [{ id: 0, read: false, @@ -57,11 +57,10 @@ describe('notice-controller', function () { noticeController.setNoticesList(testList) - noticeController.markAllNoticesRead() + await noticeController.markAllNoticesRead() const unreadNotices = noticeController.getUnreadNotices() assert.equal(unreadNotices.length, 0) - done() }) }) diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index d395c9adb..a3b979d75 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -1321,14 +1321,11 @@ describe('Actions', () => { completeOnboardingSpy.restore() }) - it('', (done) => { + it('completing onboarding marks all notices as read', async () => { const store = mockStore() - store.dispatch(actions.setCompletedOnboarding()) - .then(() => { - assert.equal(markAllNoticesReadSpy.callCount, 1) - assert.equal(completeOnboardingSpy.callCount, 1) - }) - done() + await store.dispatch(actions.setCompletedOnboarding()) + assert.equal(markAllNoticesReadSpy.callCount, 1) + assert.equal(completeOnboardingSpy.callCount, 1) }) }) From e04f0c877ba94c126bbf94622640700c728f74c7 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 29 Mar 2019 12:37:40 +0800 Subject: [PATCH 4/8] lib - nodeify - correctly wrap synchronous functions --- app/scripts/lib/nodeify.js | 15 ++++++++--- test/unit/app/nodeify-test.js | 49 ++++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/app/scripts/lib/nodeify.js b/app/scripts/lib/nodeify.js index 25be6537b..a813ae679 100644 --- a/app/scripts/lib/nodeify.js +++ b/app/scripts/lib/nodeify.js @@ -1,5 +1,5 @@ const promiseToCallback = require('promise-to-callback') -const noop = function () {} +const callbackNoop = function (err) { if (err) throw err } /** * A generator that returns a function which, when passed a promise, can treat that promise as a node style callback. @@ -11,6 +11,7 @@ const noop = function () {} */ module.exports = function nodeify (fn, context) { return function () { + // parse arguments const args = [].slice.call(arguments) const lastArg = args[args.length - 1] const lastArgIsCallback = typeof lastArg === 'function' @@ -19,8 +20,16 @@ module.exports = function nodeify (fn, context) { callback = lastArg args.pop() } else { - callback = noop + callback = callbackNoop } - promiseToCallback(fn.apply(context, args))(callback) + // call the provided function and ensure result is a promise + let result + try { + result = Promise.resolve(fn.apply(context, args)) + } catch (err) { + result = Promise.reject(err) + } + // wire up promise resolution to callback + promiseToCallback(result)(callback) } } diff --git a/test/unit/app/nodeify-test.js b/test/unit/app/nodeify-test.js index 938b76c68..0dafc6653 100644 --- a/test/unit/app/nodeify-test.js +++ b/test/unit/app/nodeify-test.js @@ -2,16 +2,16 @@ const assert = require('assert') const nodeify = require('../../../app/scripts/lib/nodeify') describe('nodeify', function () { - var obj = { + const obj = { foo: 'bar', promiseFunc: function (a) { - var solution = this.foo + a + const solution = this.foo + a return Promise.resolve(solution) }, } it('should retain original context', function (done) { - var nodified = nodeify(obj.promiseFunc, obj) + const nodified = nodeify(obj.promiseFunc, obj) nodified('baz', function (err, res) { if (!err) { assert.equal(res, 'barbaz') @@ -22,7 +22,7 @@ describe('nodeify', function () { }) }) - it('should allow the last argument to not be a function', function (done) { + it('no callback - should allow the last argument to not be a function', function (done) { const nodified = nodeify(obj.promiseFunc, obj) try { nodified('baz') @@ -31,4 +31,45 @@ describe('nodeify', function () { done(new Error('should not have thrown if the last argument is not a function')) } }) + + it('no callback - should asyncly throw an error if underlying function does', function (done) { + const nodified = nodeify(async () => { throw new Error('boom!') }, obj) + process.prependOnceListener('uncaughtException', function (err) { + assert.ok(err, 'got expected error') + assert.ok(err.message.includes('boom!'), 'got expected error message') + done() + }) + try { + nodified('baz') + } catch (err) { + done(new Error('should not have thrown an error synchronously')) + } + }) + + it('sync functions - returns value', function (done) { + const nodified = nodeify(() => 42) + try { + nodified((err, result) => { + if (err) return done(new Error(`should not have thrown any error: ${err.message}`)) + assert.equal(42, result, 'got expected result') + }) + done() + } catch (err) { + done(new Error(`should not have thrown any error: ${err.message}`)) + } + }) + + it('sync functions - handles errors', function (done) { + const nodified = nodeify(() => { throw new Error('boom!') }) + try { + nodified((err, result) => { + if (result) return done(new Error('should not have returned any result')) + assert.ok(err, 'got expected error') + assert.ok(err.message.includes('boom!'), 'got expected error message') + }) + done() + } catch (err) { + done(new Error(`should not have thrown any error: ${err.message}`)) + } + }) }) From 846038a69c4cde10417ce54b280de3624d87d45c Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 29 Mar 2019 12:39:29 +0800 Subject: [PATCH 5/8] notice-controller - make markAllNoticesRead sync --- app/scripts/notice-controller.js | 2 +- test/unit/app/controllers/notice-controller-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/notice-controller.js b/app/scripts/notice-controller.js index 050fca9c8..63b422c5b 100644 --- a/app/scripts/notice-controller.js +++ b/app/scripts/notice-controller.js @@ -58,7 +58,7 @@ module.exports = class NoticeController extends EventEmitter { } } - async markAllNoticesRead () { + markAllNoticesRead () { const noticeList = this.getNoticesList() noticeList.forEach(notice => { notice.read = true diff --git a/test/unit/app/controllers/notice-controller-test.js b/test/unit/app/controllers/notice-controller-test.js index d550d86cd..caa50a03e 100644 --- a/test/unit/app/controllers/notice-controller-test.js +++ b/test/unit/app/controllers/notice-controller-test.js @@ -57,7 +57,7 @@ describe('notice-controller', function () { noticeController.setNoticesList(testList) - await noticeController.markAllNoticesRead() + noticeController.markAllNoticesRead() const unreadNotices = noticeController.getUnreadNotices() assert.equal(unreadNotices.length, 0) From edec6cb81d24dbe07aba53c772ff762da92399d1 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 29 Mar 2019 12:40:04 +0800 Subject: [PATCH 6/8] actions - setCompletedOnboarding - make async with pify --- ui/app/store/actions.js | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 785cadc3c..db3427ee6 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -2488,34 +2488,27 @@ function setShowFiatConversionOnTestnetsPreference (value) { } function setCompletedOnboarding () { - return dispatch => { + return async dispatch => { dispatch(actions.showLoadingIndication()) - return new Promise((resolve, reject) => { - background.markAllNoticesRead(err => { + + try { + await pify(background.markAllNoticesRead).call(background) + } catch (err) { + dispatch(actions.displayWarning(err.message)) + throw err + } - if (err) { - dispatch(actions.displayWarning(err.message)) - return reject(err) - } + dispatch(actions.clearNotices()) - dispatch(actions.clearNotices()) - resolve(false) - }) - }) - .then(() => { - return new Promise((resolve, reject) => { - background.completeOnboarding(err => { - if (err) { - dispatch(actions.displayWarning(err.message)) - return reject(err) - } + try { + await pify(background.completeOnboarding).call(background) + } catch (err) { + dispatch(actions.displayWarning(err.message)) + throw err + } - dispatch(actions.completeOnboarding()) - dispatch(actions.hideLoadingIndication()) - resolve() - }) - }) - }) + dispatch(actions.completeOnboarding()) + dispatch(actions.hideLoadingIndication()) } } From 02585c0bd023b379778fb850e53eecb9c45e5509 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 29 Mar 2019 12:51:54 +0800 Subject: [PATCH 7/8] lint fix --- test/unit/app/nodeify-test.js | 2 +- ui/app/store/actions.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/app/nodeify-test.js b/test/unit/app/nodeify-test.js index 0dafc6653..fa5e49fb2 100644 --- a/test/unit/app/nodeify-test.js +++ b/test/unit/app/nodeify-test.js @@ -42,7 +42,7 @@ describe('nodeify', function () { try { nodified('baz') } catch (err) { - done(new Error('should not have thrown an error synchronously')) + done(new Error('should not have thrown an error synchronously')) } }) diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index db3427ee6..e5825b5f6 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -2490,7 +2490,7 @@ function setShowFiatConversionOnTestnetsPreference (value) { function setCompletedOnboarding () { return async dispatch => { dispatch(actions.showLoadingIndication()) - + try { await pify(background.markAllNoticesRead).call(background) } catch (err) { From ed381f5b2a3a85d351248769b3d150742acb703b Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 29 Mar 2019 13:10:42 +0800 Subject: [PATCH 8/8] test - unit - ui - actions - setCompletedOnboarding - fix stub to call callback --- test/unit/ui/app/actions.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index a3b979d75..a578ec89c 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -1313,7 +1313,9 @@ describe('Actions', () => { beforeEach(() => { markAllNoticesReadSpy = sinon.stub(background, 'markAllNoticesRead') + markAllNoticesReadSpy.callsFake(cb => cb()) completeOnboardingSpy = sinon.stub(background, 'completeOnboarding') + completeOnboardingSpy.callsFake(cb => cb()) }) after(() => {