From e1dca54bf74e43e07e60557e9c1e4a044d962057 Mon Sep 17 00:00:00 2001 From: Nazar Gargol Date: Thu, 18 Apr 2019 11:25:17 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20Made=20notifications=20dismissib?= =?UTF-8?q?le=20per=20user?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #10656 - To make sure more users see important updates or announcements notification dismissal now works per user instead of globally - Expanded acceptance test for notification deletion - Expanded regression test covering multiuser dismissal of notification - Added clarifying comment about destroyAll method use in API --- core/server/api/v2/notifications.js | 25 ++++- .../utils/serializers/output/notifications.js | 1 + .../old/admin/notifications_spec.js | 47 ++++---- .../api/v2/admin/notifications_spec.js | 105 +++++++++++++++--- 4 files changed, 135 insertions(+), 43 deletions(-) diff --git a/core/server/api/v2/notifications.js b/core/server/api/v2/notifications.js index ea36b8a1d7..5fef525113 100644 --- a/core/server/api/v2/notifications.js +++ b/core/server/api/v2/notifications.js @@ -20,12 +20,20 @@ _private.fetchAllNotifications = () => { return allNotifications; }; +_private.wasSeen = (notification, user) => { + if (notification.seenBy === undefined) { + return notification.seen; + } else { + return notification.seenBy.includes(user.id); + } +}; + module.exports = { docName: 'notifications', browse: { permissions: true, - query() { + query(frame) { let allNotifications = _private.fetchAllNotifications(); allNotifications = _.orderBy(allNotifications, 'addedAt', 'desc'); @@ -55,7 +63,7 @@ module.exports = { } } - return notification.seen !== true; + return !_private.wasSeen(notification, frame.user); }); return allNotifications; @@ -172,13 +180,19 @@ module.exports = { })); } - if (notificationToMarkAsSeen.seen) { + if (_private.wasSeen(notificationToMarkAsSeen, frame.user)) { return Promise.resolve(); } // @NOTE: We don't remove the notifications, because otherwise we will receive them again from the service. allNotifications[notificationToMarkAsSeenIndex].seen = true; + if (!allNotifications[notificationToMarkAsSeenIndex].seenBy) { + allNotifications[notificationToMarkAsSeenIndex].seenBy = []; + } + + allNotifications[notificationToMarkAsSeenIndex].seenBy.push(frame.user.id); + return api.settings.edit({ settings: [{ key: 'notifications', @@ -188,6 +202,11 @@ module.exports = { } }, + /** + * Clears all notifications. Method used in tests only + * + * @private Not exposed over HTTP + */ destroyAll: { statusCode: 204, permissions: { diff --git a/core/server/api/v2/utils/serializers/output/notifications.js b/core/server/api/v2/utils/serializers/output/notifications.js index 1c5a29a7d5..4cd0ae7667 100644 --- a/core/server/api/v2/utils/serializers/output/notifications.js +++ b/core/server/api/v2/utils/serializers/output/notifications.js @@ -16,6 +16,7 @@ module.exports = { response.forEach((notification) => { delete notification.seen; + delete notification.seenBy; delete notification.addedAt; }); diff --git a/core/test/acceptance/old/admin/notifications_spec.js b/core/test/acceptance/old/admin/notifications_spec.js index e10c017ea2..3ad04c2ba6 100644 --- a/core/test/acceptance/old/admin/notifications_spec.js +++ b/core/test/acceptance/old/admin/notifications_spec.js @@ -20,7 +20,7 @@ describe('Notifications API', function () { }); }); - it('Can add notification', function (done) { + it('Can add notification', function () { const newNotification = { type: 'info', message: 'test notification', @@ -28,17 +28,13 @@ describe('Notifications API', function () { id: 'customId' }; - request.post(localUtils.API.getApiQuery('notifications/')) + return request.post(localUtils.API.getApiQuery('notifications/')) .set('Origin', config.get('url')) .send({notifications: [newNotification]}) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) .expect(201) - .end(function (err, res) { - if (err) { - return done(err); - } - + .then(function (res) { var jsonResponse = res.body; should.exist(jsonResponse.notifications); @@ -52,13 +48,11 @@ describe('Notifications API', function () { should.exist(jsonResponse.notifications[0].location); jsonResponse.notifications[0].location.should.equal('bottom'); jsonResponse.notifications[0].id.should.be.a.String(); - - done(); }); }); - it('Can delete notification', function (done) { - var newNotification = { + it('Can delete notification', function () { + const newNotification = { type: 'info', message: 'test notification', status: 'alert', @@ -66,17 +60,13 @@ describe('Notifications API', function () { }; // create the notification that is to be deleted - request.post(localUtils.API.getApiQuery('notifications/')) + return request.post(localUtils.API.getApiQuery('notifications/')) .set('Origin', config.get('url')) .send({notifications: [newNotification]}) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) .expect(201) - .end(function (err, res) { - if (err) { - return done(err); - } - + .then(function (res) { const jsonResponse = res.body; should.exist(jsonResponse.notifications); @@ -87,18 +77,25 @@ describe('Notifications API', function () { jsonResponse.notifications[0].message.should.equal(newNotification.message); jsonResponse.notifications[0].status.should.equal(newNotification.status); - // begin delete test - request.del(localUtils.API.getApiQuery(`notifications/${jsonResponse.notifications[0].id}/`)) + return jsonResponse.notifications[0]; + }) + .then((notification) => { + return request.del(localUtils.API.getApiQuery(`notifications/${notification.id}/`)) .set('Origin', config.get('url')) .expect(204) - .end(function (err, res) { - if (err) { - return done(err); - } - + .then(function (res) { res.body.should.be.empty(); - done(); + return notification; + }); + }) + .then((notification) => { + return request.get(localUtils.API.getApiQuery(`notifications/`)) + .set('Origin', config.get('url')) + .expect(200) + .then(function (res) { + const deleted = res.body.notifications.filter(n => n.id === notification.id); + deleted.should.be.empty(); }); }); }); diff --git a/core/test/regression/api/v2/admin/notifications_spec.js b/core/test/regression/api/v2/admin/notifications_spec.js index 25ff3718c2..b67a3a8ab3 100644 --- a/core/test/regression/api/v2/admin/notifications_spec.js +++ b/core/test/regression/api/v2/admin/notifications_spec.js @@ -4,20 +4,11 @@ const testUtils = require('../../../../utils'); const config = require('../../../../../server/config'); const localUtils = require('./utils'); const ghost = testUtils.startGhost; -let request; describe('Notifications API', function () { - before(function () { - return ghost() - .then(function (_ghostServer) { - request = supertest.agent(config.get('url')); - }) - .then(function () { - return localUtils.doAuth(request); - }); - }); - describe('As Editor', function () { + let request; + before(function () { return ghost() .then(function (_ghostServer) { @@ -41,8 +32,7 @@ describe('Notifications API', function () { const newNotification = { type: 'info', message: 'test notification', - custom: true, - id: 'customId' + custom: true }; return request.post(localUtils.API.getApiQuery('notifications/')) @@ -77,6 +67,8 @@ describe('Notifications API', function () { }); describe('As Author', function () { + let request; + before(function () { return ghost() .then(function (_ghostServer) { @@ -100,8 +92,7 @@ describe('Notifications API', function () { const newNotification = { type: 'info', message: 'test notification', - custom: true, - id: 'customId' + custom: true }; return request.post(localUtils.API.getApiQuery('notifications/')) @@ -120,4 +111,88 @@ describe('Notifications API', function () { .expect(403); }); }); + + describe('Can view by multiple users', function () { + let requestEditor1; + let requestEditor2; + let notification; + + before(function () { + return ghost() + .then(function (_ghostServer) { + requestEditor1 = supertest.agent(config.get('url')); + requestEditor2 = supertest.agent(config.get('url')); + }) + .then(function () { + return testUtils.createUser({ + user: testUtils.DataGenerator.forKnex.createUser({ + email: 'test+editor1@ghost.org' + }), + role: testUtils.DataGenerator.Content.roles[1].name + }); + }) + .then((user) => { + requestEditor1.user = user; + return localUtils.doAuth(requestEditor1); + }) + .then(function () { + return testUtils.createUser({ + user: testUtils.DataGenerator.forKnex.createUser({ + email: 'test+editor2@ghost.org' + }), + role: testUtils.DataGenerator.Content.roles[1].name + }); + }) + .then((user) => { + requestEditor2.user = user; + return localUtils.doAuth(requestEditor2); + }) + .then(() => { + const newNotification = { + type: 'info', + message: 'multiple views', + custom: true + }; + + return requestEditor1.post(localUtils.API.getApiQuery('notifications/')) + .set('Origin', config.get('url')) + .send({notifications: [newNotification]}) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(201) + .then((res) => { + notification = res.body.notifications[0]; + }); + }); + }); + + it('notification is visible and dismissible by other user', function () { + return requestEditor1.del(localUtils.API.getApiQuery(`notifications/${notification.id}`)) + .set('Origin', config.get('url')) + .expect(204) + .then(() => { + return requestEditor2.get(localUtils.API.getApiQuery(`notifications/`)) + .set('Origin', config.get('url')) + .expect(200) + .then(function (res) { + const deleted = res.body.notifications.filter(n => n.id === notification.id); + deleted.should.not.be.empty(); + }); + }) + .then(() => { + return requestEditor2.del(localUtils.API.getApiQuery(`notifications/${notification.id}`)) + .set('Origin', config.get('url')) + .expect(204); + }) + .then(() => { + return requestEditor2.get(localUtils.API.getApiQuery(`notifications/`)) + .set('Origin', config.get('url')) + .expect(200) + .then(function (res) { + const deleted = res.body.notifications.filter(n => n.id === notification.id); + deleted.should.be.empty(); + }); + }); + }); + }); });