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/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 540aee936..058d527c0 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: 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 6fe8b8cf0..63b422c5b 100644 --- a/app/scripts/notice-controller.js +++ b/app/scripts/notice-controller.js @@ -58,6 +58,18 @@ module.exports = class NoticeController extends EventEmitter { } } + markAllNoticesRead () { + const noticeList = this.getNoticesList() + noticeList.forEach(notice => { + notice.read = true + notice.body = '' + }) + this.setNoticesList(noticeList) + const latestNotice = this.getNextUnreadNotice() + return latestNotice + } + + 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..caa50a03e 100644 --- a/test/unit/app/controllers/notice-controller-test.js +++ b/test/unit/app/controllers/notice-controller-test.js @@ -39,6 +39,31 @@ describe('notice-controller', function () { }) }) + describe('#markAllNoticesRead', () => { + it('marks all notices read', async () => { + 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) + }) + }) + describe('#getNextUnreadNotice', function () { it('should retrieve the latest unread notice', function (done) { var testList = [ diff --git a/test/unit/app/nodeify-test.js b/test/unit/app/nodeify-test.js index 938b76c68..fa5e49fb2 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}`)) + } + }) }) diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index 46e94bb32..a578ec89c 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -1308,6 +1308,29 @@ describe('Actions', () => { }) }) + describe('#setCompletedOnboarding', () => { + let markAllNoticesReadSpy, completeOnboardingSpy + + beforeEach(() => { + markAllNoticesReadSpy = sinon.stub(background, 'markAllNoticesRead') + markAllNoticesReadSpy.callsFake(cb => cb()) + completeOnboardingSpy = sinon.stub(background, 'completeOnboarding') + completeOnboardingSpy.callsFake(cb => cb()) + }) + + after(() => { + markAllNoticesReadSpy.restore() + completeOnboardingSpy.restore() + }) + + it('completing onboarding marks all notices as read', async () => { + const store = mockStore() + await store.dispatch(actions.setCompletedOnboarding()) + assert.equal(markAllNoticesReadSpy.callCount, 1) + assert.equal(completeOnboardingSpy.callCount, 1) + }) + }) + describe('#updateNetworkNonce', () => { let getTransactionCountSpy diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index b2aa28c93..e5825b5f6 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -2488,21 +2488,27 @@ function setShowFiatConversionOnTestnetsPreference (value) { } function setCompletedOnboarding () { - return dispatch => { + return async dispatch => { dispatch(actions.showLoadingIndication()) - return new Promise((resolve, reject) => { - background.completeOnboarding(err => { - dispatch(actions.hideLoadingIndication()) - if (err) { - dispatch(actions.displayWarning(err.message)) - return reject(err) - } + try { + await pify(background.markAllNoticesRead).call(background) + } catch (err) { + dispatch(actions.displayWarning(err.message)) + throw err + } - dispatch(actions.completeOnboarding()) - resolve() - }) - }) + dispatch(actions.clearNotices()) + + try { + await pify(background.completeOnboarding).call(background) + } catch (err) { + dispatch(actions.displayWarning(err.message)) + throw err + } + + dispatch(actions.completeOnboarding()) + dispatch(actions.hideLoadingIndication()) } }