From 575bdc62ec9566868f69b5d919832b5397b9a401 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 10 Sep 2015 14:32:18 +0100 Subject: [PATCH 1/7] Add a test for when the user can't be found. --- .../coffee/WebApiManager/WebApiManagerTests.coffee | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/services/track-changes/test/unit/coffee/WebApiManager/WebApiManagerTests.coffee b/services/track-changes/test/unit/coffee/WebApiManager/WebApiManagerTests.coffee index aec51ab2ca..177a7b26f0 100644 --- a/services/track-changes/test/unit/coffee/WebApiManager/WebApiManagerTests.coffee +++ b/services/track-changes/test/unit/coffee/WebApiManager/WebApiManagerTests.coffee @@ -72,6 +72,16 @@ describe "WebApiManager", -> .calledWith(new Error("web returned failure status code: 500")) .should.equal true + describe "when the user cannot be found", -> + beforeEach -> + @request.get = sinon.stub().callsArgWith(1, null, {statusCode: 404}, "nothing") + @WebApiManager.getUserInfo @user_id, @callback + + it "should return a null value", -> + @callback + .calledWith(null, null) + .should.equal true + describe "getProjectDetails", -> describe "successfully", -> From 810bddb2cbc6cdf1cdcbbccf80defe1a14f98c2c Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 10 Sep 2015 14:32:35 +0100 Subject: [PATCH 2/7] Log a message when the web api produces a 404 response. --- services/track-changes/app/coffee/WebApiManager.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/track-changes/app/coffee/WebApiManager.coffee b/services/track-changes/app/coffee/WebApiManager.coffee index 9264862fa3..d7441682e3 100644 --- a/services/track-changes/app/coffee/WebApiManager.coffee +++ b/services/track-changes/app/coffee/WebApiManager.coffee @@ -14,6 +14,7 @@ module.exports = WebApiManager = if error? return callback(error) if res.statusCode == 404 + logger.log url: url, "got 404 from web api" return callback null, null if res.statusCode >= 200 and res.statusCode < 300 return callback null, body From 8387383cb4a6a0773971f9f9292a87135550a35f Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 10 Sep 2015 14:32:47 +0100 Subject: [PATCH 3/7] In _summarizeUpdates, allow null users through. A null value represents a deleted or otherwise missing user record. --- .../app/coffee/UpdatesManager.coffee | 21 +++---- .../UpdatesManager/UpdatesManagerTests.coffee | 61 +++++++++++++++++++ 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/services/track-changes/app/coffee/UpdatesManager.coffee b/services/track-changes/app/coffee/UpdatesManager.coffee index 92357fc2dc..91a4877b35 100644 --- a/services/track-changes/app/coffee/UpdatesManager.coffee +++ b/services/track-changes/app/coffee/UpdatesManager.coffee @@ -188,14 +188,13 @@ module.exports = UpdatesManager = for update in updates earliestUpdate = summarizedUpdates[summarizedUpdates.length - 1] if earliestUpdate and earliestUpdate.meta.start_ts - update.meta.end_ts < @TIME_BETWEEN_DISTINCT_UPDATES - if update.meta.user? - userExists = false - for user in earliestUpdate.meta.users - if user.id == update.meta.user.id - userExists = true - break - if !userExists - earliestUpdate.meta.users.push update.meta.user + userExists = false + for user in earliestUpdate.meta.users + if (!user and !update.meta.user) or (user.id == update.meta.user?.id) + userExists = true + break + if !userExists + earliestUpdate.meta.users.push update.meta.user doc_id = update.doc_id.toString() doc = earliestUpdate.docs[doc_id] @@ -220,11 +219,7 @@ module.exports = UpdatesManager = newUpdate.docs[update.doc_id.toString()] = fromV: update.v toV: update.v - - if update.meta.user? - newUpdate.meta.users.push update.meta.user - + newUpdate.meta.users.push update.meta.user summarizedUpdates.push newUpdate return summarizedUpdates - diff --git a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee index fdc19360b7..545fa18ca0 100644 --- a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee +++ b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee @@ -627,3 +627,64 @@ describe "UpdatesManager", -> start_ts: @now end_ts: @now + 50 }] + + it "should include null user values", -> + result = @UpdatesManager._summarizeUpdates [{ + doc_id: "doc-id-1" + meta: + user: @user_1 + start_ts: @now + 20 + end_ts: @now + 30 + v: 5 + }, { + doc_id: "doc-id-1" + meta: + user: null + start_ts: @now + end_ts: @now + 10 + v: 4 + }] + expect(result).to.deep.equal [{ + docs: + "doc-id-1": + fromV: 4 + toV: 5 + meta: + users: [@user_1, null] + start_ts: @now + end_ts: @now + 30 + }] + + it "should roll several null user values into one", -> + result = @UpdatesManager._summarizeUpdates [{ + doc_id: "doc-id-1" + meta: + user: @user_1 + start_ts: @now + 20 + end_ts: @now + 30 + v: 5 + }, { + doc_id: "doc-id-1" + meta: + user: null + start_ts: @now + end_ts: @now + 10 + v: 4 + }, { + doc_id: "doc-id-1" + meta: + user: null + start_ts: @now + 2 + end_ts: @now + 4 + v: 4 + }] + expect(result).to.deep.equal [{ + docs: + "doc-id-1": + fromV: 4 + toV: 5 + meta: + users: [@user_1, null] + start_ts: @now + end_ts: @now + 30 + }] From 97326308fabf65f337d09c82f063e76a784d182e Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 10 Sep 2015 15:40:43 +0100 Subject: [PATCH 4/7] Update the Acceptance tests to include a case where a user doesn't exist. --- .../test/acceptance/coffee/GettingUpdatesTests.coffee | 6 +++--- .../test/acceptance/coffee/helpers/MockWebApi.coffee | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee b/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee index 6452649fc4..7c94bcdc34 100644 --- a/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee +++ b/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee @@ -15,6 +15,7 @@ describe "Getting updates", -> @now = Date.now() @to = @now @user_id = ObjectId().toString() + @deleted_user_id = 'deleted_user' @doc_id = ObjectId().toString() @project_id = ObjectId().toString() @@ -44,6 +45,7 @@ describe "Getting updates", -> meta: { ts: @now - (9 - i) * @hours, user_id: @user_id } v: 2 * i + 2 } + @updates[0].meta.user_id = @deleted_user_id TrackChangesClient.pushRawUpdates @project_id, @doc_id, @updates, (error) => throw error if error? @@ -91,7 +93,6 @@ describe "Getting updates", -> users: [@user] }] - describe "getting updates beyond the end of the database", -> before (done) -> TrackChangesClient.getUpdates @project_id, { before: @to - 8 * @hours + 1, min_count: 30 }, (error, body) => @@ -115,6 +116,5 @@ describe "Getting updates", -> meta: start_ts: @to - 9 * @hours - 2 * @minutes end_ts: @to - 9 * @hours - users: [@user] + users: [@user, null] }] - diff --git a/services/track-changes/test/acceptance/coffee/helpers/MockWebApi.coffee b/services/track-changes/test/acceptance/coffee/helpers/MockWebApi.coffee index f7cfc9e2e9..002d944dfd 100644 --- a/services/track-changes/test/acceptance/coffee/helpers/MockWebApi.coffee +++ b/services/track-changes/test/acceptance/coffee/helpers/MockWebApi.coffee @@ -7,7 +7,7 @@ module.exports = MockWebApi = projects: {} getUser: (user_id, callback = (error) ->) -> - callback null, @users[user_id] + callback null, @users[user_id] or null getProject: (project_id, callback = (error, project) ->) -> callback null, @projects[project_id] From 0ad374556dfabebe9bdb87e444cf5adf1e79de49 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 10 Sep 2015 16:43:40 +0100 Subject: [PATCH 5/7] Add a comment for clarity. --- services/track-changes/app/coffee/UpdatesManager.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/track-changes/app/coffee/UpdatesManager.coffee b/services/track-changes/app/coffee/UpdatesManager.coffee index 91a4877b35..49c2b6e1af 100644 --- a/services/track-changes/app/coffee/UpdatesManager.coffee +++ b/services/track-changes/app/coffee/UpdatesManager.coffee @@ -188,6 +188,8 @@ module.exports = UpdatesManager = for update in updates earliestUpdate = summarizedUpdates[summarizedUpdates.length - 1] if earliestUpdate and earliestUpdate.meta.start_ts - update.meta.end_ts < @TIME_BETWEEN_DISTINCT_UPDATES + # check if the user in this update is already present in the earliest update, + # if not, add them to the users list of the earliest update userExists = false for user in earliestUpdate.meta.users if (!user and !update.meta.user) or (user.id == update.meta.user?.id) From eab8b4b6c89f85a494be6f9017e0dbaec9fd502a Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 11 Sep 2015 14:07:06 +0100 Subject: [PATCH 6/7] Null safe access of `id` property, needed as user can be null. --- services/track-changes/app/coffee/UpdatesManager.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/track-changes/app/coffee/UpdatesManager.coffee b/services/track-changes/app/coffee/UpdatesManager.coffee index 49c2b6e1af..9bea5ba177 100644 --- a/services/track-changes/app/coffee/UpdatesManager.coffee +++ b/services/track-changes/app/coffee/UpdatesManager.coffee @@ -192,7 +192,7 @@ module.exports = UpdatesManager = # if not, add them to the users list of the earliest update userExists = false for user in earliestUpdate.meta.users - if (!user and !update.meta.user) or (user.id == update.meta.user?.id) + if (!user and !update.meta.user) or (user?.id == update.meta.user?.id) userExists = true break if !userExists From 39f528bcbcbc82cce5f137462a4d52668ce3330e Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 11 Sep 2015 14:12:01 +0100 Subject: [PATCH 7/7] Add a test to check that users are summarised properly even when a null user occurs earlier in the update list. --- .../UpdatesManager/UpdatesManagerTests.coffee | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee index 545fa18ca0..fc8853f2ab 100644 --- a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee +++ b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee @@ -655,6 +655,33 @@ describe "UpdatesManager", -> end_ts: @now + 30 }] + it "should include null user values, when the null is earlier in the updates list", -> + result = @UpdatesManager._summarizeUpdates [{ + doc_id: "doc-id-1" + meta: + user: null + start_ts: @now + end_ts: @now + 10 + v: 4 + }, { + doc_id: "doc-id-1" + meta: + user: @user_1 + start_ts: @now + 20 + end_ts: @now + 30 + v: 5 + }] + expect(result).to.deep.equal [{ + docs: + "doc-id-1": + fromV: 4 + toV: 5 + meta: + users: [null, @user_1] + start_ts: @now + end_ts: @now + 30 + }] + it "should roll several null user values into one", -> result = @UpdatesManager._summarizeUpdates [{ doc_id: "doc-id-1"