diff --git a/services/document-updater/app.coffee b/services/document-updater/app.coffee index 20170cdd34..9026f653cb 100644 --- a/services/document-updater/app.coffee +++ b/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") diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 4f02bbcf5c..9ac651b5e7 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/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 diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index 683b94230f..8448361930 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/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 diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index 25da4ec9db..ee94933b8d 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/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 diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 7a679bb6e3..e31b84f051 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/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 diff --git a/services/document-updater/test/acceptance/coffee/RangesTests.coffee b/services/document-updater/test/acceptance/coffee/RangesTests.coffee index 0849d4551f..044fd3191f 100644 --- a/services/document-updater/test/acceptance/coffee/RangesTests.coffee +++ b/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() \ No newline at end of file diff --git a/services/document-updater/test/acceptance/coffee/helpers/DocUpdaterClient.coffee b/services/document-updater/test/acceptance/coffee/helpers/DocUpdaterClient.coffee index afcbfd4b45..d2e8dbe51d 100644 --- a/services/document-updater/test/acceptance/coffee/helpers/DocUpdaterClient.coffee +++ b/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 \ No newline at end of file + 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 diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 3f0279b5c7..b7ea49ffc9 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/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 - \ No newline at end of file + + 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 \ No newline at end of file diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index 4000b402aa..859a0d1089 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/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