Handle duplicate-notification race condition when using forceCreate

services/notifications/app/coffee/Notifications.coffee

@@ -15,25 +15,18 @@ module.exports = Notifications =
 			callback err, notifications
 
 
-	_checkExistingNotifcationAndOverride : (user_id, notification, callback)->
+	_countExistingNotifications : (user_id, notification, callback = (err, count)->)->
-		self = @
 		query =
 			user_id: ObjectId(user_id)
 			key: notification.key
-		db.notifications.count query, (err, number)->
+		db.notifications.count query, (err, count)->
-			if number > 0 and !notification.forceCreate
+			return callback(err) if err?
-				logger.log number:number, user_id:user_id, key:notification.key, "alredy has notification key for user"
+			callback(null, count)
-				return callback(number)
-			else if number > 0 and notification.forceCreate
-				self.deleteNotificationByKeyOnly notification.key, callback
-			else
-				callback()
 
 	addNotification: (user_id, notification, callback)->
-		@_checkExistingNotifcationAndOverride user_id, notification, (err)->
+		@_countExistingNotifications user_id, notification, (err, count)->
-			if err?
+			return callback(err) if err?
-				callback err
+			return callback() unless count == 0 || notification.forceCreate
-			else
 			doc =
 				user_id: ObjectId(user_id)
 				key: notification.key
@@ -50,7 +43,7 @@ module.exports = Notifications =
 				catch err
 					logger.error {user_id, expires: notification.expires}, "error converting `expires` field to Date"
 					return callback(err)
-				db.notifications.insert(doc, callback)
+			db.notifications.update({user_id: doc.user_id, key: notification.key}, doc, {upsert: true}, callback)
 
 	removeNotificationId: (user_id, notification_id, callback)->
 		searchOps =

services/notifications/test/unit/coffee/NotificationsTests.coffee

@@ -29,7 +29,8 @@ describe 'Notifications Tests', ->
 				remove: @removeStub
 		@mongojs.ObjectId = ObjectId
 
-		@notifications = SandboxedModule.require modulePath, requires:
+		@notifications = SandboxedModule.require modulePath,
+			requires:
 				'logger-sharelatex': {
 					log:()->
 					error:()->
@@ -37,6 +38,8 @@ describe 'Notifications Tests', ->
 				'settings-sharelatex': {}
 				'mongojs':@mongojs
 				'metrics-sharelatex': {timeAsyncMethod: sinon.stub()}
+			globals:
+				console: console
 
 		@stubbedNotification = {user_id: ObjectId(user_id), key:"notification-key", messageOpts:"some info", templateKey:"template-key"}
 		@stubbedNotificationArray = [@stubbedNotification]
@@ -63,33 +66,34 @@ describe 'Notifications Tests', ->
 				messageOpts:"some info",
 				templateKey:"template-key"
 			}
-			@notifications.deleteNotificationByKeyOnly = sinon.stub()
+			@expectedQuery = {
-			@notifications.deleteNotificationByKeyOnly.callsArgWith(1, null)
+				user_id: @stubbedNotification.user_id,
+				key:"notification-key",
+			}
+			@updateStub.yields()
+			@countStub.yields(null, 0)
 
 		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.lastCall.args[0], @expectedDocument)
+				expect(err).not.exists
+				sinon.assert.calledWith(@updateStub, @expectedQuery, @expectedDocument, { upsert: true })
 				done()
 
-		it 'should fail insert of existing notification key', (done)->
+		describe 'when there is an existing notification', (done) ->
-			@insertStub.callsArgWith(1, null)
+			beforeEach ->
-			@countStub.callsArgWith(1, null, 1)
+				@countStub.yields(null, 1)
+
+			it 'should fail to insert', (done)->
 				@notifications.addNotification user_id, @stubbedNotification, (err)=>
-				@insertStub.calledWith(@expectedDocument).should.equal false
+					expect(err).not.exists
+					sinon.assert.notCalled(@updateStub)
 					done()
 
-		describe "when key already exists but forceCreate is passed", (done)->
+			it "should update the key if forceCreate is true", (done) ->
-
-			it "should delete the old key and insert the new one", (done) ->
-				@insertStub.callsArgWith(1, null)
-				@countStub.callsArgWith(1, null, 1)
 				@stubbedNotification.forceCreate = true
 				@notifications.addNotification user_id, @stubbedNotification, (err)=>
-					assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument)
+					expect(err).not.exists
-					@notifications.deleteNotificationByKeyOnly.calledWith(@stubbedNotification.key).should.equal true
+					sinon.assert.calledWith(@updateStub, @expectedQuery, @expectedDocument, { upsert: true })
 					done()
 
 		describe 'when the notification is set to expire', () ->
@@ -108,14 +112,15 @@ describe 'Notifications Tests', ->
 					templateKey:"template-key",
 					expires: new Date(@stubbedNotification.expires),
 				}
+				@expectedQuery = {
+					user_id: @stubbedNotification.user_id,
+					key:"notification-key",
+				}
 
 			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
+					expect(err).not.exists
-					assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument)
+					sinon.assert.calledWith(@updateStub, @expectedQuery, @expectedDocument, { upsert: true })
 					done()
 
 		describe 'when the notification has a nonsensical expires field', () ->
@@ -136,13 +141,9 @@ describe 'Notifications Tests', ->
 				}
 
 			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
+					sinon.assert.notCalled(@updateStub)
-					@insertStub.calledWith(@expectedDocument).should.equal false
 					done()
 
 	describe 'removeNotificationId', ->