SONAR-3755 Fix issue on action plan

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<ActionPlan> result = createActionPlanResult(parameters, existingActionPlan.name());
+      Result<ActionPlan> 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<ActionPlan> createActionPlanResult(Map<String, String> parameters, @Nullable String oldName) {
+  Result<ActionPlan> createActionPlanResult(Map<String, String> parameters, @Nullable DefaultActionPlan existingActionPlan) {
     Result<ActionPlan> 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<String, String> 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<ActionPlan> 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"));
   }