From 77176255346b2db99b65391c6e258a0de72031d5 Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 5 Dec 2016 16:31:51 +0000 Subject: [PATCH 1/2] Include deleted docs when archiving --- .../app/coffee/DocArchiveManager.coffee | 2 +- .../docstore/app/coffee/DocManager.coffee | 4 +- .../docstore/app/coffee/HttpController.coffee | 2 +- .../docstore/app/coffee/MongoManager.coffee | 7 +++- .../test/unit/coffee/DocArchiveManager.coffee | 10 ++--- .../test/unit/coffee/DocManagerTests.coffee | 12 +++--- .../unit/coffee/HttpControllerTests.coffee | 8 ++-- .../test/unit/coffee/MongoManagerTests.coffee | 37 ++++++++++++++----- 8 files changed, 51 insertions(+), 31 deletions(-) diff --git a/services/docstore/app/coffee/DocArchiveManager.coffee b/services/docstore/app/coffee/DocArchiveManager.coffee index 5e0272466c..52af544df6 100644 --- a/services/docstore/app/coffee/DocArchiveManager.coffee +++ b/services/docstore/app/coffee/DocArchiveManager.coffee @@ -11,7 +11,7 @@ thirtySeconds = 30 * 1000 module.exports = DocArchive = archiveAllDocs: (project_id, callback = (err, docs) ->) -> - MongoManager.getProjectsDocs project_id, (err, docs) -> + MongoManager.getProjectsDocs project_id, true, (err, docs) -> if err? return callback(err) else if !docs? diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index e30370154e..6ac4678343 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -30,9 +30,9 @@ module.exports = DocManager = else callback err, doc - getAllDocs: (project_id, callback = (error, docs) ->) -> + getAllNonDeletedDocs: (project_id, callback = (error, docs) ->) -> DocArchive.unArchiveAllDocs project_id, (error) -> - MongoManager.getProjectsDocs project_id, (error, docs) -> + MongoManager.getProjectsDocs project_id, false, (error, docs) -> if err? return callback(error) else if !docs? diff --git a/services/docstore/app/coffee/HttpController.coffee b/services/docstore/app/coffee/HttpController.coffee index e87d89ea64..c1fa3ed3d3 100644 --- a/services/docstore/app/coffee/HttpController.coffee +++ b/services/docstore/app/coffee/HttpController.coffee @@ -35,7 +35,7 @@ module.exports = HttpController = getAllDocs: (req, res, next = (error) ->) -> project_id = req.params.project_id logger.log project_id: project_id, "getting all docs" - DocManager.getAllDocs project_id, (error, docs = []) -> + DocManager.getAllNonDeletedDocs project_id, (error, docs = []) -> return next(error) if error? docViews = [] for doc in docs diff --git a/services/docstore/app/coffee/MongoManager.coffee b/services/docstore/app/coffee/MongoManager.coffee index ef35756d0c..13551639d7 100644 --- a/services/docstore/app/coffee/MongoManager.coffee +++ b/services/docstore/app/coffee/MongoManager.coffee @@ -6,8 +6,11 @@ module.exports = MongoManager = db.docs.find {_id: ObjectId(doc_id.toString()), project_id: ObjectId(project_id.toString())}, {}, (error, docs = []) -> callback error, docs[0] - getProjectsDocs: (project_id, callback)-> - db.docs.find {project_id: ObjectId(project_id.toString()), deleted: { $ne: true }}, {}, callback + getProjectsDocs: (project_id, include_deleted, callback)-> + query = {project_id: ObjectId(project_id.toString())} + if !include_deleted + query.deleted = { $ne: true } + db.docs.find query, {}, callback getArchivedProjectDocs: (project_id, callback)-> query = diff --git a/services/docstore/test/unit/coffee/DocArchiveManager.coffee b/services/docstore/test/unit/coffee/DocArchiveManager.coffee index d55e870070..05abcc0fb3 100644 --- a/services/docstore/test/unit/coffee/DocArchiveManager.coffee +++ b/services/docstore/test/unit/coffee/DocArchiveManager.coffee @@ -64,7 +64,7 @@ describe "DocArchiveManager", -> @MongoManager = markDocAsArchived: sinon.stub().callsArgWith(2, null) upsertIntoDocCollection: sinon.stub().callsArgWith(3, null) - getProjectsDocs: sinon.stub().callsArgWith(1, null, @mongoDocs) + getProjectsDocs: sinon.stub().callsArgWith(2, null, @mongoDocs) getArchivedProjectDocs: sinon.stub().callsArgWith(1, null, @mongoDocs) @requires = @@ -127,7 +127,7 @@ describe "DocArchiveManager", -> describe "archiveAllDocs", -> it "should archive all project docs which are not in s3", (done)-> - @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, @mongoDocs) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(2, null, @mongoDocs) @DocArchiveManager.archiveDoc = sinon.stub().callsArgWith(2, null) @DocArchiveManager.archiveAllDocs @project_id, (err)=> @@ -142,14 +142,14 @@ describe "DocArchiveManager", -> done() it "should return error if have no docs", (done)-> - @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, null) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(2, null, null) @DocArchiveManager.archiveAllDocs @project_id, (err)=> should.exist err done() it "should return the error", (done)-> - @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, @error, null) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(2, @error, null) @DocArchiveManager.archiveAllDocs @project_id, (err)=> err.should.equal @error @@ -163,7 +163,7 @@ describe "DocArchiveManager", -> while --numberOfDocs != 0 @mongoDocs.push({inS3:true, _id: ObjectId()}) - @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, @mongoDocs) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(2, null, @mongoDocs) @DocArchiveManager.archiveDoc = sinon.stub().callsArgWith(2, null) it "should not throw and error", (done)-> diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index 58f26abaf3..a932e5dac9 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/services/docstore/test/unit/coffee/DocManagerTests.coffee @@ -96,17 +96,17 @@ describe "DocManager", -> .calledWith(new Errors.NotFoundError("No such doc: #{@doc_id} in project #{@project_id}")) .should.equal true - describe "getAllDocs", -> + describe "getAllNonDeletedDocs", -> describe "when the project exists", -> beforeEach -> @docs = [{ _id: @doc_id, project_id: @project_id, lines: ["mock-lines"] }] - @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, @docs) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(2, null, @docs) @DocArchiveManager.unArchiveAllDocs = sinon.stub().callsArgWith(1, null, @docs) - @DocManager.getAllDocs @project_id, @callback + @DocManager.getAllNonDeletedDocs @project_id, @callback it "should get the project from the database", -> @MongoManager.getProjectsDocs - .calledWith(@project_id) + .calledWith(@project_id, false) .should.equal true it "should return the docs", -> @@ -114,9 +114,9 @@ describe "DocManager", -> describe "when there are no docs for the project", -> beforeEach -> - @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, null) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(2, null, null) @DocArchiveManager.unArchiveAllDocs = sinon.stub().callsArgWith(1, null, null) - @DocManager.getAllDocs @project_id, @callback + @DocManager.getAllNonDeletedDocs @project_id, @callback it "should return a NotFoundError", -> @callback diff --git a/services/docstore/test/unit/coffee/HttpControllerTests.coffee b/services/docstore/test/unit/coffee/HttpControllerTests.coffee index a34237982e..592ee9c407 100644 --- a/services/docstore/test/unit/coffee/HttpControllerTests.coffee +++ b/services/docstore/test/unit/coffee/HttpControllerTests.coffee @@ -120,11 +120,11 @@ describe "HttpController", -> lines: ["mock", "lines", "two"] rev: 4 }] - @DocManager.getAllDocs = sinon.stub().callsArgWith(1, null, @docs) + @DocManager.getAllNonDeletedDocs = sinon.stub().callsArgWith(1, null, @docs) @HttpController.getAllDocs @req, @res, @next - it "should get all the docs", -> - @DocManager.getAllDocs + it "should get all the (non-deleted) docs", -> + @DocManager.getAllNonDeletedDocs .calledWith(@project_id) .should.equal true @@ -158,7 +158,7 @@ describe "HttpController", -> lines: ["mock", "lines", "two"] rev: 4 }] - @DocManager.getAllDocs = sinon.stub().callsArgWith(1, null, @docs) + @DocManager.getAllNonDeletedDocs = sinon.stub().callsArgWith(1, null, @docs) @HttpController.getAllDocs @req, @res, @next it "should return the non null docs as JSON", -> diff --git a/services/docstore/test/unit/coffee/MongoManagerTests.coffee b/services/docstore/test/unit/coffee/MongoManagerTests.coffee index 5064bcffbc..0576da3ddd 100644 --- a/services/docstore/test/unit/coffee/MongoManagerTests.coffee +++ b/services/docstore/test/unit/coffee/MongoManagerTests.coffee @@ -40,18 +40,35 @@ describe "MongoManager", -> @doc3 = { name: "mock-doc3" } @doc4 = { name: "mock-doc4" } @db.docs.find = sinon.stub().callsArgWith(2, null, [@doc, @doc3, @doc4]) - @MongoManager.getProjectsDocs @project_id, @callback + + describe "with included_deleted = false", -> + beforeEach -> + @MongoManager.getProjectsDocs @project_id, false, @callback - it "should find the non-deleted docs via the project_id", -> - @db.docs.find - .calledWith({ - project_id: ObjectId(@project_id) - deleted: { $ne: true } - }, {}) - .should.equal true + it "should find the non-deleted docs via the project_id", -> + @db.docs.find + .calledWith({ + project_id: ObjectId(@project_id) + deleted: { $ne: true } + }, {}) + .should.equal true - it "should call the callback with the docs", -> - @callback.calledWith(null, [@doc, @doc3, @doc4]).should.equal true + it "should call the callback with the docs", -> + @callback.calledWith(null, [@doc, @doc3, @doc4]).should.equal true + + describe "with included_deleted = true", -> + beforeEach -> + @MongoManager.getProjectsDocs @project_id, true, @callback + + it "should find all via the project_id", -> + @db.docs.find + .calledWith({ + project_id: ObjectId(@project_id) + }, {}) + .should.equal true + + it "should call the callback with the docs", -> + @callback.calledWith(null, [@doc, @doc3, @doc4]).should.equal true describe "upsertIntoDocCollection", -> beforeEach -> From 3d195de3e91000e8317e7a5969d36b85cfac8b7a Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 5 Dec 2016 16:48:01 +0000 Subject: [PATCH 2/2] Improve testing of archiving and be explicit about include_deleted parameter --- services/docstore/app/coffee/DocArchiveManager.coffee | 2 +- services/docstore/app/coffee/DocManager.coffee | 2 +- services/docstore/app/coffee/MongoManager.coffee | 4 ++-- .../test/acceptance/coffee/ArchiveDocsTests.coffee | 8 ++++++-- services/docstore/test/unit/coffee/DocManagerTests.coffee | 2 +- .../docstore/test/unit/coffee/MongoManagerTests.coffee | 4 ++-- 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/services/docstore/app/coffee/DocArchiveManager.coffee b/services/docstore/app/coffee/DocArchiveManager.coffee index 52af544df6..ffad78bc4e 100644 --- a/services/docstore/app/coffee/DocArchiveManager.coffee +++ b/services/docstore/app/coffee/DocArchiveManager.coffee @@ -11,7 +11,7 @@ thirtySeconds = 30 * 1000 module.exports = DocArchive = archiveAllDocs: (project_id, callback = (err, docs) ->) -> - MongoManager.getProjectsDocs project_id, true, (err, docs) -> + MongoManager.getProjectsDocs project_id, {include_deleted: true}, (err, docs) -> if err? return callback(err) else if !docs? diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index 6ac4678343..5a6d3961b9 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -32,7 +32,7 @@ module.exports = DocManager = getAllNonDeletedDocs: (project_id, callback = (error, docs) ->) -> DocArchive.unArchiveAllDocs project_id, (error) -> - MongoManager.getProjectsDocs project_id, false, (error, docs) -> + MongoManager.getProjectsDocs project_id, {include_deleted: false}, (error, docs) -> if err? return callback(error) else if !docs? diff --git a/services/docstore/app/coffee/MongoManager.coffee b/services/docstore/app/coffee/MongoManager.coffee index 13551639d7..2dd5b8b24c 100644 --- a/services/docstore/app/coffee/MongoManager.coffee +++ b/services/docstore/app/coffee/MongoManager.coffee @@ -6,9 +6,9 @@ module.exports = MongoManager = db.docs.find {_id: ObjectId(doc_id.toString()), project_id: ObjectId(project_id.toString())}, {}, (error, docs = []) -> callback error, docs[0] - getProjectsDocs: (project_id, include_deleted, callback)-> + getProjectsDocs: (project_id, options = {include_deleted: true}, callback)-> query = {project_id: ObjectId(project_id.toString())} - if !include_deleted + if !options.include_deleted query.deleted = { $ne: true } db.docs.find query, {}, callback diff --git a/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee b/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee index 26c57e7712..ef34901b6a 100644 --- a/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee @@ -31,6 +31,9 @@ describe "Archiving all docs", -> DocstoreClient.createDoc @project_id, doc._id, doc.lines, (err)=> doc.lines[0] = doc.lines[0]+" added" DocstoreClient.updateDoc @project_id, doc._id, doc.lines, null, callback + # Make sure archiving works on deleted docs too + jobs.push (cb) => + DocstoreClient.deleteDoc @project_id, @docs[2]._id, cb async.series jobs, done afterEach (done) -> @@ -141,10 +144,11 @@ describe "Archiving all docs", -> describe "Unarchiving all docs", -> it "should unarchive all the docs", (done) -> + non_deleted_docs = @docs.slice(0,2) DocstoreClient.archiveAllDoc @project_id, (error, res) => DocstoreClient.getAllDocs @project_id, (error, res, docs) => throw error if error? - docs.length.should.equal @docs.length - for doc, i in docs + docs.length.should.equal non_deleted_docs.length + for doc, i in non_deleted_docs doc.lines.should.deep.equal @docs[i].lines done() diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index a932e5dac9..e427ea9e79 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/services/docstore/test/unit/coffee/DocManagerTests.coffee @@ -106,7 +106,7 @@ describe "DocManager", -> it "should get the project from the database", -> @MongoManager.getProjectsDocs - .calledWith(@project_id, false) + .calledWith(@project_id, {include_deleted: false}) .should.equal true it "should return the docs", -> diff --git a/services/docstore/test/unit/coffee/MongoManagerTests.coffee b/services/docstore/test/unit/coffee/MongoManagerTests.coffee index 0576da3ddd..d330001e11 100644 --- a/services/docstore/test/unit/coffee/MongoManagerTests.coffee +++ b/services/docstore/test/unit/coffee/MongoManagerTests.coffee @@ -43,7 +43,7 @@ describe "MongoManager", -> describe "with included_deleted = false", -> beforeEach -> - @MongoManager.getProjectsDocs @project_id, false, @callback + @MongoManager.getProjectsDocs @project_id, include_deleted: false, @callback it "should find the non-deleted docs via the project_id", -> @db.docs.find @@ -58,7 +58,7 @@ describe "MongoManager", -> describe "with included_deleted = true", -> beforeEach -> - @MongoManager.getProjectsDocs @project_id, true, @callback + @MongoManager.getProjectsDocs @project_id, include_deleted: true, @callback it "should find all via the project_id", -> @db.docs.find