Merge pull request #14 from sharelatex/ja-modify-threads

Allow deleting of comment ranges

services/document-updater/app.coffee

@@ -46,6 +46,7 @@ app.delete '/project/:project_id/doc/:doc_id',       HttpController.flushAndDele
 app.delete '/project/:project_id',                   HttpController.deleteProject
 app.post   '/project/:project_id/flush',             HttpController.flushProject
 app.post   '/project/:project_id/doc/:doc_id/change/:change_id/accept', HttpController.acceptChange
+app.del    '/project/:project_id/doc/:doc_id/comment/:comment_id', HttpController.deleteComment
 
 app.get '/total', (req, res)->
 	timer = new Metrics.Timer("http.allDocList")	

services/document-updater/app/coffee/DocumentManager.coffee

@@ -136,6 +136,22 @@ module.exports = DocumentManager =
 				RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) ->
 					return callback(error) if error?
 					callback()
+	
+	deleteComment: (project_id, doc_id, comment_id, _callback = (error) ->) ->
+		timer = new Metrics.Timer("docManager.deleteComment")
+		callback = (args...) ->
+			timer.done()
+			_callback(args...)
+
+		DocumentManager.getDoc project_id, doc_id, (error, lines, version, ranges) ->
+			return callback(error) if error?
+			if !lines? or !version?
+				return callback(new Errors.NotFoundError("document not found: #{doc_id}"))
+			RangesManager.deleteComment comment_id, ranges, (error, new_ranges) ->
+				return callback(error) if error?
+				RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) ->
+					return callback(error) if error?
+					callback()
 
 	getDocWithLock: (project_id, doc_id, callback = (error, lines, version) ->) ->
 		UpdateManager = require "./UpdateManager"
@@ -160,3 +176,7 @@ module.exports = DocumentManager =
 	acceptChangeWithLock: (project_id, doc_id, change_id, callback = (error) ->) ->
 		UpdateManager = require "./UpdateManager"
 		UpdateManager.lockUpdatesAndDo DocumentManager.acceptChange, project_id, doc_id, change_id, callback
+
+	deleteCommentWithLock: (project_id, doc_id, thread_id, callback = (error) ->) ->
+		UpdateManager = require "./UpdateManager"
+		UpdateManager.lockUpdatesAndDo DocumentManager.deleteComment, project_id, doc_id, thread_id, callback

services/document-updater/app/coffee/HttpController.coffee

@@ -107,5 +107,15 @@ module.exports = HttpController =
 			return next(error) if error?
 			logger.log {project_id, doc_id, change_id}, "accepted change via http"
 			res.send 204 # No Content
+	
+	deleteComment: (req, res, next = (error) ->) ->
+		{project_id, doc_id, comment_id} = req.params
+		logger.log {project_id, doc_id, comment_id}, "deleting comment via http"
+		timer = new Metrics.Timer("http.deleteComment")
+		DocumentManager.deleteCommentWithLock project_id, doc_id, comment_id, (error) ->
+			timer.done()
+			return next(error) if error?
+			logger.log {project_id, doc_id, comment_id}, "deleted comment via http"
+			res.send 204 # No Content
 		
 

services/document-updater/app/coffee/RangesManager.coffee

@@ -30,6 +30,14 @@ module.exports = RangesManager =
 		rangesTracker.removeChangeId(change_id)
 		response = RangesManager._getRanges(rangesTracker)
 		callback null, response
+
+	deleteComment: (comment_id, ranges, callback = (error, ranges) ->) ->
+		{changes, comments} = ranges
+		logger.log {changes, comments, comment_id}, "deleting comment in ranges"
+		rangesTracker = new RangesTracker(changes, comments)
+		rangesTracker.removeCommentId(comment_id)
+		response = RangesManager._getRanges(rangesTracker)
+		callback null, response
 	
 	_getRanges: (rangesTracker) ->
 		# Return the minimal data structure needed, since most documents won't have any

services/document-updater/app/coffee/RangesTracker.coffee

@@ -105,7 +105,6 @@ load = (EventEmitter) ->
 				throw new Error("unknown op type")
 			
 		addComment: (op, metadata) ->
-			# TODO: Don't allow overlapping comments?
 			@comments.push comment = {
 				id: op.t or @newId()
 				op: # Copy because we'll modify in place

services/document-updater/test/acceptance/coffee/RangesTests.coffee

@@ -91,6 +91,7 @@ describe "Ranges", ->
 						ranges = data.ranges
 						comment = ranges.comments[0]
 						comment.op.should.deep.equal { c: "bar", p: 4, t: @tid }
+						comment.id.should.equal @tid
 						done()
 
 			describe "with conflicting ops needing OT", ->
@@ -223,4 +224,44 @@ describe "Ranges", ->
 				DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) =>
 					throw error if error?
 					expect(data.ranges.changes).to.be.undefined
