From 4e66b045e38cb1b8bf354e3936994b48588e1c6e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 09:44:50 +0100 Subject: [PATCH 1/4] fix unhandled exception in ProjectDetailsHandler --- .../coffee/Features/Project/ProjectDetailsHandler.coffee | 2 +- .../coffee/Project/ProjectDetailsHandlerTests.coffee | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee index 7bd1d33561..a067a600d1 100644 --- a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee @@ -10,7 +10,7 @@ module.exports = getDetails: (project_id, callback)-> ProjectGetter.getProject project_id, {name:true, description:true, compiler:true, features:true, owner_ref:true}, (err, project)-> - if err? + if err? or !project? logger.err err:err, project_id:project_id, "error getting project" return callback(err) UserGetter.getUser project.owner_ref, (err, user) -> diff --git a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee index b9e8ca27c6..228319abf2 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee @@ -48,6 +48,13 @@ describe 'ProjectDetailsHandler', -> assert.equal(details.something, undefined) done() + it "should return an error for a non-existent project", (done)-> + @ProjectGetter.getProject.callsArg(2, null, null) + @handler.getDetails "0123456789012345678901234", (err, details)=> + assert.equal(err, undefined) + assert.equal(details, undefined) + done() + it "should return the error", (done)-> error = "some error" @ProjectGetter.getProject.callsArgWith(2, error) From f433510e619d116190d8f7b30fea3343fdf8cf5f Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 10:12:52 +0100 Subject: [PATCH 2/4] return NotFound error in ProjectDetailsHandler --- .../coffee/Features/Project/ProjectDetailsHandler.coffee | 4 +++- .../coffee/Project/ProjectDetailsHandlerTests.coffee | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee index a067a600d1..9c823c9f17 100644 --- a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee @@ -5,14 +5,16 @@ logger = require("logger-sharelatex") tpdsUpdateSender = require '../ThirdPartyDataStore/TpdsUpdateSender' _ = require("underscore") PublicAccessLevels = require("../Authorization/PublicAccessLevels") +Errors = require("../Errors/Errors") module.exports = getDetails: (project_id, callback)-> ProjectGetter.getProject project_id, {name:true, description:true, compiler:true, features:true, owner_ref:true}, (err, project)-> - if err? or !project? + if err? logger.err err:err, project_id:project_id, "error getting project" return callback(err) + return callback(new Errors.NotFoundError("project not found")) if !project? UserGetter.getUser project.owner_ref, (err, user) -> return callback(err) if err? details = diff --git a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee index 228319abf2..501e48f1a5 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee @@ -1,5 +1,6 @@ should = require('chai').should() modulePath = "../../../../app/js/Features/Project/ProjectDetailsHandler" +Errors = require "../../../../app/js/Features/Errors/Errors" SandboxedModule = require('sandboxed-module') sinon = require('sinon') assert = require("chai").assert @@ -50,9 +51,9 @@ describe 'ProjectDetailsHandler', -> it "should return an error for a non-existent project", (done)-> @ProjectGetter.getProject.callsArg(2, null, null) - @handler.getDetails "0123456789012345678901234", (err, details)=> - assert.equal(err, undefined) - assert.equal(details, undefined) + err = new Errors.NotFoundError("project not found") + @handler.getDetails "0123456789012345678901234", (error, details) => + err.should.eql error done() it "should return the error", (done)-> From 6002fdbad60b2dd3d42d8e6ce0537d6f20079a40 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 10:30:53 +0100 Subject: [PATCH 3/4] return 404 on project details not found --- .../coffee/Features/Project/ProjectApiController.coffee | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectApiController.coffee b/services/web/app/coffee/Features/Project/ProjectApiController.coffee index b16991ac62..715ccf19ae 100644 --- a/services/web/app/coffee/Features/Project/ProjectApiController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectApiController.coffee @@ -1,4 +1,5 @@ ProjectDetailsHandler = require("./ProjectDetailsHandler") +Errors = require("../Errors/Errors") logger = require("logger-sharelatex") @@ -7,8 +8,11 @@ module.exports = getProjectDetails : (req, res)-> {project_id} = req.params ProjectDetailsHandler.getDetails project_id, (err, projDetails)-> - if err? + if err? and err instanceof Errors.NotFoundError + return res.sendStatus 404 + else if err? logger.log err:err, project_id:project_id, "something went wrong getting project details" return res.sendStatus 500 - res.json(projDetails) + else + res.json(projDetails) From 835d8d618d21f0bd9098953c0570f589654915f9 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 11:33:37 +0100 Subject: [PATCH 4/4] use error handler --- .../Features/Project/ProjectApiController.coffee | 12 +++--------- .../coffee/Project/ProjectApiControllerTests.coffee | 9 ++++----- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectApiController.coffee b/services/web/app/coffee/Features/Project/ProjectApiController.coffee index 715ccf19ae..f832a75b54 100644 --- a/services/web/app/coffee/Features/Project/ProjectApiController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectApiController.coffee @@ -1,18 +1,12 @@ ProjectDetailsHandler = require("./ProjectDetailsHandler") -Errors = require("../Errors/Errors") logger = require("logger-sharelatex") module.exports = - getProjectDetails : (req, res)-> + getProjectDetails : (req, res, next)-> {project_id} = req.params ProjectDetailsHandler.getDetails project_id, (err, projDetails)-> - if err? and err instanceof Errors.NotFoundError - return res.sendStatus 404 - else if err? - logger.log err:err, project_id:project_id, "something went wrong getting project details" - return res.sendStatus 500 - else - res.json(projDetails) + return next(err) if err? + res.json(projDetails) diff --git a/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee index 9476a80019..ddf23c0bac 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee @@ -20,6 +20,7 @@ describe 'Project api controller', -> session: destroy:sinon.stub() @res = {} + @next = sinon.stub() @projDetails = {name:"something"} @@ -34,9 +35,7 @@ describe 'Project api controller', -> @controller.getProjectDetails @req, @res - it "should send a 500 if there is an error", (done)-> + it "should send a 500 if there is an error", ()-> @ProjectDetailsHandler.getDetails.callsArgWith(1, "error") - @res.sendStatus = (resCode)=> - resCode.should.equal 500 - done() - @controller.getProjectDetails @req, @res + @controller.getProjectDetails @req, @res, @next + @next.calledWith("error").should.equal true