From ea4b51a3ad43e7d6ce6b18b9eac32fd45e0cc367 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Tue, 4 Jun 2013 15:53:02 +0200 Subject: [PATCH] SONAR-3755 Fix issue on action plan --- .../issue/InternalRubyIssueService.java | 30 +++++--- .../api/action_plans_controller.rb | 72 ++++++++----------- .../issue/InternalRubyIssueServiceTest.java | 5 +- 3 files changed, 51 insertions(+), 56 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java index 53da80c91d9..f8ad8068b62 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java +++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java @@ -228,7 +228,7 @@ public class InternalRubyIssueService implements ServerComponent { result.addError(Result.Message.ofL10n("action_plans.errors.action_plan_does_not_exist", key)); return result; } else { - Result result = createActionPlanResult(parameters, existingActionPlan.name()); + Result result = createActionPlanResult(parameters, existingActionPlan); if (result.ok()) { DefaultActionPlan actionPlan = (DefaultActionPlan) result.get(); actionPlan.setKey(existingActionPlan.key()); @@ -269,7 +269,7 @@ public class InternalRubyIssueService implements ServerComponent { } @VisibleForTesting - Result createActionPlanResult(Map parameters, @Nullable String oldName) { + Result createActionPlanResult(Map parameters, @Nullable DefaultActionPlan existingActionPlan) { Result result = Result.of(); String name = parameters.get("name"); @@ -290,12 +290,15 @@ public class InternalRubyIssueService implements ServerComponent { result.addError(Result.Message.ofL10n("errors.is_too_long", "description", 1000)); } - if (Strings.isNullOrEmpty(projectParam)) { - result.addError(Result.Message.ofL10n("errors.cant_be_empty", "project")); - } else { - ResourceDto project = resourceDao.getResource(ResourceQuery.create().setKey(projectParam)); - if (project == null) { - result.addError(Result.Message.ofL10n("action_plans.errors.project_does_not_exist", projectParam)); + // Can only set project on creation + if (existingActionPlan == null) { + if (Strings.isNullOrEmpty(projectParam)) { + result.addError(Result.Message.ofL10n("errors.cant_be_empty", "project")); + } else { + ResourceDto project = resourceDao.getResource(ResourceQuery.create().setKey(projectParam)); + if (project == null) { + result.addError(Result.Message.ofL10n("action_plans.errors.project_does_not_exist", projectParam)); + } } } @@ -311,17 +314,24 @@ public class InternalRubyIssueService implements ServerComponent { } } - if (!Strings.isNullOrEmpty(projectParam) && !Strings.isNullOrEmpty(name) && !name.equals(oldName) + if (!Strings.isNullOrEmpty(projectParam) && !Strings.isNullOrEmpty(name) && (existingActionPlan == null || !name.equals(existingActionPlan.name())) && actionPlanService.isNameAlreadyUsedForProject(name, projectParam)) { result.addError(Result.Message.ofL10n("action_plans.same_name_in_same_project")); } if (result.ok()) { DefaultActionPlan actionPlan = DefaultActionPlan.create(name) - .setProjectKey(projectParam) .setDescription(description) .setUserLogin(UserSession.get().login()) .setDeadLine(deadLine); + + // Can only set project on creation + if (existingActionPlan == null) { + actionPlan.setProjectKey(projectParam); + } else { + actionPlan.setProjectKey(existingActionPlan.projectKey()); + } + result.set(actionPlan); } return result; diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/action_plans_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/action_plans_controller.rb index 6408df9ceef..d3a15fce5ed 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/action_plans_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/action_plans_controller.rb @@ -30,7 +30,7 @@ class Api::ActionPlansController < Api::ApiController require_parameters :project action_plans = Internal.issues.findActionPlanStats(params[:project]) - hash = {:actionPlans => action_plans.map { |plan| action_plan_to_hash(plan)}} + hash = {:actionPlans => action_plans.map { |plan| ActionPlan.to_hash(plan)}} respond_to do |format| format.json { render :json => jsonp(hash) } @@ -47,7 +47,7 @@ class Api::ActionPlansController < Api::ApiController # # -- Optional parameters # 'description' is the plain-text description - # 'deadLine' is the due date of the action plan. Format is 'day/month/year', for instance, '31/12/2013'. + # 'deadLine' is the due date of the action plan. Format is 'year-month-day', for instance, '2013-12-31'. # # -- Example # curl -X POST -v -u admin:admin 'http://localhost:9000/api/action_plans/create?name=Current&project=org.sonar.Sample' @@ -58,12 +58,7 @@ class Api::ActionPlansController < Api::ApiController require_parameters :project, :name result = Internal.issues.createActionPlan(params) - if result.ok() - action_plan = result.get() - render :json => jsonp({:actionPlan => action_plan_to_hash(action_plan)}) - else - render_result_error(result) - end + render_result(result) end # @@ -81,11 +76,7 @@ class Api::ActionPlansController < Api::ApiController require_parameters :key result = Internal.issues.deleteActionPlan(params[:key]) - if result.ok() - render_success('Action plan deleted') - else - render_result_error(result) - end + render_result(result) end # @@ -93,11 +84,13 @@ class Api::ActionPlansController < Api::ApiController # # -- Mandatory parameters # 'name' is the name of the action plan - # 'project' is the key of the project to link the action plan to # # -- Optional parameters # 'description' is the plain-text description - # 'deadLine' is the due date of the action plan. Format is 'day/month/year', for instance, '31/12/2013'. + # 'deadLine' is the due date of the action plan. Format is 'year-month-day', for instance, '2013-12-31'. + # + # -- Information + # 'project' cannot be update # # -- Example # curl -X POST -v -u admin:admin 'http://localhost:9000/api/action_plans/update?key=9b6f89c0-3347-46f6-a6d1-dd6c761240e0&name=Current' @@ -108,12 +101,7 @@ class Api::ActionPlansController < Api::ApiController require_parameters :key result = Internal.issues.updateActionPlan(params[:key], params) - if result.ok() - action_plan = result.get() - render :json => jsonp({:actionPlan => action_plan_to_hash(action_plan)}) - else - render_result_error(result) - end + render_result(result) end # @@ -131,12 +119,7 @@ class Api::ActionPlansController < Api::ApiController require_parameters :key result = Internal.issues.closeActionPlan(params[:key]) - if result.ok() - action_plan = result.get() - render :json => jsonp({:actionPlan => action_plan_to_hash(action_plan)}) - else - render_result_error(result) - end + render_result(result) end # @@ -154,31 +137,34 @@ class Api::ActionPlansController < Api::ApiController require_parameters :key result = Internal.issues.openActionPlan(params[:key]) - if result.ok() - action_plan = result.get() - render :json => jsonp({:actionPlan => action_plan_to_hash(action_plan)}) - else - render_result_error(result) - end + render_result(result) end private - def action_plan_to_hash(action_plan) - ActionPlan.to_hash(action_plan) - end + def render_result(result) + http_status = (result.ok ? 200 : 400) + hash = result_to_hash(result) + hash[:actionPlan] = ActionPlan.to_hash(result.get) if result.get - def error_to_hash(msg) - {:msg => message(msg.text(), {:params => msg.params()}).capitalize} + respond_to do |format| + # if the request header "Accept" is "*/*", then the default format is the first one (json) + format.json { render :json => jsonp(hash), :status => result.httpStatus } + format.xml { render :xml => hash.to_xml(:skip_types => true, :root => 'sonar', :status => http_status) } + end end - def render_result_error(result) - hash = {:errors => result.errors().map { |error| error_to_hash(error) }} - respond_to do |format| - format.json { render :json => jsonp(hash), :status => 400} - format.xml { render :xml => hash.to_xml(:skip_types => true, :root => 'errors', :status => 400)} + def result_to_hash(result) + hash = {} + if result.errors + hash[:errors] = result.errors().map do |error| + { + :msg => (error.text ? error.text : Api::Utils.message(error.l10nKey, :params => error.l10nParams)) + } + end end + hash end end \ No newline at end of file diff --git a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java index 31b7e4d4e11..7c3b38d610e 100644 --- a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java +++ b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java @@ -102,13 +102,12 @@ public class InternalRubyIssueServiceTest { @Test public void should_update_action_plan_with_new_project() { - when(actionPlanService.findByKey(eq("ABCD"), any(UserSession.class))).thenReturn(DefaultActionPlan.create("Long term")); + when(actionPlanService.findByKey(eq("ABCD"), any(UserSession.class))).thenReturn(DefaultActionPlan.create("Long term").setProjectKey("org.sonar.MultiSample")); Map parameters = newHashMap(); parameters.put("name", "New Long term"); parameters.put("description", "New Long term issues"); parameters.put("deadLine", "2113-05-13"); - parameters.put("project", "org.sonar.MultiSample"); ArgumentCaptor actionPlanCaptor = ArgumentCaptor.forClass(ActionPlan.class); Result result = internalRubyIssueService.updateActionPlan("ABCD", parameters); @@ -253,7 +252,7 @@ public class InternalRubyIssueServiceTest { when(actionPlanService.isNameAlreadyUsedForProject(anyString(), anyString())).thenReturn(true); - Result result = internalRubyIssueService.createActionPlanResult(parameters, "Short term"); + Result result = internalRubyIssueService.createActionPlanResult(parameters, DefaultActionPlan.create("Short term")); assertThat(result.ok()).isFalse(); assertThat(result.errors()).contains(Result.Message.ofL10n("action_plans.same_name_in_same_project")); } -- 2.39.5