Merge pull request #1 from sharelatex/pr-remove-by-key

Remove notifications by their key alone

services/notifications/.nvmrc

@@ -0,0 +1 @@
+4.2

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.expires` must be created:
+
+```javascript
+db.notifications.createIndex({expires: 1}, {expireAfterSeconds: 10})
+```

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 '/key/:key', controller.removeNotificationByKeyOnly
 
 app.get '/status', (req, res)->
 	res.send('notifications sharelatex up')

services/notifications/app/coffee/Notifications.coffee

@@ -22,25 +22,43 @@ 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
 					templateKey: notification.templateKey
+				# 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?
+					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)->
-		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

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()

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()
+				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()

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')
@@ -17,7 +18,6 @@ describe 'Notifications Tests', ->
 		@insertStub = sinon.stub()
 		@countStub = sinon.stub()
 		@updateStub = sinon.stub()
-		@findStub.cock = "helllo"
 		@mongojs = =>
 			notifications:
 				update: self.mongojsUpdate
@@ -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
 
@@ -44,12 +47,26 @@ describe 'Notifications Tests', ->
 				done()
 
 	describe 'addNotification', ->
-		it 'should insert the notification into the collectionssss', (done)->
+		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"
+			}
+
+		it 'should insert the notification into the collection', (done)->
 			@insertStub.callsArgWith(1, null)
 			@countStub.callsArgWith(1, null, 0)
 
 			@notifications.addNotification user_id, @stubbedNotification, (err)=>
-				assert.deepEqual(@insertStub.args[0][0], @stubbedNotification)
+				@insertStub.calledWith(@expectedDocument).should.equal true
 				done()
 
 		it 'should fail insert of existing notification key', (done)->
@@ -57,18 +74,71 @@ 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: '2922-02-13T09:32:56.289Z'
+				}
+				@expectedDocument = {
+					user_id: @stubbedNotification.user_id,
+					key:"notification-key",
+					messageOpts:"some info",
+					templateKey:"template-key",
+					expires: new Date(@stubbedNotification.expires),
+				}
+
+			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
+					@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)
 
 			@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}
 				assert.deepEqual(@updateStub.args[0][0], searchOps)
 				assert.deepEqual(@updateStub.args[0][1], updateOperation)
@@ -79,10 +149,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}
 				assert.deepEqual(@updateStub.args[0][0], searchOps)
 				assert.deepEqual(@updateStub.args[0][1], updateOperation)