From cb12e1c6f6574b0c1f6558fef7385e7ac7f6dfe0 Mon Sep 17 00:00:00 2001 From: Chrystal Griffiths Date: Fri, 8 Feb 2019 15:39:51 +0000 Subject: [PATCH 1/4] Send an empty string for every nameless user --- .../app/coffee/WebsocketController.coffee | 36 +++++++++---------- .../coffee/WebsocketControllerTests.coffee | 34 ++++++++++++++---- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index b51e86c495..734dfc3005 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -145,26 +145,24 @@ module.exports = WebsocketController = cursorData.id = client.id cursorData.user_id = user_id if user_id? cursorData.email = email if email? - if first_name or last_name - cursorData.name = if first_name && last_name - "#{first_name} #{last_name}" - else if first_name - first_name - else if last_name - last_name - ConnectedUsersManager.updateUserPosition(project_id, client.id, { - first_name: first_name, - last_name: last_name, - email: email, - _id: user_id - }, { - row: cursorData.row, - column: cursorData.column, - doc_id: cursorData.doc_id - }, callback) + cursorData.name = if first_name && last_name + "#{first_name} #{last_name}" + else if first_name + first_name + else if last_name + last_name else - cursorData.name = "Anonymous" - callback() + "" + ConnectedUsersManager.updateUserPosition(project_id, client.id, { + first_name: first_name, + last_name: last_name, + email: email, + _id: user_id + }, { + row: cursorData.row, + column: cursorData.column, + doc_id: cursorData.doc_id + }, callback) WebsocketLoadBalancer.emitToRoom(project_id, "clientTracking.clientUpdated", cursorData) getConnectedUsers: (client, callback = (error, users) ->) -> diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index b6a4069c67..0a0bd50843 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -426,7 +426,7 @@ describe 'WebsocketController', -> doc_id: @doc_id }).should.equal true done() - + it "should increment the update-client-position metric at 0.1 frequency", -> @metrics.inc.calledWith("editor.update-client-position", 0.1).should.equal true @@ -509,6 +509,30 @@ describe 'WebsocketController', -> it "should increment the update-client-position metric at 0.1 frequency", -> @metrics.inc.calledWith("editor.update-client-position", 0.1).should.equal true + describe "with a logged in user who has no names set", -> + beforeEach -> + @clientParams = { + project_id: @project_id + first_name: undefined + last_name: undefined + email: @email = "joe@example.com" + user_id: @user_id = "user-id-123" + } + @client.get = (param, callback) => callback null, @clientParams[param] + @WebsocketController.updateClientPosition @client, @update + + it "should send the update to the project name with no name", -> + @WebsocketLoadBalancer.emitToRoom + .calledWith(@project_id, "clientTracking.clientUpdated", { + doc_id: @doc_id, + id: @client.id, + user_id: @user_id, + name: "", + row: @row, + column: @column, + email: @email + }) + .should.equal true describe "with an anonymous user", -> @@ -519,20 +543,16 @@ describe 'WebsocketController', -> @client.get = (param, callback) => callback null, @clientParams[param] @WebsocketController.updateClientPosition @client, @update - it "should send the update to the project room with an anonymous name", -> + it "should send the update to the project room with no name", -> @WebsocketLoadBalancer.emitToRoom .calledWith(@project_id, "clientTracking.clientUpdated", { doc_id: @doc_id, id: @client.id - name: "Anonymous" + name: "" row: @row column: @column }) .should.equal true - - it "should not send cursor data to the connected user manager", (done)-> - @ConnectedUsersManager.updateUserPosition.called.should.equal false - done() describe "applyOtUpdate", -> beforeEach -> From 2ec760403f18a78cd13f3a2b8ead1d718ec03ed2 Mon Sep 17 00:00:00 2001 From: Chrystal Griffiths Date: Mon, 11 Feb 2019 11:52:14 +0000 Subject: [PATCH 2/4] Revert to method not sending cursorData because of duplication --- .../app/coffee/WebsocketController.coffee | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index 734dfc3005..e5c4f0c1c0 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -129,7 +129,6 @@ module.exports = WebsocketController = # after the initial joinDoc since we know they are already authorised. ## AuthorizationManager.removeAccessToDoc client, doc_id callback() - updateClientPosition: (client, cursorData, callback = (error) ->) -> metrics.inc "editor.update-client-position", 0.1 Utils.getClientAttributes client, [ @@ -145,24 +144,26 @@ module.exports = WebsocketController = cursorData.id = client.id cursorData.user_id = user_id if user_id? cursorData.email = email if email? - cursorData.name = if first_name && last_name - "#{first_name} #{last_name}" - else if first_name - first_name - else if last_name - last_name + if first_name or last_name + cursorData.name = if first_name && last_name + "#{first_name} #{last_name}" + else if first_name + first_name + else if last_name + last_name + ConnectedUsersManager.updateUserPosition(project_id, client.id, { + first_name: first_name, + last_name: last_name, + email: email, + _id: user_id + }, { + row: cursorData.row, + column: cursorData.column, + doc_id: cursorData.doc_id + }, callback) else - "" - ConnectedUsersManager.updateUserPosition(project_id, client.id, { - first_name: first_name, - last_name: last_name, - email: email, - _id: user_id - }, { - row: cursorData.row, - column: cursorData.column, - doc_id: cursorData.doc_id - }, callback) + cursorData.name = "" + callback() WebsocketLoadBalancer.emitToRoom(project_id, "clientTracking.clientUpdated", cursorData) getConnectedUsers: (client, callback = (error, users) ->) -> @@ -221,4 +222,4 @@ module.exports = WebsocketController = for op in update.op if !op.c? return false - return true + return true \ No newline at end of file From bb06f82e0433f8ca369b65191e56c1f44fcc1985 Mon Sep 17 00:00:00 2001 From: Chrystal Griffiths Date: Tue, 12 Feb 2019 14:00:47 +0000 Subject: [PATCH 3/4] Still send cursorData for logged in users --- .../app/coffee/WebsocketController.coffee | 19 +++++++++++-------- .../coffee/WebsocketControllerTests.coffee | 4 ++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index e5c4f0c1c0..b4a682f294 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -52,6 +52,10 @@ module.exports = WebsocketController = metrics.inc "editor.leave-project" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> return callback(error) if error? + + logger.log {project_id, user_id, client_id: client.id}, "client leaving project" + WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.id + # bail out if the client had not managed to authenticate or join # the project. Prevents downstream errors in docupdater from # flushProjectToMongoAndDelete with null project_id. @@ -61,9 +65,6 @@ module.exports = WebsocketController = if not project_id? logger.log {user_id: user_id, client_id: client.id}, "client leaving, not in project" return callback() - - logger.log {project_id, user_id, client_id: client.id}, "client leaving project" - WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.id # We can do this in the background ConnectedUsersManager.markUserAsDisconnected project_id, client.id, (err) -> @@ -144,13 +145,18 @@ module.exports = WebsocketController = cursorData.id = client.id cursorData.user_id = user_id if user_id? cursorData.email = email if email? - if first_name or last_name + if !user_id or user_id == 'anonymous-user' + cursorData.name = "" + callback() + else cursorData.name = if first_name && last_name "#{first_name} #{last_name}" else if first_name first_name else if last_name last_name + else + "" ConnectedUsersManager.updateUserPosition(project_id, client.id, { first_name: first_name, last_name: last_name, @@ -160,10 +166,7 @@ module.exports = WebsocketController = row: cursorData.row, column: cursorData.column, doc_id: cursorData.doc_id - }, callback) - else - cursorData.name = "" - callback() + }, callback) WebsocketLoadBalancer.emitToRoom(project_id, "clientTracking.clientUpdated", cursorData) getConnectedUsers: (client, callback = (error, users) ->) -> diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index 0a0bd50843..5b36057813 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -554,6 +554,10 @@ describe 'WebsocketController', -> }) .should.equal true + it "should not send cursor data to the connected user manager", (done)-> + @ConnectedUsersManager.updateUserPosition.called.should.equal false + done() + describe "applyOtUpdate", -> beforeEach -> @update = {op: {p: 12, t: "foo"}} From 26acdfd072ead1549d925288e0059630b5c33465 Mon Sep 17 00:00:00 2001 From: Chrystal Griffiths Date: Tue, 12 Feb 2019 14:06:59 +0000 Subject: [PATCH 4/4] Add comment explaining why not sending anon data up --- services/real-time/app/coffee/WebsocketController.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index b4a682f294..008a5560a7 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -137,7 +137,7 @@ module.exports = WebsocketController = ], (error, {project_id, first_name, last_name, email, user_id}) -> return callback(error) if error? logger.log {user_id, project_id, client_id: client.id, cursorData: cursorData}, "updating client position" - + AuthorizationManager.assertClientCanViewProjectAndDoc client, cursorData.doc_id, (error) -> if error? logger.warn {err: error, client_id: client.id, project_id, user_id}, "silently ignoring unauthorized updateClientPosition. Client likely hasn't called joinProject yet." @@ -145,10 +145,11 @@ module.exports = WebsocketController = cursorData.id = client.id cursorData.user_id = user_id if user_id? cursorData.email = email if email? + # Don't store anonymous users in redis to avoid influx if !user_id or user_id == 'anonymous-user' cursorData.name = "" callback() - else + else cursorData.name = if first_name && last_name "#{first_name} #{last_name}" else if first_name @@ -166,7 +167,7 @@ module.exports = WebsocketController = row: cursorData.row, column: cursorData.column, doc_id: cursorData.doc_id - }, callback) + }, callback) WebsocketLoadBalancer.emitToRoom(project_id, "clientTracking.clientUpdated", cursorData) getConnectedUsers: (client, callback = (error, users) ->) ->