From f6def039bf6623ae75fed7e2736727fb32b16a33 Mon Sep 17 00:00:00 2001
From: Hayden Faulds <fauldsh@gmail.com>
Date: Mon, 16 Jul 2018 16:01:39 +0100
Subject: [PATCH] code review feedback

---
 .../Features/History/HistoryController.coffee | 79 ++++++++-----------
 services/web/app/coffee/router.coffee         |  6 +-
 2 files changed, 37 insertions(+), 48 deletions(-)

diff --git a/services/web/app/coffee/Features/History/HistoryController.coffee b/services/web/app/coffee/Features/History/HistoryController.coffee
index 4d4cb93b34..189e07b0f5 100644
--- a/services/web/app/coffee/Features/History/HistoryController.coffee
+++ b/services/web/app/coffee/Features/History/HistoryController.coffee
@@ -21,16 +21,12 @@ module.exports = HistoryController =
 				req.useProjectHistory = false
 			next()
 
-	ensureProjectHistoryEnabled: (req, res, next) ->
-		{project_id} = req.params
-		ProjectDetailsHandler.getDetails project_id, (err, project) ->
-			return next(err) if err?
-			historyEnabled = project.overleaf?.history?.id?
-			if historyEnabled
-				next()
-			else
-				logger.log {project_id}, "project history not enabled"
-				res.sendStatus(404)
+	ensureProjectHistoryEnabled: (req, res, next = (error) ->) ->
+		if req.useProjectHistory?
+			next()
+		else
+			logger.log {project_id}, "project history not enabled"
+			res.sendStatus(404)
 
 	proxyToHistoryApi: (req, res, next = (error) ->) ->
 		user_id = AuthenticationController.getLoggedInUserId req
@@ -52,22 +48,17 @@ module.exports = HistoryController =
 		user_id = AuthenticationController.getLoggedInUserId req
 		url = HistoryController.buildHistoryServiceUrl(req.useProjectHistory) + req.url
 		logger.log url: url, "proxying to history api"
-		request {
+		HistoryController._makeRequest {
 			url: url
 			method: req.method
 			json: true
 			headers:
 				"X-User-Id": user_id
-		}, (error, response, body) ->
+		}, (error, body) ->
 			return next(error) if error?
-			if 200 <= response.statusCode < 300
-				HistoryManager.injectUserDetails body, (error, data) ->
-					return next(error) if error?
-					res.json data
-			else
-				error = new Error("history api responded with non-success code: #{response.statusCode}")
-				logger.error err: error, user_id: user_id, "error proxying request to history api"
-				next(error)
+			HistoryManager.injectUserDetails body, (error, data) ->
+				return next(error) if error?
+				res.json data
 
 	buildHistoryServiceUrl: (useProjectHistory) ->
 		# choose a history service, either document-level (trackchanges)
@@ -110,47 +101,45 @@ module.exports = HistoryController =
 			}
 
 	getLabels: (req, res, next) ->
-		{project_id} = req.params
+		project_id = req.params.Project_id
 		user_id = AuthenticationController.getLoggedInUserId(req)
-		request.get {
+		HistoryController._makeRequest {
+			method: "GET"
 			url: "#{settings.apis.project_history.url}/project/#{project_id}/labels"
 			json: true
-		}, (error, response, body) ->
+		}, (error, labels) ->
 			return next(error) if error?
-			if 200 <= response.statusCode < 300
-				res.json body
-			else
-				error = new Error("history api responded with non-success code: #{response.statusCode}")
-				logger.error err: error, user_id: user_id, "error getting labels from project-history"
-				next(error)
+			res.json labels
 
 	createLabel: (req, res, next) ->
-		{project_id} = req.params
+		project_id = req.params.Project_id
 		{comment, version} = req.body
 		user_id = AuthenticationController.getLoggedInUserId(req)
-		request.post {
+		HistoryController._makeRequest {
+			method: "POST"
 			url: "#{settings.apis.project_history.url}/project/#{project_id}/user/#{user_id}/labels"
 			json: {comment, version}
-		}, (error, response, body) ->
+		}, (error, label) ->
 			return next(error) if error?
-			if 200 <= response.statusCode < 300
-				res.json body
-			else
-				error = new Error("history api responded with non-success code: #{response.statusCode}")
-				logger.error err: error, user_id: user_id, "error creating label in project-history"
-				next(error)
+			res.json label
 
 	deleteLabel: (req, res, next) ->
-		{project_id, label_id} = req.params
+		project_id = req.params.Project_id
+		label_id = req.params.label_id
 		user_id = AuthenticationController.getLoggedInUserId(req)
-		console.log "hof-debug DEL #{settings.apis.project_history.url}/project/#{project_id}/user/#{user_id}/labels/#{label_id}"
-		request.del {
+		HistoryController._makeRequest {
+			method: "DELETE"
 			url: "#{settings.apis.project_history.url}/project/#{project_id}/user/#{user_id}/labels/#{label_id}"
-		}, (error, response, body) ->
+		}, (error) ->
 			return next(error) if error?
+			res.sendStatus 204
+
+	_makeRequest: (options, callback) ->
+		request options, (error, response, body) ->
+			return callback(error) if error?
 			if 200 <= response.statusCode < 300
-				res.sendStatus 204
+				callback(null, body)
 			else
 				error = new Error("history api responded with non-success code: #{response.statusCode}")
-				logger.error err: error, user_id: user_id, "error deleting label in project-history"
-				next(error)
+				logger.error err: error, "project-history api responded with non-success code: #{response.statusCode}"
+				callback(error)
diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee
index d77af1fb96..9ae764781c 100644
--- a/services/web/app/coffee/router.coffee
+++ b/services/web/app/coffee/router.coffee
@@ -239,9 +239,9 @@ module.exports = class Router
 		webRouter.post "/project/:project_id/restore_file", AuthorizationMiddlewear.ensureUserCanWriteProjectContent, HistoryController.restoreFileFromV2
 		privateApiRouter.post "/project/:Project_id/history/resync", AuthenticationController.httpAuth, HistoryController.resyncProjectHistory
 
-		webRouter.get "/project/:project_id/labels", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.ensureProjectHistoryEnabled, HistoryController.getLabels
-		webRouter.post "/project/:project_id/labels", AuthorizationMiddlewear.ensureUserCanWriteProjectContent, HistoryController.ensureProjectHistoryEnabled, HistoryController.createLabel
-		webRouter.delete "/project/:project_id/labels/:label_id", AuthorizationMiddlewear.ensureUserCanWriteProjectContent, HistoryController.ensureProjectHistoryEnabled, HistoryController.deleteLabel
+		webRouter.get "/project/:Project_id/labels", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.ensureProjectHistoryEnabled, HistoryController.getLabels
+		webRouter.post "/project/:Project_id/labels", AuthorizationMiddlewear.ensureUserCanWriteProjectContent, HistoryController.selectHistoryApi, HistoryController.ensureProjectHistoryEnabled, HistoryController.createLabel
+		webRouter.delete "/project/:Project_id/labels/:label_id", AuthorizationMiddlewear.ensureUserCanWriteProjectContent, HistoryController.selectHistoryApi, HistoryController.ensureProjectHistoryEnabled, HistoryController.deleteLabel
 
 		webRouter.post '/project/:project_id/export/:brand_variation_id', AuthorizationMiddlewear.ensureUserCanAdminProject, ExportsController.exportProject
 		webRouter.get '/project/:project_id/export/:export_id', AuthorizationMiddlewear.ensureUserCanAdminProject, ExportsController.exportStatus