From 1c68ea7328c313846d01ecbbb38455557c7f4076 Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 21 Jul 2014 13:16:07 +0100 Subject: [PATCH] Don't care if user key has expired when we receive a cursor update --- .../ConnectedUsersManager.coffee | 22 +++++---- .../Features/Editor/EditorController.coffee | 9 +++- .../ConnectedUsersManagerTests.coffee | 47 +++++++------------ .../Editor/EditorControllerTests.coffee | 18 ++++--- 4 files changed, 48 insertions(+), 48 deletions(-) diff --git a/services/web/app/coffee/Features/ConnectedUsers/ConnectedUsersManager.coffee b/services/web/app/coffee/Features/ConnectedUsers/ConnectedUsersManager.coffee index b3f18ec60f..7f20f3143e 100644 --- a/services/web/app/coffee/Features/ConnectedUsers/ConnectedUsersManager.coffee +++ b/services/web/app/coffee/Features/ConnectedUsers/ConnectedUsersManager.coffee @@ -19,18 +19,27 @@ buildUserKey = (project_id, client_id)-> return "connected_user:#{project_id}:#{ module.exports = - markUserAsConnected: (project_id, client_id, user, callback = (err)->)-> + # Use the same method for when a user connects, and when a user sends a cursor + # update. This way we don't care if the connected_user key has expired when + # we receive a cursor update. + updateUserPosition: (project_id, client_id, user, cursorData, callback = (err)->)-> logger.log project_id:project_id, client_id:client_id, "marking user as connected" multi = rclient.multi() - multi.sadd buildProjectSetKey(project_id), client_id + + multi.sadd buildProjectSetKey(project_id), client_id multi.expire buildProjectSetKey(project_id), FOUR_DAYS_IN_S - multi.hset buildUserKey(project_id, client_id), "connected_at", Date.now() + + multi.hset buildUserKey(project_id, client_id), "last_updated_at", Date.now() multi.hset buildUserKey(project_id, client_id), "user_id", user._id multi.hset buildUserKey(project_id, client_id), "first_name", user.first_name multi.hset buildUserKey(project_id, client_id), "last_name", user.last_name multi.hset buildUserKey(project_id, client_id), "email", user.email + + if cursorData? + multi.hset buildUserKey(project_id, client_id), "cursorData", JSON.stringify(cursorData) multi.expire buildUserKey(project_id, client_id), USER_TIMEOUT_IN_S + multi.exec (err)-> if err? logger.err err:err, project_id:project_id, client_id:client_id, "problem marking user as connected" @@ -58,13 +67,6 @@ module.exports = result.cursorData = JSON.parse(result.cursorData) callback err, result - setUserCursorPosition: (project_id, client_id, cursorData, callback)-> - multi = rclient.multi() - multi.hset buildUserKey(project_id, client_id), "cursorData", JSON.stringify(cursorData) - multi.expire buildUserKey(project_id, client_id), USER_TIMEOUT_IN_S - multi.exec callback - - getConnectedUsers: (project_id, callback)-> self = @ rclient.smembers buildProjectSetKey(project_id), (err, results)-> diff --git a/services/web/app/coffee/Features/Editor/EditorController.coffee b/services/web/app/coffee/Features/Editor/EditorController.coffee index 15960b3d84..2714e41603 100644 --- a/services/web/app/coffee/Features/Editor/EditorController.coffee +++ b/services/web/app/coffee/Features/Editor/EditorController.coffee @@ -59,7 +59,7 @@ module.exports = EditorController = callback null, ProjectEditorHandler.buildProjectModelView(project), privilegeLevel, EditorController.protocolVersion # can be done affter the connection has happened - ConnectedUsersManager.markUserAsConnected project_id, client.id, user, -> + ConnectedUsersManager.updateUserPosition project_id, client.id, user, null, -> leaveProject: (client, user) -> self = @ @@ -124,7 +124,12 @@ module.exports = EditorController = cursorData.email = email if email? if first_name? and last_name? cursorData.name = first_name + " " + last_name - ConnectedUsersManager.setUserCursorPosition(project_id, client.id, { + ConnectedUsersManager.updateUserPosition(project_id, client.id, { + first_name: first_name, + last_name: last_name, + email: email, + user_id: user_id + }, { row: cursorData.row, column: cursorData.column, doc_id: cursorData.doc_id diff --git a/services/web/test/UnitTests/coffee/ConnectedUsers/ConnectedUsersManagerTests.coffee b/services/web/test/UnitTests/coffee/ConnectedUsers/ConnectedUsersManagerTests.coffee index bdba5cd2a6..762aaa520d 100644 --- a/services/web/test/UnitTests/coffee/ConnectedUsers/ConnectedUsersManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/ConnectedUsers/ConnectedUsersManagerTests.coffee @@ -44,54 +44,60 @@ describe "ConnectedUsersManager", -> last_name: "Bloggs" email: "joe@example.com" } + @cursorData = { row: 12, column: 9, doc_id: '53c3b8c85fee64000023dc6e' } afterEach -> tk.reset() - describe "markUserAsConnected", -> + describe "updateUserPosition", -> beforeEach -> @rClient.exec.callsArgWith(0) it "should set a key with the date and give it a ttl", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> - @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "connected_at", Date.now()).should.equal true + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> + @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "last_updated_at", Date.now()).should.equal true done() it "should set a key with the user_id", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "user_id", @user._id).should.equal true done() it "should set a key with the first_name", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "first_name", @user.first_name).should.equal true done() it "should set a key with the last_name", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "last_name", @user.last_name).should.equal true done() it "should set a key with the email", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "email", @user.email).should.equal true done() it "should push the client_id on to the project list", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.sadd.calledWith("clients_in_project:#{@project_id}", @client_id).should.equal true done() - it "should add a ttl to the connected user set so it stays clean", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + it "should add a ttl to the project set so it stays clean", (done)-> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.expire.calledWith("clients_in_project:#{@project_id}", 24 * 4 * 60 * 60).should.equal true done() - it "should add a ttl to the connected user so it stays clean", (done)-> - @ConnectedUsersManager.markUserAsConnected @project_id, @client_id, @user, (err)=> + it "should add a ttl to the connected user so it stays clean", (done) -> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, null, (err)=> @rClient.expire.calledWith("connected_user:#{@project_id}:#{@client_id}", 60 * 15).should.equal true done() + it "should set the cursor position when provided", (done)-> + @ConnectedUsersManager.updateUserPosition @project_id, @client_id, @user, @cursorData, (err)=> + @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "cursorData", JSON.stringify(@cursorData)).should.equal true + done() + describe "markUserAsDisconnected", -> beforeEach -> @rClient.exec.callsArgWith(0) @@ -146,20 +152,3 @@ describe "ConnectedUsersManager", -> users[1].should.deep.equal {client_id:@users[2], connected:true} done() - describe "setUserCursorPosition", -> - - beforeEach -> - @cursorData = { row: 12, column: 9, doc_id: '53c3b8c85fee64000023dc6e' } - @rClient.exec.callsArgWith(0) - - it "should add the cursor data to the users hash", (done)-> - @ConnectedUsersManager.setUserCursorPosition @project_id, @client_id, @cursorData, (err)=> - @rClient.hset.calledWith("connected_user:#{@project_id}:#{@client_id}", "cursorData", JSON.stringify(@cursorData)).should.equal true - done() - - - it "should add the ttl on", (done)-> - @ConnectedUsersManager.setUserCursorPosition @project_id, @client_id, @cursorData, (err)=> - @rClient.expire.calledWith("connected_user:#{@project_id}:#{@client_id}", 60 * 15).should.equal true - done() - diff --git a/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee b/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee index b1f10d0a2f..d15b9c00c3 100644 --- a/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee @@ -55,8 +55,7 @@ describe "EditorController", -> deleteProject: sinon.stub() @ConnectedUsersManager = markUserAsDisconnected:sinon.stub() - markUserAsConnected:sinon.stub() - setUserCursorPosition:sinon.stub() + updateUserPosition:sinon.stub() @EditorController = SandboxedModule.require modulePath, requires: "../../infrastructure/Server" : io : @io @@ -91,7 +90,7 @@ describe "EditorController", -> @ProjectGetter.populateProjectWithUsers = sinon.stub().callsArgWith(1, null, @project) @AuthorizationManager.setPrivilegeLevelOnClient = sinon.stub() @EditorRealTimeController.emitToRoom = sinon.stub() - @ConnectedUsersManager.markUserAsConnected.callsArgWith(3) + @ConnectedUsersManager.updateUserPosition.callsArgWith(4) describe "when authorized", -> beforeEach -> @@ -124,7 +123,7 @@ describe "EditorController", -> @callback.calledWith(null, @projectModelView, "owner", @EditorController.protocolVersion).should.equal true it "should mark the user as connected with the ConnectedUsersManager", -> - @ConnectedUsersManager.markUserAsConnected.calledWith(@project_id, @client.id, @user).should.equal true + @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.id, @user, null).should.equal true describe "when not authorized", -> @@ -257,7 +256,7 @@ describe "EditorController", -> describe "updateClientPosition", -> beforeEach -> @EditorRealTimeController.emitToRoom = sinon.stub() - @ConnectedUsersManager.setUserCursorPosition.callsArgWith(3) + @ConnectedUsersManager.updateUserPosition.callsArgWith(4) @update = { doc_id: @doc_id = "doc-id-123" row: @row = 42 @@ -290,7 +289,12 @@ describe "EditorController", -> @EditorRealTimeController.emitToRoom.calledWith(@project_id, "clientTracking.clientUpdated", @populatedCursorData).should.equal true it "should send the cursor data to the connected user manager", (done)-> - @ConnectedUsersManager.setUserCursorPosition.calledWith(@project_id, @client.id, { + @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.id, { + user_id: @user_id, + email: @email, + first_name: @first_name, + last_name: @last_name + }, { row: @row column: @column doc_id: @doc_id @@ -317,7 +321,7 @@ describe "EditorController", -> .should.equal true it "should not send cursor data to the connected user manager", (done)-> - @ConnectedUsersManager.setUserCursorPosition.called.should.equal false + @ConnectedUsersManager.updateUserPosition.called.should.equal false done() describe "addUserToProject", ->