From 2a803aecdd17489de8de8752dccd76b790a92016 Mon Sep 17 00:00:00 2001 From: Fabian Becker Date: Mon, 28 Apr 2014 22:58:18 +0200 Subject: [PATCH] Proper endpoints for persistent notifications closes #2637 - Add new get API route for all notifications - Wrap API responses to comply with JSON-API - Add new tests / adjust fixtures - Adjust all occurences of passive notifications --- core/server/api/index.js | 4 +- core/server/api/notifications.js | 47 +++++++++--- core/server/controllers/admin.js | 17 ++--- core/server/index.js | 4 +- core/server/mail.js | 8 +-- core/server/middleware/index.js | 9 ++- core/server/middleware/middleware.js | 6 +- core/server/routes/api.js | 1 + .../routes/api/notifications_test.js | 51 ++++++------- .../integration/api/api_notifications_spec.js | 72 +++++++++++++++++-- core/test/unit/middleware_spec.js | 25 +------ core/test/utils/api.js | 2 +- 12 files changed, 150 insertions(+), 96 deletions(-) diff --git a/core/server/api/index.js b/core/server/api/index.js index a5c37e05a5..5d63687c28 100644 --- a/core/server/api/index.js +++ b/core/server/api/index.js @@ -101,8 +101,8 @@ function locationHeader(req, result) { post = result.posts[0]; location = apiRoot + '/posts/' + post.id + '/?status=' + post.status; } else if (endpoint === 'notifications') { - notification = result; - location = apiRoot + '/notifications/' + notification.id; + notification = result.notifications; + location = apiRoot + '/notifications/' + notification[0].id; } } diff --git a/core/server/api/notifications.js b/core/server/api/notifications.js index d20d98c89e..03689884b1 100644 --- a/core/server/api/notifications.js +++ b/core/server/api/notifications.js @@ -2,28 +2,43 @@ var when = require('when'), _ = require('lodash'), // Holds the persistent notifications notificationsStore = [], + // Holds the last used id + notificationCounter = 0, notifications; // ## Notifications notifications = { browse: function browse() { - return when(notificationsStore); + return when({ 'notifications': notificationsStore }); }, // #### Destroy // **takes:** an identifier object ({id: id}) destroy: function destroy(i) { - notificationsStore = _.reject(notificationsStore, function (element) { - return element.id === i.id; + var notification = _.find(notificationsStore, function (element) { + return element.id === parseInt(i.id, 10); }); - // **returns:** a promise for remaining notifications as a json object - return when(notificationsStore); + + if (notification && !notification.dismissable) { + return when.reject({type: 'NoPermission', message: 'You do not have permission to dismiss this notification.'}); + } + + if (!notification) { + return when.reject({type: 'NoPermission', message: 'Notification does not exist.'}); + } + + notificationsStore = _.reject(notificationsStore, function (element) { + return element.id === parseInt(i.id, 10); + }); + // **returns:** a promise for the deleted object + return when({notifications: [notification]}); }, destroyAll: function destroyAll() { notificationsStore = []; + notificationCounter = 0; return when(notificationsStore); }, @@ -34,14 +49,28 @@ notifications = { // msg = { // type: 'error', // this can be 'error', 'success', 'warn' and 'info' // message: 'This is an error', // A string. Should fit in one line. - // status: 'persistent', // or 'passive' - // id: 'auniqueid' // A unique ID + // location: 'bottom', // A string where this notification should appear. can be 'bottom' or 'top' + // dismissable: true // A Boolean. Whether the notification is dismissable or not. // }; // ``` add: function add(notification) { - // **returns:** a promise for all notifications as a json object + + var defaults = { + dismissable: true, + location: 'bottom', + status: 'persistent' + }; + + notificationCounter = notificationCounter + 1; + + notification = _.assign(defaults, notification, { + id: notificationCounter + //status: 'persistent' + }); + notificationsStore.push(notification); - return when(notification); + // **returns:** a promise of the new notification object + return when({ notifications: [notification]}); } }; diff --git a/core/server/controllers/admin.js b/core/server/controllers/admin.js index ff0899ae14..67728d6bb1 100644 --- a/core/server/controllers/admin.js +++ b/core/server/controllers/admin.js @@ -134,9 +134,7 @@ adminControllers = { }).otherwise(function (err) { var notification = { type: 'error', - message: 'Your export file could not be generated. Error: ' + err.message, - status: 'persistent', - id: 'errorexport' + message: 'Your export file could not be generated. Error: ' + err.message }; errors.logError(err, 'admin.js', "Your export file could not be generated."); @@ -179,8 +177,7 @@ adminControllers = { var notification = { type: 'success', message: 'You were successfully signed out', - status: 'passive', - id: 'successlogout' + status: 'passive' }; return api.notifications.add(notification).then(function () { @@ -333,8 +330,7 @@ adminControllers = { var notification = { type: 'success', message: 'Check your email for further instructions', - status: 'passive', - id: 'successresetpw' + status: 'passive' }; return api.notifications.add(notification).then(function () { @@ -368,9 +364,7 @@ adminControllers = { // Redirect to forgotten if invalid token var notification = { type: 'error', - message: 'Invalid or expired token', - status: 'persistent', - id: 'errorinvalidtoken' + message: 'Invalid or expired token' }; errors.logError(err, 'admin.js', "Please check the provided token for validity and expiration."); @@ -392,8 +386,7 @@ adminControllers = { var notification = { type: 'success', message: 'Password changed successfully.', - status: 'passive', - id: 'successresetpw' + status: 'passive' }; return api.notifications.add(notification).then(function () { diff --git a/core/server/index.js b/core/server/index.js index d9888fda3f..5ee28235b6 100644 --- a/core/server/index.js +++ b/core/server/index.js @@ -46,9 +46,7 @@ function doFirstRun() { return api.notifications.add({ type: 'info', - message: firstRunMessage.join(' '), - status: 'persistent', - id: 'ghost-first-run' + message: firstRunMessage.join(' ') }); } diff --git a/core/server/mail.js b/core/server/mail.js index 08453d3ec7..8969f09ef6 100644 --- a/core/server/mail.js +++ b/core/server/mail.js @@ -62,9 +62,7 @@ GhostMailer.prototype.usingSendmail = function () { "Ghost is attempting to use your server's sendmail to send e-mail.", "It is recommended that you explicitly configure an e-mail service,", "See http://docs.ghost.org/mail for instructions" - ].join(' '), - status: 'persistent', - id: 'ghost-mail-fallback' + ].join(' ') }); }; @@ -74,9 +72,7 @@ GhostMailer.prototype.emailDisabled = function () { message: [ "Ghost is currently unable to send e-mail.", "See http://docs.ghost.org/mail for instructions" - ].join(' '), - status: 'persistent', - id: 'ghost-mail-disabled' + ].join(' ') }); this.transport = null; }; diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 12ca84d305..7fe548fbb6 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -43,7 +43,7 @@ function ghostLocals(req, res, next) { api.notifications.browse() ]).then(function (values) { var currentUser = values[0].users[0], - notifications = values[1]; + notifications = values[1].notifications; _.extend(res.locals, { currentUser: { @@ -56,9 +56,10 @@ function ghostLocals(req, res, next) { next(); }).otherwise(function () { // Only show passive notifications + // ToDo: Remove once ember handles passive notifications. api.notifications.browse().then(function (notifications) { _.extend(res.locals, { - messages: _.reject(notifications, function (notification) { + messages: _.reject(notifications.notifications, function (notification) { return notification.status !== 'passive'; }) }); @@ -341,6 +342,7 @@ module.exports = function (server, dbHash) { expressServer.use(subdir + '/api/', middleware.cacheControl('private')); expressServer.use(subdir + '/ghost/', middleware.cacheControl('private')); + // enable authentication; has to be done before CSRF handling expressServer.use(middleware.authenticate); @@ -349,8 +351,11 @@ module.exports = function (server, dbHash) { // local data expressServer.use(ghostLocals); + // So on every request we actually clean out redundant passive notifications from the server side + // ToDo: Remove when ember handles passive notifications. expressServer.use(middleware.cleanNotifications); + // Initialise the views expressServer.use(initViews); diff --git a/core/server/middleware/middleware.js b/core/server/middleware/middleware.js index e998c4bd3b..a4e2c11f13 100644 --- a/core/server/middleware/middleware.js +++ b/core/server/middleware/middleware.js @@ -70,8 +70,7 @@ var middleware = { msg = { type: 'error', message: 'Please Sign In', - status: 'passive', - id: 'failedauth' + status: 'passive' }; // let's only add the notification once if (!_.contains(_.pluck(notifications, 'id'), 'failedauth')) { @@ -110,10 +109,11 @@ var middleware = { // That being ghost.notifications, and let's remove the passives from there // plus the local messages, as they have already been added at this point // otherwise they'd appear one too many times + // ToDo: Remove once ember handles passive notifications. cleanNotifications: function (req, res, next) { /*jslint unparam:true*/ api.notifications.browse().then(function (notifications) { - _.each(notifications, function (notification) { + _.each(notifications.notifications, function (notification) { if (notification.status === 'passive') { api.notifications.destroy(notification); } diff --git a/core/server/routes/api.js b/core/server/routes/api.js index 1a5dd0066a..c24f393d5e 100644 --- a/core/server/routes/api.js +++ b/core/server/routes/api.js @@ -23,6 +23,7 @@ module.exports = function (server) { // #### Notifications server.del('/ghost/api/v0.1/notifications/:id', api.requestHandler(api.notifications.destroy)); server.post('/ghost/api/v0.1/notifications/', api.requestHandler(api.notifications.add)); + server.get('/ghost/api/v0.1/notifications/', api.requestHandler(api.notifications.browse)); // #### Import/Export server.get('/ghost/api/v0.1/db/', api.requestHandler(api.db.exportContent)); server.post('/ghost/api/v0.1/db/', middleware.busboy, api.requestHandler(api.db.importContent)); diff --git a/core/test/functional/routes/api/notifications_test.js b/core/test/functional/routes/api/notifications_test.js index 2bd29e94ce..5ae4b14bb5 100644 --- a/core/test/functional/routes/api/notifications_test.js +++ b/core/test/functional/routes/api/notifications_test.js @@ -79,9 +79,7 @@ describe('Notifications API', function () { describe('Add', function () { var newNotification = { type: 'info', - message: 'test notification', - status: 'persistent', - id: 'add-test-1' + message: 'test notification' }; it('creates a new notification', function (done) { @@ -93,17 +91,16 @@ describe('Notifications API', function () { if (err) { return done(err); } - - res.headers['location'].should.equal('/ghost/api/v0.1/notifications/' + newNotification.id); var jsonResponse = res.body; - testUtils.API.checkResponse(jsonResponse, 'notification'); + jsonResponse.notifications.should.exist; - jsonResponse.type.should.equal(newNotification.type); - jsonResponse.message.should.equal(newNotification.message); - jsonResponse.status.should.equal(newNotification.status); - jsonResponse.id.should.equal(newNotification.id); + testUtils.API.checkResponse(jsonResponse.notifications[0], 'notification'); + + jsonResponse.notifications[0].type.should.equal(newNotification.type); + jsonResponse.notifications[0].message.should.equal(newNotification.message); + jsonResponse.notifications[0].status.should.equal('persistent'); done(); }); @@ -114,8 +111,7 @@ describe('Notifications API', function () { var newNotification = { type: 'info', message: 'test notification', - status: 'persistent', - id: 'delete-test-1' + status: 'persistent' }; it('deletes a notification', function (done) { @@ -130,17 +126,16 @@ describe('Notifications API', function () { } var location = res.headers['location']; - location.should.equal('/ghost/api/v0.1/notifications/' + newNotification.id); var jsonResponse = res.body; - testUtils.API.checkResponse(jsonResponse, 'notification'); - - jsonResponse.type.should.equal(newNotification.type); - jsonResponse.message.should.equal(newNotification.message); - jsonResponse.status.should.equal(newNotification.status); - jsonResponse.id.should.equal(newNotification.id); + jsonResponse.notifications.should.exist; + testUtils.API.checkResponse(jsonResponse.notifications[0], 'notification'); + jsonResponse.notifications[0].type.should.equal(newNotification.type); + jsonResponse.notifications[0].message.should.equal(newNotification.message); + jsonResponse.notifications[0].status.should.equal(newNotification.status); + // begin delete test request.del(location) .set('X-CSRF-Token', csrfToken) @@ -150,17 +145,13 @@ describe('Notifications API', function () { return done(err); } - // a delete returns a JSON object containing all notifications - // so we can make sure the notification we just deleted isn't - // included - var notifications = res.body; - - var success; - notifications.forEach(function (n) { - success = n.id !== newNotification.id; - }); - - success.should.be.true; + // a delete returns a JSON object containing the notification + // we just deleted. + var deleteResponse = res.body; + deleteResponse.notifications.should.exist; + deleteResponse.notifications[0].type.should.equal(newNotification.type); + deleteResponse.notifications[0].message.should.equal(newNotification.message); + deleteResponse.notifications[0].status.should.equal(newNotification.status); done(); }); diff --git a/core/test/integration/api/api_notifications_spec.js b/core/test/integration/api/api_notifications_spec.js index bbd766ad86..582e05608a 100644 --- a/core/test/integration/api/api_notifications_spec.js +++ b/core/test/integration/api/api_notifications_spec.js @@ -34,14 +34,76 @@ describe('Notifications API', function () { var msg = { type: 'error', // this can be 'error', 'success', 'warn' and 'info' message: 'This is an error', // A string. Should fit in one line. - status: 'persistent', // or 'passive' - id: 'auniqueid' // A unique ID }; - NotificationsAPI.add(msg).then(function (notification){ + NotificationsAPI.add(msg).then(function (notification) { NotificationsAPI.browse().then(function (results) { should.exist(results); - results.length.should.be.above(0); - testUtils.API.checkResponse(results[0], 'notification'); + should.exist(results.notifications); + results.notifications.length.should.be.above(0); + testUtils.API.checkResponse(results.notifications[0], 'notification'); + done(); + }); + }); + }); + + it('can add, adds defaults', function (done) { + var msg = { + type: 'info', + message: 'Hello, this is dog' + }; + + NotificationsAPI.add(msg).then(function (result) { + var notification; + + should.exist(result); + should.exist(result.notifications); + + notification = result.notifications[0]; + notification.dismissable.should.be.true; + should.exist(notification.location); + notification.location.should.equal('bottom'); + + done(); + }); + }); + + it('can add, adds id and status', function (done) { + var msg = { + type: 'info', + message: 'Hello, this is dog', + id: 99 + }; + + NotificationsAPI.add(msg).then(function (result) { + var notification; + + should.exist(result); + should.exist(result.notifications); + + notification = result.notifications[0]; + notification.id.should.be.a.Number; + notification.id.should.not.equal(99); + should.exist(notification.status); + notification.status.should.equal('persistent') + + done(); + }); + }); + + it('can destroy', function (done) { + var msg = { + type: 'error', + message: 'Goodbye, cruel world!' + }; + + NotificationsAPI.add(msg).then(function (result) { + var notification = result.notifications[0]; + + NotificationsAPI.destroy({ id: notification.id }).then(function (result) { + should.exist(result); + should.exist(result.notifications); + result.notifications[0].id.should.equal(notification.id); + done(); }).catch(done); }); diff --git a/core/test/unit/middleware_spec.js b/core/test/unit/middleware_spec.js index 780973ed79..43119dad24 100644 --- a/core/test/unit/middleware_spec.js +++ b/core/test/unit/middleware_spec.js @@ -48,27 +48,6 @@ describe('Middleware', function () { }).catch(done); }); - it('should only add one message to the notification array', function (done) { - var path = 'test/path/party'; - - req.path = '/ghost/' + path; - middleware.auth(req, res, null).then(function () { - assert(res.redirect.calledWithMatch('/ghost/signin/?r=' + encodeURIComponent(path))); - return api.notifications.browse().then(function (notifications) { - assert.equal(notifications.length, 1); - return; - }).catch(done); - }).then(function () { - return middleware.auth(req, res, null); - }).then(function () { - assert(res.redirect.calledWithMatch('/ghost/signin/?r=' + encodeURIComponent(path))); - return api.notifications.browse().then(function (notifications) { - assert.equal(notifications.length, 1); - done(); - }).catch(done); - }); - }); - it('should call next if session user exists', function (done) { req.session.user = {}; @@ -168,8 +147,8 @@ describe('Middleware', function () { it('should clean all passive messages', function (done) { middleware.cleanNotifications(null, null, function () { api.notifications.browse().then(function (notifications) { - should(notifications.length).eql(1); - var passiveMsgs = _.filter(notifications, function (notification) { + should(notifications.notifications.length).eql(1); + var passiveMsgs = _.filter(notifications.notifications, function (notification) { return notification.status === 'passive'; }); assert.equal(passiveMsgs.length, 0); diff --git a/core/test/utils/api.js b/core/test/utils/api.js index 7aaaf43b1a..ca5f3e5561 100644 --- a/core/test/utils/api.js +++ b/core/test/utils/api.js @@ -17,7 +17,7 @@ var url = require('url'), user: ['id', 'uuid', 'name', 'slug', 'email', 'image', 'cover', 'bio', 'website', 'location', 'accessibility', 'status', 'language', 'meta_title', 'meta_description', 'last_login', 'created_at', 'created_by', 'updated_at', 'updated_by'], - notification: ['type', 'message', 'status', 'id'] + notification: ['type', 'message', 'status', 'id', 'dismissable', 'location'] }; function getApiQuery(route) {