Merge pull request #256 from sharelatex/ja-display-v2-history-users

Inject v2 user details into project-history updates and diffs

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)

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
+				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 callback(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 }

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

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 {

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}

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
 	]

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

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

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()

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
 

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 "?"