From 197ee9f4d94e3b442626603ad96d29f276924abe Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 10 Aug 2016 15:27:40 +0100 Subject: [PATCH 1/8] add a .nvmrc file --- services/notifications/.nvmrc | 1 + 1 file changed, 1 insertion(+) create mode 100644 services/notifications/.nvmrc diff --git a/services/notifications/.nvmrc b/services/notifications/.nvmrc new file mode 100644 index 0000000000..bf77d54968 --- /dev/null +++ b/services/notifications/.nvmrc @@ -0,0 +1 @@ +4.2 From fc78bfe86ca1b3e54881c09a75f3bcf097e05ca4 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 11 Aug 2016 10:01:21 +0100 Subject: [PATCH 2/8] Add an endpoint to remove a notification by its key alone --- services/notifications/app.coffee | 1 + .../app/coffee/Notifications.coffee | 17 +++++++++++----- .../app/coffee/NotificationsController.coffee | 7 +++++++ .../coffee/NotificationsControllerTest.coffee | 20 ++++++++++++++----- .../unit/coffee/NotificationsTests.coffee | 20 +++++++++++++++---- 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/services/notifications/app.coffee b/services/notifications/app.coffee index 8b78c4b8f8..7008c814b0 100644 --- a/services/notifications/app.coffee +++ b/services/notifications/app.coffee @@ -24,6 +24,7 @@ app.post '/user/:user_id', controller.addNotification app.get '/user/:user_id', controller.getUserNotifications app.del '/user/:user_id/notification/:notification_id', controller.removeNotificationId app.del '/user/:user_id', controller.removeNotificationKey +app.del '/notification/key/:key', controller.removeNotificationByKeyOnly app.get '/status', (req, res)-> res.send('notifications sharelatex up') diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index f9a6adc86a..bb752d35af 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -22,7 +22,7 @@ module.exports = logger.log number:number, user_id:user_id, key:notification.key, "alredy has notification key for user" callback number else - doc = + doc = user_id: ObjectId(user_id) key: notification.key messageOpts: notification.messageOpts @@ -30,17 +30,24 @@ module.exports = db.notifications.insert(doc, callback) removeNotificationId: (user_id, notification_id, callback)-> - searchOps = + searchOps = user_id:ObjectId(user_id) _id:ObjectId(notification_id) - updateOperation = + updateOperation = "$unset": {templateKey:true, messageOpts: true} db.notifications.update searchOps, updateOperation, callback removeNotificationKey: (user_id, notification_key, callback)-> - searchOps = + searchOps = user_id:ObjectId(user_id) key: notification_key - updateOperation = + updateOperation = + "$unset": {templateKey:true} + db.notifications.update searchOps, updateOperation, callback + + removeNotificationByKeyOnly: (notification_key, callback)-> + searchOps = + key: notification_key + updateOperation = "$unset": {templateKey:true} db.notifications.update searchOps, updateOperation, callback diff --git a/services/notifications/app/coffee/NotificationsController.coffee b/services/notifications/app/coffee/NotificationsController.coffee index 6ee7c68cbc..416cdeccc0 100644 --- a/services/notifications/app/coffee/NotificationsController.coffee +++ b/services/notifications/app/coffee/NotificationsController.coffee @@ -30,3 +30,10 @@ module.exports = metrics.inc "removeNotificationKey" Notifications.removeNotificationKey req.params.user_id, req.body.key, (err, notifications)-> res.send() + + removeNotificationByKeyOnly: (req, res)-> + notification_key = req.params.key + logger.log {notification_key}, "mark notification as read by key only" + metrics.inc "removeNotificationKey" + Notifications.removeNotificationByKeyOnly notification_key, (err, notifications)-> + res.send() diff --git a/services/notifications/test/unit/coffee/NotificationsControllerTest.coffee b/services/notifications/test/unit/coffee/NotificationsControllerTest.coffee index 286abdf00e..c7c8fdd987 100644 --- a/services/notifications/test/unit/coffee/NotificationsControllerTest.coffee +++ b/services/notifications/test/unit/coffee/NotificationsControllerTest.coffee @@ -24,7 +24,7 @@ describe 'Notifications Controller', -> describe "getUserNotifications", -> it 'should ask the notifications for the users notifications', (done)-> @notifications.getUserNotifications = sinon.stub().callsArgWith(1, null, @stubbedNotification) - req = + req = params: user_id: user_id @controller.getUserNotifications req, json:(result)=> @@ -35,7 +35,7 @@ describe 'Notifications Controller', -> describe "addNotification", -> it "should tell the notifications to add the notification for the user", (done)-> @notifications.addNotification = sinon.stub().callsArgWith(2) - req = + req = params: user_id: user_id body: @stubbedNotification @@ -46,7 +46,7 @@ describe 'Notifications Controller', -> describe "removeNotificationId", -> it "should tell the notifications to mark the notification Id as read", (done)-> @notifications.removeNotificationId = sinon.stub().callsArgWith(2) - req = + req = params: user_id: user_id notification_id: notification_id @@ -57,10 +57,20 @@ describe 'Notifications Controller', -> describe "removeNotificationKey", -> it "should tell the notifications to mark the notification Key as read", (done)-> @notifications.removeNotificationKey = sinon.stub().callsArgWith(2) - req = + req = params: user_id: user_id body: {key: notification_key} @controller.removeNotificationKey req, send:(result)=> @notifications.removeNotificationKey.calledWith(user_id, notification_key).should.equal true - done() \ No newline at end of file + done() + + describe "removeNotificationByKeyOnly", -> + it "should tell the notifications to mark the notification Key as read", (done)-> + @notifications.removeNotificationByKeyOnly = sinon.stub().callsArgWith(1) + req = + params: + key: notification_key + @controller.removeNotificationByKeyOnly req, send:(result)=> + @notifications.removeNotificationByKeyOnly.calledWith(notification_key).should.equal true + done() diff --git a/services/notifications/test/unit/coffee/NotificationsTests.coffee b/services/notifications/test/unit/coffee/NotificationsTests.coffee index 47f31a3d06..46d21db649 100644 --- a/services/notifications/test/unit/coffee/NotificationsTests.coffee +++ b/services/notifications/test/unit/coffee/NotificationsTests.coffee @@ -65,10 +65,10 @@ describe 'Notifications Tests', -> @updateStub.callsArgWith(2, null) @notifications.removeNotificationId user_id, notification_id, (err)=> - searchOps = + searchOps = user_id:ObjectId(user_id) _id:ObjectId(notification_id) - updateOperation = + updateOperation = "$unset": {templateKey:true, messageOpts:true} @updateStub.calledWith(searchOps, updateOperation).should.equal true done() @@ -78,10 +78,22 @@ describe 'Notifications Tests', -> @updateStub.callsArgWith(2, null) @notifications.removeNotificationKey user_id, notification_key, (err)=> - searchOps = + searchOps = user_id:ObjectId(user_id) key: notification_key - updateOperation = + updateOperation = + "$unset": {templateKey:true} + @updateStub.calledWith(searchOps, updateOperation).should.equal true + done() + + describe 'removeNotificationByKeyOnly', -> + it 'should mark the notification key as read', (done)-> + @updateStub.callsArgWith(2, null) + + @notifications.removeNotificationByKeyOnly notification_key, (err)=> + searchOps = + key: notification_key + updateOperation = "$unset": {templateKey:true} @updateStub.calledWith(searchOps, updateOperation).should.equal true done() From 05e390ef8927228b6548e8396d3ea2f359dd8f4d Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 11 Aug 2016 11:05:11 +0100 Subject: [PATCH 3/8] Add an optional expiry to notifications. --- .../app/coffee/Notifications.coffee | 4 ++ .../unit/coffee/NotificationsTests.coffee | 48 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index bb752d35af..4238ae37e9 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -27,6 +27,10 @@ module.exports = key: notification.key messageOpts: notification.messageOpts templateKey: notification.templateKey + expires: notification.expires || false + # ttl index on `expiresFrom` field + if doc.expires + doc.expiresFrom = new Date() db.notifications.insert(doc, callback) removeNotificationId: (user_id, notification_id, callback)-> diff --git a/services/notifications/test/unit/coffee/NotificationsTests.coffee b/services/notifications/test/unit/coffee/NotificationsTests.coffee index 46d21db649..c09867a39f 100644 --- a/services/notifications/test/unit/coffee/NotificationsTests.coffee +++ b/services/notifications/test/unit/coffee/NotificationsTests.coffee @@ -1,5 +1,6 @@ sinon = require('sinon') chai = require('chai') +expect = chai.should should = chai.should() modulePath = "../../../app/js/Notifications.js" SandboxedModule = require('sandboxed-module') @@ -44,12 +45,27 @@ describe 'Notifications Tests', -> done() describe 'addNotification', -> + beforeEach -> + @stubbedNotification = { + user_id: ObjectId(user_id), + key:"notification-key", + messageOpts:"some info", + templateKey:"template-key" + } + @expectedDocument = { + user_id: @stubbedNotification.user_id, + key:"notification-key", + messageOpts:"some info", + templateKey:"template-key", + expires: false + } + it 'should insert the notification into the collection', (done)-> @insertStub.callsArgWith(1, null) @countStub.callsArgWith(1, null, 0) @notifications.addNotification user_id, @stubbedNotification, (err)=> - @insertStub.calledWith(@stubbedNotification).should.equal true + @insertStub.calledWith(@expectedDocument).should.equal true done() it 'should fail insert of existing notification key', (done)-> @@ -57,9 +73,37 @@ describe 'Notifications Tests', -> @countStub.callsArgWith(1, null, 1) @notifications.addNotification user_id, @stubbedNotification, (err)=> - @insertStub.calledWith(@stubbedNotification).should.equal false + @insertStub.calledWith(@expectedDocument).should.equal false done() + describe 'when the notification is set to expire', () -> + beforeEach -> + @stubbedNotification = { + user_id: ObjectId(user_id), + key:"notification-key", + messageOpts:"some info", + templateKey:"template-key", + expires: true + } + @expectedDocument = { + user_id: @stubbedNotification.user_id, + key:"notification-key", + messageOpts:"some info", + templateKey:"template-key", + expires: true, + expiresFrom: new Date() + } + + it 'should add an `expiresFrom` Date field to the inserted notification', (done)-> + @insertStub.callsArgWith(1, null) + @countStub.callsArgWith(1, null, 0) + + @notifications.addNotification user_id, @stubbedNotification, (err)=> + @insertStub.callCount.should.equal 1 + Object.keys(@insertStub.lastCall.args[0]).should.deep.equal Object.keys(@expectedDocument) + @insertStub.firstCall.args[0].expiresFrom.should.be.instanceof Date + done() + describe 'removeNotificationId', -> it 'should mark the notification id as read', (done)-> @updateStub.callsArgWith(2, null) From a17040f00f69ac01c54f9b12721ef179b75fc3a0 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 11 Aug 2016 12:03:50 +0100 Subject: [PATCH 4/8] Add commentary on ttl index --- services/notifications/README.md | 10 ++++++++++ services/notifications/app/coffee/Notifications.coffee | 2 ++ 2 files changed, 12 insertions(+) diff --git a/services/notifications/README.md b/services/notifications/README.md index 02e1a15e78..9db344dae4 100644 --- a/services/notifications/README.md +++ b/services/notifications/README.md @@ -2,3 +2,13 @@ notifications-sharelatex =============== An API for managing user notifications in ShareLaTeX + + +database indexes +================ + +For notification expiry to work, a ttl index on `notifications.expiresFrom` must be created: + +```javascript +db.notifications.createIndex({expiresFrom: 1}, {expireAfterSeconds: (60*60*24*30)}) +``` diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index 4238ae37e9..1f0e1b8237 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -29,6 +29,8 @@ module.exports = templateKey: notification.templateKey expires: notification.expires || false # ttl index on `expiresFrom` field + # in Mongo, TTL indexes only work on date fields, + # and ignore the document when that field is missing if doc.expires doc.expiresFrom = new Date() db.notifications.insert(doc, callback) From 0a5430468cc11513191be2741829ade8db7d0a21 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 12 Aug 2016 09:54:08 +0100 Subject: [PATCH 5/8] Alter how the notification doc is built --- services/notifications/app/coffee/Notifications.coffee | 4 ++-- .../notifications/test/unit/coffee/NotificationsTests.coffee | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index 1f0e1b8237..a9053734b9 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -27,11 +27,11 @@ module.exports = key: notification.key messageOpts: notification.messageOpts templateKey: notification.templateKey - expires: notification.expires || false # ttl index on `expiresFrom` field # in Mongo, TTL indexes only work on date fields, # and ignore the document when that field is missing - if doc.expires + if notification.expires? + doc.expires = notification.expires doc.expiresFrom = new Date() db.notifications.insert(doc, callback) diff --git a/services/notifications/test/unit/coffee/NotificationsTests.coffee b/services/notifications/test/unit/coffee/NotificationsTests.coffee index e329a9e34c..51215cb697 100644 --- a/services/notifications/test/unit/coffee/NotificationsTests.coffee +++ b/services/notifications/test/unit/coffee/NotificationsTests.coffee @@ -56,7 +56,6 @@ describe 'Notifications Tests', -> key:"notification-key", messageOpts:"some info", templateKey:"template-key", - expires: false } it 'should insert the notification into the collection', (done)-> From 5cb7e3dc035fb503ca5f47b8db008bf545318fe5 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 12 Aug 2016 09:56:16 +0100 Subject: [PATCH 6/8] Remove redundant `/notification` path component --- services/notifications/app.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/notifications/app.coffee b/services/notifications/app.coffee index 7008c814b0..4a8136cb92 100644 --- a/services/notifications/app.coffee +++ b/services/notifications/app.coffee @@ -24,7 +24,7 @@ app.post '/user/:user_id', controller.addNotification app.get '/user/:user_id', controller.getUserNotifications app.del '/user/:user_id/notification/:notification_id', controller.removeNotificationId app.del '/user/:user_id', controller.removeNotificationKey -app.del '/notification/key/:key', controller.removeNotificationByKeyOnly +app.del '/key/:key', controller.removeNotificationByKeyOnly app.get '/status', (req, res)-> res.send('notifications sharelatex up') From 88f5f29982e34b86bc0d23f690b5eae6c335d5c1 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 12 Aug 2016 11:38:17 +0100 Subject: [PATCH 7/8] Change how notification expiry works Set the `expires` field to be a date in the future, after which the document should be removed. --- services/notifications/README.md | 4 ++-- .../notifications/app/coffee/Notifications.coffee | 10 +++++----- .../test/unit/coffee/NotificationsTests.coffee | 12 +++++------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/services/notifications/README.md b/services/notifications/README.md index 9db344dae4..4b2b4184b0 100644 --- a/services/notifications/README.md +++ b/services/notifications/README.md @@ -7,8 +7,8 @@ An API for managing user notifications in ShareLaTeX database indexes ================ -For notification expiry to work, a ttl index on `notifications.expiresFrom` must be created: +For notification expiry to work, a TTL index on `notifications.expires` must be created: ```javascript -db.notifications.createIndex({expiresFrom: 1}, {expireAfterSeconds: (60*60*24*30)}) +db.notifications.createIndex({expires: 1}, {expireAfterSeconds: 10}) ``` diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index a9053734b9..2a7e043f78 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -27,12 +27,12 @@ module.exports = key: notification.key messageOpts: notification.messageOpts templateKey: notification.templateKey - # ttl index on `expiresFrom` field - # in Mongo, TTL indexes only work on date fields, - # and ignore the document when that field is missing + # TTL index on the optional `expires` field, which should arrive as an iso date-string, corresponding to + # a datetime in the future when the document should be automatically removed. + # in Mongo, TTL indexes only work on date fields, and ignore the document when that field is missing + # see `README.md` for instruction on creating TTL index if notification.expires? - doc.expires = notification.expires - doc.expiresFrom = new Date() + doc.expires = new Date(notification.expires) db.notifications.insert(doc, callback) removeNotificationId: (user_id, notification_id, callback)-> diff --git a/services/notifications/test/unit/coffee/NotificationsTests.coffee b/services/notifications/test/unit/coffee/NotificationsTests.coffee index 51215cb697..144b8407be 100644 --- a/services/notifications/test/unit/coffee/NotificationsTests.coffee +++ b/services/notifications/test/unit/coffee/NotificationsTests.coffee @@ -55,7 +55,7 @@ describe 'Notifications Tests', -> user_id: @stubbedNotification.user_id, key:"notification-key", messageOpts:"some info", - templateKey:"template-key", + templateKey:"template-key" } it 'should insert the notification into the collection', (done)-> @@ -81,25 +81,23 @@ describe 'Notifications Tests', -> key:"notification-key", messageOpts:"some info", templateKey:"template-key", - expires: true + expires: '2922-02-13T09:32:56.289Z' } @expectedDocument = { user_id: @stubbedNotification.user_id, key:"notification-key", messageOpts:"some info", templateKey:"template-key", - expires: true, - expiresFrom: new Date() + expires: new Date(@stubbedNotification.expires), } - it 'should add an `expiresFrom` Date field to the inserted notification', (done)-> + it 'should add an `expires` Date field to the document', (done)-> @insertStub.callsArgWith(1, null) @countStub.callsArgWith(1, null, 0) @notifications.addNotification user_id, @stubbedNotification, (err)=> @insertStub.callCount.should.equal 1 - Object.keys(@insertStub.lastCall.args[0]).should.deep.equal Object.keys(@expectedDocument) - @insertStub.firstCall.args[0].expiresFrom.should.be.instanceof Date + @insertStub.calledWith(@expectedDocument).should.equal true done() describe 'removeNotificationId', -> From bea41977323049fefbe5a502f720ff5416488aa3 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Mon, 15 Aug 2016 08:54:45 +0100 Subject: [PATCH 8/8] return an error if `expires` is not a valid date. --- .../app/coffee/Notifications.coffee | 7 +++- .../unit/coffee/NotificationsTests.coffee | 32 ++++++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index 2a7e043f78..e8edae5aab 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -32,7 +32,12 @@ module.exports = # in Mongo, TTL indexes only work on date fields, and ignore the document when that field is missing # see `README.md` for instruction on creating TTL index if notification.expires? - doc.expires = new Date(notification.expires) + try + doc.expires = new Date(notification.expires) + _testValue = doc.expires.toISOString() + catch err + logger.error {user_id, expires: notification.expires}, "error converting `expires` field to Date" + return callback(err) db.notifications.insert(doc, callback) removeNotificationId: (user_id, notification_id, callback)-> diff --git a/services/notifications/test/unit/coffee/NotificationsTests.coffee b/services/notifications/test/unit/coffee/NotificationsTests.coffee index 144b8407be..f070889de4 100644 --- a/services/notifications/test/unit/coffee/NotificationsTests.coffee +++ b/services/notifications/test/unit/coffee/NotificationsTests.coffee @@ -28,7 +28,10 @@ describe 'Notifications Tests', -> @mongojs.ObjectId = ObjectId @notifications = SandboxedModule.require modulePath, requires: - 'logger-sharelatex': log:-> + 'logger-sharelatex': { + log:()-> + error:()-> + } 'settings-sharelatex': {} 'mongojs':@mongojs @@ -100,6 +103,33 @@ describe 'Notifications Tests', -> @insertStub.calledWith(@expectedDocument).should.equal true done() + describe 'when the notification has a nonsensical expires field', () -> + beforeEach -> + @stubbedNotification = { + user_id: ObjectId(user_id), + key:"notification-key", + messageOpts:"some info", + templateKey:"template-key", + expires: 'WAT' + } + @expectedDocument = { + user_id: @stubbedNotification.user_id, + key:"notification-key", + messageOpts:"some info", + templateKey:"template-key", + expires: new Date(@stubbedNotification.expires), + } + + it 'should produce an error', (done)-> + @insertStub.callsArgWith(1, null) + @countStub.callsArgWith(1, null, 0) + + @notifications.addNotification user_id, @stubbedNotification, (err)=> + (err instanceof Error).should.equal true + @insertStub.callCount.should.equal 0 + @insertStub.calledWith(@expectedDocument).should.equal false + done() + describe 'removeNotificationId', -> it 'should mark the notification id as read', (done)-> @updateStub.callsArgWith(2, null)