From 17a5dfa5a5f60c3538b2b4fa5640caa4935dfbe1 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 11 Jan 2018 11:31:41 +0000 Subject: [PATCH 1/2] Inject v2 user details into project-history updates and diffs --- .../Features/History/HistoryController.coffee | 17 +++ .../Features/History/HistoryManager.coffee | 53 ++++++++- services/web/app/coffee/router.coffee | 4 +- .../coffee/ide/history/HistoryManager.coffee | 10 +- .../ide/history/HistoryV2Manager.coffee | 15 +-- .../controllers/HistoryListController.coffee | 20 +--- .../history/util/displayNameForUser.coffee | 14 +++ .../History/HistoryControllerTests.coffee | 68 ++++++++++++ .../coffee/History/HistoryManagerTests.coffee | 104 ++++++++++++++++++ .../history}/HistoryManagerV2Tests.coffee | 2 +- .../ide/history/displayUserNameTests.coffee | 68 ++++++++++++ 11 files changed, 339 insertions(+), 36 deletions(-) create mode 100644 services/web/public/coffee/ide/history/util/displayNameForUser.coffee rename services/web/test/unit_frontend/coffee/{ => ide/history}/HistoryManagerV2Tests.coffee (97%) create mode 100644 services/web/test/unit_frontend/coffee/ide/history/displayUserNameTests.coffee diff --git a/services/web/app/coffee/Features/History/HistoryController.coffee b/services/web/app/coffee/Features/History/HistoryController.coffee index 76476c8248..f8f1578bd5 100644 --- a/services/web/app/coffee/Features/History/HistoryController.coffee +++ b/services/web/app/coffee/Features/History/HistoryController.coffee @@ -3,6 +3,7 @@ request = require "request" settings = require "settings-sharelatex" AuthenticationController = require "../Authentication/AuthenticationController" ProjectDetailsHandler = require "../Project/ProjectDetailsHandler" +HistoryManager = require "./HistoryManager" module.exports = HistoryController = selectHistoryApi: (req, res, next = (error) ->) -> @@ -33,6 +34,22 @@ module.exports = HistoryController = logger.error url: url, err: error, "history API error" next(error) + proxyToHistoryApiAndInjectUserDetails: (req, res, next = (error) ->) -> + user_id = AuthenticationController.getLoggedInUserId req + url = HistoryController.buildHistoryServiceUrl(req.useProjectHistory) + req.url + logger.log url: url, "proxying to history api" + request { + url: url + method: req.method + json: true + headers: + "X-User-Id": user_id + }, (error, response, body) -> + return next(error) if error? + HistoryManager.injectUserDetails body, (error, data) -> + return next(error) if error? + res.json data + buildHistoryServiceUrl: (useProjectHistory) -> # choose a history service, either document-level (trackchanges) # or project-level (project_history) diff --git a/services/web/app/coffee/Features/History/HistoryManager.coffee b/services/web/app/coffee/Features/History/HistoryManager.coffee index 75f552c907..85c16a9c92 100644 --- a/services/web/app/coffee/Features/History/HistoryManager.coffee +++ b/services/web/app/coffee/Features/History/HistoryManager.coffee @@ -1,5 +1,7 @@ request = require "request" settings = require "settings-sharelatex" +async = require 'async' +UserGetter = require "../User/UserGetter" module.exports = HistoryManager = initializeProject: (callback = (error, history_id) ->) -> @@ -23,4 +25,53 @@ module.exports = HistoryManager = callback null, { overleaf_id } else error = new Error("project-history returned a non-success status code: #{res.statusCode}") - callback error \ No newline at end of file + callback error + + injectUserDetails: (data, callback = (error, data_with_users) ->) -> + # data can be either: + # { + # diff: [{ + # i: "foo", + # meta: { + # users: ["user_id", { first_name: "James", ... }, ...] + # ... + # } + # }, ...] + # } + # or + # { + # updates: [{ + # pathnames: ["main.tex"] + # meta: { + # users: ["user_id", { first_name: "James", ... }, ...] + # ... + # }, + # ... + # }, ...] + # } + # Either way, the top level key points to an array of objects with a meta.users property + # that we need to replace user_ids with populated user objects. + # Note that some entries in the users arrays may already have user objects from the v1 history + # service + user_ids = new Set() + for entry in data.diff or data.updates or [] + for user in entry.meta?.users or [] + if typeof user == "string" + user_ids.add user + user_ids = Array.from(user_ids) + UserGetter.getUsers user_ids, { first_name: 1, last_name: 1, email: 1 }, (error, users_array) -> + return next(error) if error? + users = {} + for user in users_array or [] + users[user._id.toString()] = HistoryManager._userView(user) + for entry in data.diff or data.updates or [] + entry.meta?.users = (entry.meta?.users or []).map (user) -> + if typeof user == "string" + return users[user] + else + return user + callback null, data + + _userView: (user) -> + { _id, first_name, last_name, email } = user + return { first_name, last_name, email, id: _id } \ No newline at end of file diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 05770f7b67..072f5a1f6a 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -195,9 +195,9 @@ module.exports = class Router webRouter.post '/project/:Project_id/rename', AuthorizationMiddlewear.ensureUserCanAdminProject, ProjectController.renameProject - webRouter.get "/project/:Project_id/updates", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi + webRouter.get "/project/:Project_id/updates", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApiAndInjectUserDetails webRouter.get "/project/:Project_id/doc/:doc_id/diff", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi - webRouter.get "/project/:Project_id/diff", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi + webRouter.get "/project/:Project_id/diff", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApiAndInjectUserDetails webRouter.post "/project/:Project_id/doc/:doc_id/version/:version_id/restore", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi webRouter.get '/Project/:Project_id/download/zip', AuthorizationMiddlewear.ensureUserCanReadProject, ProjectDownloadsController.downloadProject diff --git a/services/web/public/coffee/ide/history/HistoryManager.coffee b/services/web/public/coffee/ide/history/HistoryManager.coffee index f896fbb8b4..8c77d97965 100644 --- a/services/web/public/coffee/ide/history/HistoryManager.coffee +++ b/services/web/public/coffee/ide/history/HistoryManager.coffee @@ -1,10 +1,11 @@ define [ "moment" "ide/colors/ColorManager" + "ide/history/util/displayNameForUser" "ide/history/controllers/HistoryListController" "ide/history/controllers/HistoryDiffController" "ide/history/directives/infiniteScroll" -], (moment, ColorManager) -> +], (moment, ColorManager, displayNameForUser) -> class HistoryManager constructor: (@ide, @$scope) -> @reset() @@ -165,12 +166,7 @@ define [ } if entry.i? or entry.d? - if entry.meta.user? - name = "#{entry.meta.user.first_name} #{entry.meta.user.last_name}" - else - name = "Anonymous" - if entry.meta.user?.id == @$scope.user.id - name = "you" + name = displayNameForUser(entry.meta.user) date = moment(entry.meta.end_ts).format("Do MMM YYYY, h:mm a") if entry.i? highlights.push { diff --git a/services/web/public/coffee/ide/history/HistoryV2Manager.coffee b/services/web/public/coffee/ide/history/HistoryV2Manager.coffee index 923cbd7cf0..f5124a71d9 100644 --- a/services/web/public/coffee/ide/history/HistoryV2Manager.coffee +++ b/services/web/public/coffee/ide/history/HistoryV2Manager.coffee @@ -1,10 +1,11 @@ define [ "moment" "ide/colors/ColorManager" + "ide/history/util/displayNameForUser" "ide/history/controllers/HistoryListController" "ide/history/controllers/HistoryDiffController" "ide/history/directives/infiniteScroll" -], (moment, ColorManager) -> +], (moment, ColorManager, displayNameForUser) -> class HistoryManager constructor: (@ide, @$scope) -> @reset() @@ -152,24 +153,20 @@ define [ } if entry.i? or entry.d? - if entry.meta.user? - name = "#{entry.meta.user.first_name} #{entry.meta.user.last_name}" - else - name = "Anonymous" - if entry.meta.user?.id == @$scope.user.id - name = "you" + user = entry.meta.users?[0] + name = displayNameForUser(user) date = moment(entry.meta.end_ts).format("Do MMM YYYY, h:mm a") if entry.i? highlights.push { label: "Added by #{name} on #{date}" highlight: range - hue: ColorManager.getHueForUserId(entry.meta.user?.id) + hue: ColorManager.getHueForUserId(user?.id) } else if entry.d? highlights.push { label: "Deleted by #{name} on #{date}" strikeThrough: range - hue: ColorManager.getHueForUserId(entry.meta.user?.id) + hue: ColorManager.getHueForUserId(user?.id) } return {text, highlights} diff --git a/services/web/public/coffee/ide/history/controllers/HistoryListController.coffee b/services/web/public/coffee/ide/history/controllers/HistoryListController.coffee index f6e7c724c3..c736014c8b 100644 --- a/services/web/public/coffee/ide/history/controllers/HistoryListController.coffee +++ b/services/web/public/coffee/ide/history/controllers/HistoryListController.coffee @@ -1,6 +1,7 @@ define [ - "base" -], (App) -> + "base", + "ide/history/util/displayNameForUser" +], (App, displayNameForUser) -> App.controller "HistoryPremiumPopup", ($scope, ide, sixpack)-> $scope.$watch "ui.view", -> @@ -114,18 +115,5 @@ define [ $scope.history.hoveringOverListSelectors = false $scope.resetHoverState() - $scope.displayName = (user) -> - if user.name? - full_name = user.name - else - full_name = "#{user.first_name} #{user.last_name}" - fallback_name = "Unknown" - if !user? - fallback_name - else if full_name != " " - full_name - else if user.email - user.email - else - fallback_name + $scope.displayName = displayNameForUser ] diff --git a/services/web/public/coffee/ide/history/util/displayNameForUser.coffee b/services/web/public/coffee/ide/history/util/displayNameForUser.coffee new file mode 100644 index 0000000000..0aef84172f --- /dev/null +++ b/services/web/public/coffee/ide/history/util/displayNameForUser.coffee @@ -0,0 +1,14 @@ +define [], () -> + return displayNameForUser = (user) -> + if !user? + return "Anonymous" + if user.id == window.user.id + return "you" + if user.name? + return user.name + name = [user.first_name, user.last_name].filter((n) -> n?).join(" ").trim() + if name == "" + name = user.email.split("@")[0] + if !name? or name == "" + return "?" + return name diff --git a/services/web/test/unit/coffee/History/HistoryControllerTests.coffee b/services/web/test/unit/coffee/History/HistoryControllerTests.coffee index f0669a5902..46bfa49f8f 100644 --- a/services/web/test/unit/coffee/History/HistoryControllerTests.coffee +++ b/services/web/test/unit/coffee/History/HistoryControllerTests.coffee @@ -14,6 +14,7 @@ describe "HistoryController", -> "request" : @request = sinon.stub() "settings-sharelatex": @settings = {} "logger-sharelatex": @logger = {log: sinon.stub(), error: sinon.stub()} + "./HistoryManager": @HistoryManager = {} "../Authentication/AuthenticationController": @AuthenticationController "../Project/ProjectDetailsHandler": @ProjectDetailsHandler = {} @settings.apis = @@ -114,3 +115,70 @@ describe "HistoryController", -> it "should pass the error up the call chain", -> @next.calledWith(@error).should.equal true + + describe "proxyToHistoryApiAndInjectUserDetails", -> + beforeEach -> + @req = { url: "/mock/url", method: "POST" } + @res = + json: sinon.stub() + @next = sinon.stub() + @request.yields(null, {}, @data = "mock-data") + @HistoryManager.injectUserDetails = sinon.stub().yields(null, @data_with_users = "mock-injected-data") + + describe "for a project with the project history flag", -> + beforeEach -> + @req.useProjectHistory = true + @HistoryController.proxyToHistoryApiAndInjectUserDetails @req, @res, @next + + it "should get the user id", -> + @AuthenticationController.getLoggedInUserId + .calledWith(@req) + .should.equal true + + it "should call the project history api", -> + @request + .calledWith({ + url: "#{@settings.apis.project_history.url}#{@req.url}" + method: @req.method + json: true + headers: + "X-User-Id": @user_id + }) + .should.equal true + + it "should inject the user data", -> + @HistoryManager.injectUserDetails + .calledWith(@data) + .should.equal true + + it "should return the data with users to the client", -> + @res.json.calledWith(@data_with_users).should.equal true + + describe "for a project without the project history flag", -> + beforeEach -> + @req.useProjectHistory = false + @HistoryController.proxyToHistoryApiAndInjectUserDetails @req, @res, @next + + it "should get the user id", -> + @AuthenticationController.getLoggedInUserId + .calledWith(@req) + .should.equal true + + it "should call the track changes api", -> + @request + .calledWith({ + url: "#{@settings.apis.trackchanges.url}#{@req.url}" + method: @req.method + json: true + headers: + "X-User-Id": @user_id + }) + .should.equal true + + it "should inject the user data", -> + @HistoryManager.injectUserDetails + .calledWith(@data) + .should.equal true + + it "should return the data with users to the client", -> + @res.json.calledWith(@data_with_users).should.equal true diff --git a/services/web/test/unit/coffee/History/HistoryManagerTests.coffee b/services/web/test/unit/coffee/History/HistoryManagerTests.coffee index 9b5f80df04..ab9d54b8d9 100644 --- a/services/web/test/unit/coffee/History/HistoryManagerTests.coffee +++ b/services/web/test/unit/coffee/History/HistoryManagerTests.coffee @@ -1,5 +1,6 @@ chai = require('chai') chai.should() +expect = chai.expect sinon = require("sinon") modulePath = "../../../../app/js/Features/History/HistoryManager" SandboxedModule = require('sandboxed-module') @@ -13,6 +14,7 @@ describe "HistoryManager", -> @HistoryManager = SandboxedModule.require modulePath, requires: "request" : @request = sinon.stub() "settings-sharelatex": @settings = {} + "../User/UserGetter": @UserGetter = {} @settings.apis = trackchanges: enabled: false @@ -84,3 +86,105 @@ describe "HistoryManager", -> it "should return the callback", -> @callback.calledWithExactly().should.equal true + + describe "injectUserDetails", -> + beforeEach -> + @user1 = { + _id: @user_id1 = "123456" + first_name: "Jane", + last_name: "Doe" + email: "jane@example.com" + } + @user1_view = { + id: @user_id1 + first_name: "Jane", + last_name: "Doe" + email: "jane@example.com" + } + @user2 = { + _id: @user_id2 = "abcdef" + first_name: "John", + last_name: "Doe" + email: "john@example.com" + } + @user2_view = { + id: @user_id2 + first_name: "John", + last_name: "Doe" + email: "john@example.com" + } + @UserGetter.getUsers = sinon.stub().yields(null, [@user1, @user2]) + + describe "with a diff", -> + it "should turn user_ids into user objects", (done) -> + @HistoryManager.injectUserDetails { + diff: [{ + i: "foo" + meta: + users: [@user_id1] + }, { + i: "bar" + meta: + users: [@user_id2] + }] + }, (error, diff) => + expect(error).to.be.null + expect(diff.diff[0].meta.users).to.deep.equal [@user1_view] + expect(diff.diff[1].meta.users).to.deep.equal [@user2_view] + done() + + it "should leave user objects", (done) -> + @HistoryManager.injectUserDetails { + diff: [{ + i: "foo" + meta: + users: [@user1_view] + }, { + i: "bar" + meta: + users: [@user_id2] + }] + }, (error, diff) => + expect(error).to.be.null + expect(diff.diff[0].meta.users).to.deep.equal [@user1_view] + expect(diff.diff[1].meta.users).to.deep.equal [@user2_view] + done() + + describe "with a list of updates", -> + it "should turn user_ids into user objects", (done) -> + @HistoryManager.injectUserDetails { + updates: [{ + fromV: 5 + toV: 8 + meta: + users: [@user_id1] + }, { + fromV: 4 + toV: 5 + meta: + users: [@user_id2] + }] + }, (error, updates) => + expect(error).to.be.null + expect(updates.updates[0].meta.users).to.deep.equal [@user1_view] + expect(updates.updates[1].meta.users).to.deep.equal [@user2_view] + done() + + it "should leave user objects", (done) -> + @HistoryManager.injectUserDetails { + updates: [{ + fromV: 5 + toV: 8 + meta: + users: [@user1_view] + }, { + fromV: 4 + toV: 5 + meta: + users: [@user_id2] + }] + }, (error, updates) => + expect(error).to.be.null + expect(updates.updates[0].meta.users).to.deep.equal [@user1_view] + expect(updates.updates[1].meta.users).to.deep.equal [@user2_view] + done() \ No newline at end of file diff --git a/services/web/test/unit_frontend/coffee/HistoryManagerV2Tests.coffee b/services/web/test/unit_frontend/coffee/ide/history/HistoryManagerV2Tests.coffee similarity index 97% rename from services/web/test/unit_frontend/coffee/HistoryManagerV2Tests.coffee rename to services/web/test/unit_frontend/coffee/ide/history/HistoryManagerV2Tests.coffee index 0087522603..d5e4101744 100644 --- a/services/web/test/unit_frontend/coffee/HistoryManagerV2Tests.coffee +++ b/services/web/test/unit_frontend/coffee/ide/history/HistoryManagerV2Tests.coffee @@ -1,6 +1,6 @@ Path = require 'path' SandboxedModule = require "sandboxed-module" -modulePath = Path.join __dirname, '../../../public/js/ide/history/HistoryV2Manager' +modulePath = Path.join __dirname, '../../../../../public/js/ide/history/HistoryV2Manager' sinon = require("sinon") expect = require("chai").expect diff --git a/services/web/test/unit_frontend/coffee/ide/history/displayUserNameTests.coffee b/services/web/test/unit_frontend/coffee/ide/history/displayUserNameTests.coffee new file mode 100644 index 0000000000..2a756171a7 --- /dev/null +++ b/services/web/test/unit_frontend/coffee/ide/history/displayUserNameTests.coffee @@ -0,0 +1,68 @@ +Path = require 'path' +SandboxedModule = require "sandboxed-module" +modulePath = Path.join __dirname, '../../../../../public/js/ide/history/util/displayNameForUser' +sinon = require("sinon") +expect = require("chai").expect + +describe "displayNameForUser", -> + beforeEach -> + SandboxedModule.require modulePath, globals: + "define": (dependencies, builder) => + @displayNameForUser = builder() + "window": @window = {} + @window.user = { id: 42 } + + it "should return 'Anonymous' with no user", -> + expect( + @displayNameForUser(null) + ).to.equal "Anonymous" + + it "should return 'you' when the user has the same id as the window", -> + expect( + @displayNameForUser({ + id: @window.user.id + email: "james.allen@overleaf.com" + first_name: "James" + last_name: "Allen" + }) + ).to.equal "you" + + it "should return the first_name and last_name when present", -> + expect( + @displayNameForUser({ + id: @window.user.id + 1 + email: "james.allen@overleaf.com" + first_name: "James" + last_name: "Allen" + }) + ).to.equal "James Allen" + + it "should return only the firstAname if no last_name", -> + expect( + @displayNameForUser({ + id: @window.user.id + 1 + email: "james.allen@overleaf.com" + first_name: "James" + last_name: "" + }) + ).to.equal "James" + + it "should return the email username if there are no names", -> + expect( + @displayNameForUser({ + id: @window.user.id + 1 + email: "james.allen@overleaf.com" + first_name: "" + last_name: "" + }) + ).to.equal "james.allen" + + it "should return the '?' if it has nothing", -> + expect( + @displayNameForUser({ + id: @window.user.id + 1 + email: "" + first_name: "" + last_name: "" + }) + ).to.equal "?" From 8edef2f94d09bb61f59512d870acf82015fa4782 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 11 Jan 2018 14:11:44 +0000 Subject: [PATCH 2/2] Fix next -> callback --- services/web/app/coffee/Features/History/HistoryManager.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/History/HistoryManager.coffee b/services/web/app/coffee/Features/History/HistoryManager.coffee index 85c16a9c92..bef9773a6b 100644 --- a/services/web/app/coffee/Features/History/HistoryManager.coffee +++ b/services/web/app/coffee/Features/History/HistoryManager.coffee @@ -60,7 +60,7 @@ module.exports = HistoryManager = user_ids.add user user_ids = Array.from(user_ids) UserGetter.getUsers user_ids, { first_name: 1, last_name: 1, email: 1 }, (error, users_array) -> - return next(error) if error? + return callback(error) if error? users = {} for user in users_array or [] users[user._id.toString()] = HistoryManager._userView(user)