From f1e65cc7765b295280b1a6d469c4493b804d25a1 Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 29 Jan 2016 12:25:31 +0000 Subject: [PATCH] Use tag_id for deleting tags from project --- .../Features/Tags/TagsController.coffee | 19 ++--- .../coffee/Features/Tags/TagsHandler.coffee | 71 ++++++++----------- services/web/app/coffee/router.coffee | 1 + .../main/project-list/project-list.coffee | 10 +-- .../coffee/Tags/TagsControllerTests.coffee | 25 ++++--- .../coffee/Tags/TagsHandlerTests.coffee | 47 ++++++++---- 6 files changed, 100 insertions(+), 73 deletions(-) diff --git a/services/web/app/coffee/Features/Tags/TagsController.coffee b/services/web/app/coffee/Features/Tags/TagsController.coffee index 168adf1352..8536f7a114 100644 --- a/services/web/app/coffee/Features/Tags/TagsController.coffee +++ b/services/web/app/coffee/Features/Tags/TagsController.coffee @@ -6,20 +6,23 @@ module.exports = processTagsUpdate: (req, res)-> user_id = req.session.user._id project_id = req.params.project_id - if req.body.deletedTag? - tag = req.body.deletedTag - TagsHandler.removeProject user_id, project_id, tag, -> - res.send() - else - tag = req.body.tag - TagsHandler.addTag user_id, project_id, tag, -> - res.send() + tag = req.body.tag + TagsHandler.addTag user_id, project_id, tag, -> + res.send() logger.log user_id:user_id, project_id:project_id, body:req.body, "processing tag update" getAllTags: (req, res)-> TagsHandler.getAllTags req.session.user._id, (err, allTags)-> res.send(allTags) + removeProjectFromTag: (req, res, next) -> + user_id = req.session.user._id + {tag_id, project_id} = req.params + logger.log {user_id, tag_id, project_id}, "removing tag from project" + TagsHandler.removeProjectFromTag user_id, tag_id, project_id, (error) -> + return next(error) if error? + res.status(204).end() + deleteTag: (req, res, next) -> user_id = req.session.user._id tag_id = req.params.tag_id diff --git a/services/web/app/coffee/Features/Tags/TagsHandler.coffee b/services/web/app/coffee/Features/Tags/TagsHandler.coffee index d4593c9470..cc6b3d151d 100644 --- a/services/web/app/coffee/Features/Tags/TagsHandler.coffee +++ b/services/web/app/coffee/Features/Tags/TagsHandler.coffee @@ -3,47 +3,40 @@ settings = require("settings-sharelatex") request = require("request") logger = require("logger-sharelatex") -oneSecond = 1000 -module.exports = +TIMEOUT = 1000 +module.exports = TagsHandler = + _handleResponse: (res, params, callback) -> + if err? + params.err = err + logger.err params, "error in tag api" + return callback(err) + else if res? and res.statusCode >= 200 and res.statusCode < 300 + return callback(null) + else + err = new Error("tags api returned a failure status code: #{res?.statusCode}") + params.err = err + logger.err params, "tags api returned failure status code: #{res?.statusCode}" + return callback(err) + renameTag: (user_id, tag_id, name, callback = (error) ->) -> url = "#{settings.apis.tags.url}/user/#{user_id}/tag/#{tag_id}/rename" request.post { url: url json: name: name + timeout: TIMEOUT }, (err, res, body) -> - if err? - logger.err {err, user_id, tag_id, name}, "error renaming tag in tag api" - return callback(err) - else if res.statusCode >= 200 and res.statusCode < 300 - return callback(null) - else - err = new Error("tags api returned a failure status code: #{res.statusCode}") - logger.err {err, user_id, tag_id, name}, "tags api returned failure status code: #{res.statusCode}" - return callback(err) + TagsHandler._handleResponse res, {url, user_id, tag_id, name}, callback deleteTag: (user_id, tag_id, callback = (error) ->) -> url = "#{settings.apis.tags.url}/user/#{user_id}/tag/#{tag_id}" - request.del url, (err, res, body) -> - if err? - logger.err {err, user_id, tag_id}, "error deleting tag from tag api" - return callback(err) - else if res.statusCode >= 200 and res.statusCode < 300 - return callback(null) - else - err = new Error("tags api returned a failure status code: #{res.statusCode}") - logger.err {err, user_id, tag_id}, "tags api returned failure status code: #{res.statusCode}" - return callback(err) + request.del {url, timeout: TIMEOUT}, (err, res, body) -> + TagsHandler._handleResponse res, {url, user_id, tag_id}, callback - removeProject: (user_id, project_id, tag, callback)-> - uri = buildUri(user_id, project_id) - opts = - uri:uri - json: - name:tag - timeout:oneSecond - logger.log user_id:user_id, project_id:project_id, tag:tag, "send delete tag to tags api" - request.del opts, callback + removeProjectFromTag: (user_id, tag_id, project_id, callback)-> + url = "#{settings.apis.tags.url}/user/#{user_id}/tag/#{tag_id}/project/#{project_id}" + request.del {url, timeout: TIMEOUT}, (err, res, body) -> + TagsHandler._handleResponse res, {url, user_id, tag_id, project_id}, callback addTag: (user_id, project_id, tag, callback)-> uri = buildUri(user_id, project_id) @@ -51,23 +44,19 @@ module.exports = uri:uri json: name:tag - timeout:oneSecond + timeout: TIMEOUT logger.log user_id:user_id, project_id:project_id, tag:tag, "send add tag to tags api" request.post opts, callback requestTags: (user_id, callback)-> opts = - uri: "#{settings.apis.tags.url}/user/#{user_id}/tag" + url: "#{settings.apis.tags.url}/user/#{user_id}/tag" json: true - timeout: 2000 + timeout: TIMEOUT request.get opts, (err, res, body)-> - statusCode = if res? then res.statusCode else 500 - if err? or statusCode != 200 - e = new Error("something went wrong getting tags, #{err}, #{statusCode}") - logger.err err:err - callback(e, []) - else - callback(null, body) + TagsHandler._handleResponse res, {user_id}, (error) -> + return callback(error, []) if error? + callback(null, body or []) getAllTags: (user_id, callback)-> @requestTags user_id, (err, allTags)=> @@ -81,7 +70,7 @@ module.exports = uri = buildUri(user_id, project_id) opts = uri:"#{settings.apis.tags.url}/user/#{user_id}/project/#{project_id}" - timeout:oneSecond + timeout:TIMEOUT logger.log user_id:user_id, project_id:project_id, "removing project_id from tags" request.del opts, callback diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index a5c35a627f..d241525e8d 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -133,6 +133,7 @@ module.exports = class Router webRouter.get '/tag', AuthenticationController.requireLogin(), TagsController.getAllTags webRouter.post '/project/:project_id/tag', AuthenticationController.requireLogin(), TagsController.processTagsUpdate + webRouter.delete '/tag/:tag_id/project/:project_id', AuthenticationController.requireLogin(), TagsController.removeProjectFromTag webRouter.delete '/tag/:tag_id', AuthenticationController.requireLogin(), TagsController.deleteTag webRouter.post '/tag/:tag_id/rename', AuthenticationController.requireLogin(), TagsController.renameTag diff --git a/services/web/public/coffee/main/project-list/project-list.coffee b/services/web/public/coffee/main/project-list/project-list.coffee index 4e7efafa78..ca844b2072 100644 --- a/services/web/public/coffee/main/project-list/project-list.coffee +++ b/services/web/public/coffee/main/project-list/project-list.coffee @@ -169,10 +169,12 @@ define [ project.tags.splice(index, 1) for project_id in removed_project_ids - queuedHttp.post "/project/#{project_id}/tag", { - deletedTag: tag.name - _csrf: window.csrfToken - } + queuedHttp({ + method: "DELETE" + url: "/tag/#{tag._id}/project/#{project_id}" + headers: + "X-CSRF-Token": window.csrfToken + }) # If we're filtering by this tag then we need to remove # the projects from view diff --git a/services/web/test/UnitTests/coffee/Tags/TagsControllerTests.coffee b/services/web/test/UnitTests/coffee/Tags/TagsControllerTests.coffee index 2d0c0c8653..38908ec71d 100644 --- a/services/web/test/UnitTests/coffee/Tags/TagsControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Tags/TagsControllerTests.coffee @@ -13,7 +13,7 @@ describe 'Tags controller', -> beforeEach -> @handler = addTag: sinon.stub().callsArgWith(3) - removeProject: sinon.stub().callsArgWith(3) + removeProjectFromTag: sinon.stub().callsArgWith(3) deleteTag: sinon.stub().callsArg(2) renameTag: sinon.stub().callsArg(3) @controller = SandboxedModule.require modulePath, requires: @@ -39,13 +39,6 @@ describe 'Tags controller', -> @handler.addTag.calledWith(user_id, project_id, tag).should.equal true done() - - it 'should send a delete request when a delete has been recived with the body format standardised', (done)-> - @req.body = {deletedTag:tag} - @controller.processTagsUpdate @req, send:=> - @handler.removeProject.calledWith(user_id, project_id, tag).should.equal true - done() - describe "getAllTags", -> it 'should ask the handler for all tags', (done)-> allTags = [{name:"tag", projects:["123423","423423"]}] @@ -99,4 +92,20 @@ describe 'Tags controller', -> it "should return 400 (bad request) status code", -> @res.status.calledWith(400).should.equal true @res.end.called.should.equal true + + describe "removeProjectFromTag", -> + beforeEach -> + @req.params.tag_id = @tag_id = "tag-id-123" + @req.params.project_id = @project_id = "project-id-123" + @req.session.user._id = @user_id = "user-id-123" + @controller.removeProjectFromTag @req, @res + + it "should remove the tag from the project in the backend", -> + @handler.removeProjectFromTag + .calledWith(@user_id, @tag_id, @project_id) + .should.equal true + + it "should return 204 status code", -> + @res.status.calledWith(204).should.equal true + @res.end.called.should.equal true \ No newline at end of file diff --git a/services/web/test/UnitTests/coffee/Tags/TagsHandlerTests.coffee b/services/web/test/UnitTests/coffee/Tags/TagsHandlerTests.coffee index d19b780dfa..88ebb7b182 100644 --- a/services/web/test/UnitTests/coffee/Tags/TagsHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Tags/TagsHandlerTests.coffee @@ -7,11 +7,11 @@ _ = require('underscore') describe 'TagsHandler', -> - user_id = "123nd3ijdks" + user_id = "user-id-123" tag_id = "tag-id-123" - project_id = "123njdskj9jlk" + project_id = "project-id-123" tagsUrl = "tags.sharelatex.testing" - tag = "class101" + tag = "tag_name" beforeEach -> @request = @@ -31,12 +31,6 @@ describe 'TagsHandler', -> @handler.addTag user_id, project_id, tag, => @request.post.calledWith({uri:"#{tagsUrl}/user/#{user_id}/project/#{project_id}/tag", timeout:1000, json:{name:tag}}).should.equal true done() - - describe "removeProject", -> - it 'should send a delete request when a delete has been recived with the body format standardised', (done)-> - @handler.removeProject user_id, project_id, tag, => - @request.del.calledWith({uri:"#{tagsUrl}/user/#{user_id}/project/#{project_id}/tag", timeout:1000, json:{name:tag}}).should.equal true - done() describe "removeProjectFromAllTags", -> it 'should tell the tags api to remove the project_id from all the users tags', (done)-> @@ -93,9 +87,9 @@ describe 'TagsHandler', -> @handler.getAllTags user_id, (err, allTags)=> stubbedAllTags.should.deep.equal allTags getOpts = - uri: "#{tagsUrl}/user/#{user_id}/tag" + url: "#{tagsUrl}/user/#{user_id}/tag" json:true - timeout:2000 + timeout:1000 @request.get.calledWith(getOpts).should.equal true done() @@ -113,7 +107,10 @@ describe 'TagsHandler', -> it "should send a request to the tag backend", -> @request.del - .calledWith("#{tagsUrl}/user/#{user_id}/tag/#{tag_id}") + .calledWith({ + url: "#{tagsUrl}/user/#{user_id}/tag/#{tag_id}" + timeout: 1000 + }) .should.equal true it "should call the callback with no error", -> @@ -139,6 +136,7 @@ describe 'TagsHandler', -> url: "#{tagsUrl}/user/#{user_id}/tag/#{tag_id}/rename" json: name: @name + timeout: 1000 }) .should.equal true @@ -152,3 +150,28 @@ describe 'TagsHandler', -> it "should call the callback with an Error", -> @callback.calledWith(new Error()).should.equal true + + describe "removeProjectFromTag", -> + describe "successfully", -> + beforeEach -> + @request.del = sinon.stub().callsArgWith(1, null, {statusCode: 204}, "") + @handler.removeProjectFromTag user_id, tag_id, project_id, @callback + + it "should send a request to the tag backend", -> + @request.del + .calledWith({ + url: "#{tagsUrl}/user/#{user_id}/tag/#{tag_id}/project/#{project_id}" + timeout: 1000 + }) + .should.equal true + + it "should call the callback with no error", -> + @callback.calledWith(null).should.equal true + + describe "with error", -> + beforeEach -> + @request.del = sinon.stub().callsArgWith(1, null, {statusCode: 500}, "") + @handler.removeProjectFromTag user_id, tag_id, project_id, @callback + + it "should call the callback with an Error", -> + @callback.calledWith(new Error()).should.equal true \ No newline at end of file