+					done()
+
+	describe "deleting a comment range", ->
+		before (done) ->
+			@project_id = DocUpdaterClient.randomId()
+			@user_id = DocUpdaterClient.randomId()
+			@doc = {
+				id: DocUpdaterClient.randomId()
+				lines: ["foo bar"]
+			}
+			@update = {
+				doc: @doc.id
+				op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }]
+				v: 0
+			}
+			MockWebApi.insertDoc @project_id, @doc.id, {
+				lines: @doc.lines
+				version: 0
+			}
+			DocUpdaterClient.preloadDoc @project_id, @doc.id, (error) =>
+				throw error if error?
+				DocUpdaterClient.sendUpdate @project_id, @doc.id, @update, (error) =>
+					throw error if error?
+					setTimeout () =>
+						DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) =>
+							throw error if error?
+							ranges = data.ranges
+							change = ranges.comments[0]
+							change.op.should.deep.equal { c: "bar", p: 4, t: @tid }
+							change.id.should.equal @tid
+							done()
+					, 200
+		
+		it "should remove the comment range", (done) ->
+			DocUpdaterClient.removeComment @project_id, @doc.id, @tid, (error, res) =>
+				throw error if error?
+				expect(res.statusCode).to.equal 204
+				DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) =>
+					throw error if error?
+					expect(data.ranges.comments).to.be.undefined
 					done()

services/document-updater/test/acceptance/coffee/helpers/DocUpdaterClient.coffee

@@ -74,4 +74,7 @@ module.exports = DocUpdaterClient =
 		request.del "http://localhost:3003/project/#{project_id}", callback
 
 	acceptChange: (project_id, doc_id, change_id, callback = () ->) ->
-		request.post "http://localhost:3003/project/#{project_id}/doc/#{doc_id}/change/#{change_id}/accept", callback
+		request.post "http://localhost:3003/project/#{project_id}/doc/#{doc_id}/change/#{change_id}/accept", callback
+
+	removeComment: (project_id, doc_id, comment, callback = () ->) ->
+		request.del "http://localhost:3003/project/#{project_id}/doc/#{doc_id}/comment/#{comment}", callback

services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee

@@ -307,4 +307,48 @@ describe "DocumentManager", ->
 			it "should call the callback with a not found error", ->
 				error = new Errors.NotFoundError("document not found: #{@doc_id}")
 				@callback.calledWith(error).should.equal true
-			
+	
+	describe "deleteComment", ->
+		beforeEach ->
+			@comment_id = "mock-comment-id"
+			@version = 34
+			@lines = ["original", "lines"]
+			@ranges = { comments: ["one", "two", "three"] }
+			@updated_ranges = { comments: ["one", "three"] }
+			@DocumentManager.getDoc = sinon.stub().yields(null, @lines, @version, @ranges)
+			@RangesManager.deleteComment = sinon.stub().yields(null, @updated_ranges)
+			@RedisManager.updateDocument = sinon.stub().yields()
+		
+		describe "successfully", ->
+			beforeEach ->
+				@DocumentManager.deleteComment @project_id, @doc_id, @comment_id, @callback
+			
+			it "should get the document's current ranges", ->
+				@DocumentManager.getDoc
+					.calledWith(@project_id, @doc_id)
+					.should.equal true
+			
+			it "should delete the comment from the ranges", ->
+				@RangesManager.deleteComment
+					.calledWith(@comment_id, @ranges)
+					.should.equal true
+					
+			it "should save the updated ranges", ->
+				@RedisManager.updateDocument
+					.calledWith(@doc_id, @lines, @version, [], @updated_ranges)
+					.should.equal true
+			
+			it "should call the callback", ->
+				@callback.called.should.equal true
+
+		describe "when the doc is not found", ->
+			beforeEach ->
+				@DocumentManager.getDoc = sinon.stub().yields(null, null, null, null)
+				@DocumentManager.acceptChange @project_id, @doc_id, @comment_id, @callback
+
+			it "should not save anything", ->
+				@RedisManager.updateDocument.called.should.equal false
+			
+			it "should call the callback with a not found error", ->
+				error = new Errors.NotFoundError("document not found: #{@doc_id}")
+				@callback.calledWith(error).should.equal true

services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee

@@ -374,3 +374,44 @@ describe "HttpController", ->
 				@next
 					.calledWith(new Error("oops"))
 					.should.equal true
+	
+	describe "deleteComment", ->
+		beforeEach ->
+			@req =
+				params:
+					project_id: @project_id
+					doc_id: @doc_id
+					comment_id: @comment_id = "mock-comment-id"
+
+		describe "successfully", ->
+			beforeEach ->
+				@DocumentManager.deleteCommentWithLock = sinon.stub().callsArgWith(3)
+				@HttpController.deleteComment(@req, @res, @next)
+
+			it "should accept the change", ->
+				@DocumentManager.deleteCommentWithLock
+					.calledWith(@project_id, @doc_id, @comment_id)
+					.should.equal true
+
+			it "should return a successful No Content response", ->
+				@res.send
+					.calledWith(204)
+					.should.equal true
+
+			it "should log the request", ->
+				@logger.log
+					.calledWith({@project_id, @doc_id, @comment_id}, "deleting comment via http")
+					.should.equal true
+
+			it "should time the request", ->
+				@Metrics.Timer::done.called.should.equal true
+
+		describe "when an errors occurs", ->
+			beforeEach ->
+				@DocumentManager.deleteCommentWithLock = sinon.stub().callsArgWith(3, new Error("oops"))
+				@HttpController.deleteComment(@req, @res, @next)
+
+			it "should call next with the error", ->
+				@next
+					.calledWith(new Error("oops"))
+					.should.equal true