SONAR-4304 better exception handling/UI

plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties

@@ -179,6 +179,7 @@ bulleted_point=Bulleted point
 coding_rules=Coding rules
 click_to_add_to_favourites=Click to add to favourites
 click_to_remove_from_favourites=Click to remove from favourites
+contact_admin=Please contact your administrator.
 created_by=Created by
 deactivate_all=Deactivate all
 default_severity=Default severity
@@ -553,6 +554,8 @@ issue.resolution.OPEN=Open
 issue.resolution.FALSE-POSITIVE=False-positive
 issue.resolution.FIXED=Fixed
 issue.planned_for_x=Planned for {0}
+issue.manual.missing_rule=Rule is not selected
+issue.manual.no_rules=No rules.
 
 #------------------------------------------------------------------------------
 #
@@ -1306,7 +1309,6 @@ code_viewer.create_violation.submit=Create Violation
 code_viewer.create_violation.missing_rule=Missing rule
 code_viewer.create_violation.missing_message=Missing message
 code_viewer.create_violation.missing_severity=Missing severity
-code_viewer.create_violation.no_rules=No rules. Please contact your administrator.
 code_viewer.create_violation.bad_assignee=Unknown assignee
 
 

sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java

@@ -28,6 +28,7 @@ import org.sonar.api.rules.RuleFinder;
 import org.sonar.api.rules.RuleQuery;
 import org.sonar.jpa.session.DatabaseSessionFactory;
 
+import javax.annotation.CheckForNull;
 import javax.persistence.Query;
 
 import java.util.Collection;
@@ -69,14 +70,17 @@ public class DefaultRuleFinder implements RuleFinder {
     return hqlQuery.getResultList();
   }
 
+  @CheckForNull
   public Rule findByKey(RuleKey key) {
     return findByKey(key.repository(), key.rule());
   }
 
+  @CheckForNull
   public Rule findByKey(String repositoryKey, String key) {
     return doFindByKey(repositoryKey, key);
   }
 
+  @CheckForNull
   protected final Rule doFindByKey(String repositoryKey, String key) {
     DatabaseSession session = sessionFactory.getSession();
     return session.getSingleResult(

sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleFinder.java

@@ -24,6 +24,7 @@ import org.sonar.api.task.TaskComponent;
 
 import org.sonar.api.ServerComponent;
 
+import javax.annotation.CheckForNull;
 import java.util.Collection;
 
 /**
@@ -34,10 +35,13 @@ public interface RuleFinder extends TaskComponent, ServerComponent {
   /**
    * @since 2.5
    */
+  @CheckForNull
   Rule findById(int ruleId);
 
+  @CheckForNull
   Rule findByKey(String repositoryKey, String key);
 
+  @CheckForNull
   Rule findByKey(RuleKey key);
 
   Rule find(RuleQuery query);

sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java

@@ -109,19 +109,43 @@ public class InternalRubyIssueService implements ServerComponent {
     return commentService.findComment(commentKey);
   }
 
-  public Issue create(Map<String, String> parameters) {
-    String componentKey = parameters.get("component");
-    // TODO verify authorization
-    // TODO check existence of component
-    DefaultIssueBuilder builder = new DefaultIssueBuilder().componentKey(componentKey);
-    builder.line(RubyUtils.toInteger(parameters.get("line")));
-    builder.message(parameters.get("message"));
-    builder.severity(parameters.get("severity"));
-    builder.effortToFix(RubyUtils.toDouble(parameters.get("effortToFix")));
-    // TODO verify existence of rule
-    builder.ruleKey(RuleKey.parse(parameters.get("rule")));
-    Issue issue = builder.build();
-    return issueService.create((DefaultIssue) issue, UserSession.get());
+  /**
+   * Create manual issue
+   */
+  public Result<DefaultIssue> create(Map<String, String> params) {
+    Result<DefaultIssue> result = Result.of();
+    try {
+      // mandatory parameters
+      String componentKey = params.get("component");
+      if (StringUtils.isBlank(componentKey)) {
+        result.addError("Component is not set");
+      }
+      RuleKey ruleKey = null;
+      String rule = params.get("rule");
+      if (StringUtils.isBlank(rule)) {
+        result.addError(Result.Message.ofL10n("issue.manual.missing_rule"));
+      } else {
+        ruleKey = RuleKey.parse(rule);
+      }
+
+      if (result.ok()) {
+        DefaultIssue issue = (DefaultIssue)new DefaultIssueBuilder()
+          .componentKey(componentKey)
+          .line(RubyUtils.toInteger(params.get("line")))
+          .message(params.get("message"))
+          .severity(params.get("severity"))
+          .effortToFix(RubyUtils.toDouble(params.get("effortToFix")))
+          .ruleKey(ruleKey)
+          .reporter(UserSession.get().login())
+          .build();
+        issue = issueService.createManualIssue(issue, UserSession.get());
+        result.set(issue);
+      }
+
+    } catch (Exception e) {
+      result.addError(e.getMessage());
+    }
+    return result;
   }
 
   public Collection<ActionPlan> findOpenActionPlans(String issueKey) {
@@ -153,7 +177,7 @@ public class InternalRubyIssueService implements ServerComponent {
     DefaultActionPlan existingActionPlan = (DefaultActionPlan) actionPlanService.findByKey(key);
     if (existingActionPlan == null) {
       Result<ActionPlan> result = Result.of();
-      result.addError("issues_action_plans.errors.action_plan_does_not_exists", key);
+      result.addError(Result.Message.ofL10n("issues_action_plans.errors.action_plan_does_not_exists", key));
       return result;
     } else {
       Result<ActionPlan> result = createActionPlanResult(parameters, existingActionPlan.name());
@@ -210,23 +234,23 @@ public class InternalRubyIssueService implements ServerComponent {
     Date deadLine = null;
 
     if (Strings.isNullOrEmpty(name)) {
-      result.addError("errors.cant_be_empty", "name");
+      result.addError(Result.Message.ofL10n("errors.cant_be_empty", "name"));
     } else {
       if (name.length() > 200) {
-        result.addError("errors.is_too_long", "name", 200);
+        result.addError(Result.Message.ofL10n("errors.is_too_long", "name", 200));
       }
     }
 
     if (!Strings.isNullOrEmpty(description) && description.length() > 1000) {
-      result.addError("errors.is_too_long", "description", 1000);
+      result.addError(Result.Message.ofL10n("errors.is_too_long", "description", 1000));
     }
 
     if (Strings.isNullOrEmpty(projectParam) && oldName == null) {
-      result.addError("errors.cant_be_empty", "project");
+      result.addError(Result.Message.ofL10n("errors.cant_be_empty", "project"));
     } else {
       ResourceDto project = resourceDao.getResource(ResourceQuery.create().setKey(projectParam));
       if (project == null) {
-        result.addError("issues_action_plans.errors.project_does_not_exists", projectParam);
+        result.addError(Result.Message.ofL10n("issues_action_plans.errors.project_does_not_exists", projectParam));
       }
     }
 
@@ -235,16 +259,16 @@ public class InternalRubyIssueService implements ServerComponent {
         SimpleDateFormat dateFormat = new SimpleDateFormat("dd/MM/yyyy");
         deadLine = dateFormat.parse(deadLineParam);
         if (deadLine.before(new Date())) {
-          result.addError("issues_action_plans.date_cant_be_in_past");
+          result.addError(Result.Message.ofL10n("issues_action_plans.date_cant_be_in_past"));
         }
       } catch (Exception e) {
-        result.addError("errors.is_not_valid", "date");
+        result.addError(Result.Message.ofL10n("errors.is_not_valid", "date"));
       }
     }
 
     if (!Strings.isNullOrEmpty(projectParam) && !Strings.isNullOrEmpty(name) && !name.equals(oldName)
       && actionPlanService.isNameAlreadyUsedForProject(name, projectParam)) {
-      result.addError("issues_action_plans.same_name_in_same_project");
+      result.addError(Result.Message.ofL10n("issues_action_plans.same_name_in_same_project"));
     }
 
     if (result.ok()) {
@@ -261,7 +285,7 @@ public class InternalRubyIssueService implements ServerComponent {
   private Result<ActionPlan> createResultForExistingActionPlan(String actionPlanKey) {
     Result<ActionPlan> result = Result.of();
     if (findActionPlan(actionPlanKey) == null) {
-      result.addError("issues_action_plans.errors.action_plan_does_not_exists", actionPlanKey);
+      result.addError(Result.Message.ofL10n("issues_action_plans.errors.action_plan_does_not_exists", actionPlanKey));
     }
     return result;
   }

sonar-server/src/main/java/org/sonar/server/issue/IssueService.java

@@ -22,6 +22,8 @@ package org.sonar.server.issue;
 import com.google.common.base.Strings;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.issue.Issue;
+import org.sonar.api.rules.Rule;
+import org.sonar.api.rules.RuleFinder;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.DefaultIssue;
 import org.sonar.core.issue.IssueChangeContext;
@@ -48,20 +50,32 @@ public class IssueService implements ServerComponent {
   private final IssueUpdater issueUpdater;
   private final IssueStorage issueStorage;
   private final ActionPlanService actionPlanService;
+  private final RuleFinder ruleFinder;
 
   public IssueService(DefaultIssueFinder finder,
                       IssueWorkflow workflow,
                       IssueStorage issueStorage,
-                      IssueUpdater issueUpdater, ActionPlanService actionPlanService) {
+                      IssueUpdater issueUpdater,
+                      ActionPlanService actionPlanService,
+                      RuleFinder ruleFinder) {
     this.finder = finder;
     this.workflow = workflow;
     this.issueStorage = issueStorage;
     this.issueUpdater = issueUpdater;
     this.actionPlanService = actionPlanService;
+    this.ruleFinder = ruleFinder;
   }
 
+  /**
+   * List of available transitions, ordered by key.
+   * <p/>
+   * Never return null, but return an empty list if the issue does not exist.
+   */
   public List<Transition> listTransitions(String issueKey) {
     DefaultIssue issue = loadIssue(issueKey);
+    if (issue == null) {
+      return Collections.emptyList();
+    }
     List<Transition> transitions = workflow.outTransitions(issue);
     Collections.sort(transitions, new Comparator<Transition>() {
       @Override
@@ -72,7 +86,13 @@ public class IssueService implements ServerComponent {
     return transitions;
   }
 
-  public List<Transition> listTransitions(Issue issue) {
+  /**
+   * Never return null, but an empty list if the issue does not exist.
+   */
+  public List<Transition> listTransitions(@Nullable Issue issue) {
+    if (issue == null) {
+      return Collections.emptyList();
+    }
     List<Transition> transitions = workflow.outTransitions(issue);
     Collections.sort(transitions, new Comparator<Transition>() {
       @Override
@@ -130,15 +150,25 @@ public class IssueService implements ServerComponent {
     return issue;
   }
 
-  public Issue create(DefaultIssue issue, UserSession userSession) {
-    // TODO merge with InternalRubyIssueService
-    issue.setReporter(userSession.login());
+  public DefaultIssue createManualIssue(DefaultIssue issue, UserSession userSession) {
+    verifyLoggedIn(userSession);
+    if (!"manual".equals(issue.ruleKey().repository())) {
+      throw new IllegalArgumentException("Issues can be created only on rules marked as 'manual': " + issue.ruleKey());
+    }
+    Rule rule = ruleFinder.findByKey(issue.ruleKey());
+    if (rule == null) {
+      throw new IllegalArgumentException("Unknown rule: " + issue.ruleKey());
+    }
 
-    issueStorage.save(issue);
 
+    // TODO check existence of component
+    // TODO verify authorization
+
+    issueStorage.save(issue);
     return issue;
   }
 
+
   public DefaultIssue loadIssue(String issueKey) {
     return finder.findByKey(issueKey, UserRole.USER);
   }

sonar-server/src/main/java/org/sonar/server/issue/Result.java

@@ -19,6 +19,9 @@
  */
 package org.sonar.server.issue;
 
+import org.apache.commons.lang.builder.ReflectionToStringBuilder;
+
+import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import java.util.Arrays;
 import java.util.List;
@@ -31,8 +34,7 @@ import static com.google.common.collect.Lists.newArrayList;
 public class Result<T> {
 
   private T object = null;
-  private List<Message> errors = newArrayList();
-  ;
+  private final List<Message> errors = newArrayList();
 
   private Result(@Nullable T object) {
     this.object = object;
@@ -55,35 +57,69 @@ public class Result<T> {
     return object;
   }
 
-  public Result<T> addError(String text, Object... params) {
-    Message message = new Message(text, params);
+  public Result<T> addError(String text) {
+    return addError(Message.of(text));
+  }
+
+  public Result<T> addError(Message message) {
     errors.add(message);
     return this;
   }
 
+  /**
+   * List of error messages. Empty if there are no errors.
+   */
   public List<Message> errors() {
     return errors;
   }
 
+  /**
+   * True if there are no errors.
+   */
   public boolean ok() {
     return errors.isEmpty();
   }
 
+  public int httpStatus() {
+    if (ok()) {
+      return 200;
+    }
+    // TODO support 401, 403 and 500
+    return 400;
+  }
+
   public static class Message {
-    private String text;
-    private Object[] params;
+    private final String l10nKey;
+    private final Object[] l10nParams;
+    private final String text;
 
-    public Message(String text, Object... params) {
+    private Message(@Nullable String l10nKey, @Nullable Object[] l10nParams, @Nullable String text) {
+      this.l10nKey = l10nKey;
+      this.l10nParams = l10nParams;
       this.text = text;
-      this.params = params;
     }
 
+    public static Message of(String text) {
+      return new Message(null, null, text);
+    }
+
+    public static Message ofL10n(String l10nKey, Object... l10nParams) {
+      return new Message(l10nKey, l10nParams, null);
+    }
+
+    @CheckForNull
     public String text() {
       return text;
     }
 
-    public Object[] params() {
-      return params;
+    @CheckForNull
+    public String l10nKey() {
+      return l10nKey;
+    }
+
+    @CheckForNull
+    public Object[] l10nParams() {
+      return l10nParams;
     }
 
     @Override
@@ -94,24 +130,32 @@ public class Result<T> {
       if (o == null || getClass() != o.getClass()) {
         return false;
       }
-
       Message message = (Message) o;
 
-      if (!Arrays.equals(params, message.params)) {
+      if (l10nKey != null ? !l10nKey.equals(message.l10nKey) : message.l10nKey != null) {
+        return false;
+      }
+      // Probably incorrect - comparing Object[] arrays with Arrays.equals
+      if (!Arrays.equals(l10nParams, message.l10nParams)) {
         return false;
       }
       if (text != null ? !text.equals(message.text) : message.text != null) {
         return false;
       }
-
       return true;
     }
 
     @Override
     public int hashCode() {
-      int result = text != null ? text.hashCode() : 0;
-      result = 31 * result + (params != null ? Arrays.hashCode(params) : 0);
+      int result = l10nKey != null ? l10nKey.hashCode() : 0;
+      result = 31 * result + (l10nParams != null ? Arrays.hashCode(l10nParams) : 0);
+      result = 31 * result + (text != null ? text.hashCode() : 0);
       return result;
     }
+
+    @Override
+    public String toString() {
+      return new ReflectionToStringBuilder(this).toString();
+    }
   }
 }

sonar-server/src/main/java/org/sonar/server/util/RubyUtils.java

@@ -22,10 +22,11 @@ package org.sonar.server.util;
 import com.google.common.base.Splitter;
 import com.google.common.collect.Lists;
 import com.google.common.primitives.Ints;
+import org.apache.commons.lang.StringUtils;
 import org.sonar.api.utils.DateUtils;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
-
 import java.util.Date;
 import java.util.List;
 
@@ -51,52 +52,84 @@ public class RubyUtils {
     return result;
   }
 
+  @CheckForNull
   public static Integer toInteger(@Nullable Object o) {
+    if (o == null) {
+      return null;
+    }
     if (o instanceof Integer) {
       return (Integer) o;
     }
     if (o instanceof Long) {
       return Ints.checkedCast((Long) o);
     }
-
     if (o instanceof String) {
+      if (StringUtils.isBlank((String)o)) {
+        return null;
+      }
       return Integer.parseInt((String) o);
     }
-    return null;
+    throw new IllegalArgumentException("Unsupported type for integer: " + o.getClass());
   }
 
+  @CheckForNull
   public static Double toDouble(@Nullable Object o) {
+    if (o == null) {
+      return null;
+    }
     if (o instanceof Double) {
       return (Double) o;
     }
+    if (o instanceof Integer) {
+      return ((Integer) o).doubleValue();
+    }
+    if (o instanceof Long) {
+      return ((Long) o).doubleValue();
+    }
     if (o instanceof String) {
+      if (StringUtils.isBlank((String)o)) {
+        return null;
+      }
       return Double.parseDouble((String) o);
     }
-    return null;
+    throw new IllegalArgumentException("Unsupported type for double: " + o.getClass());
   }
 
-
+  @CheckForNull
   public static Date toDate(@Nullable Object o) {
+    if (o == null) {
+      return null;
+    }
     if (o instanceof Date) {
       return (Date) o;
     }
     if (o instanceof String) {
+      if (StringUtils.isBlank((String)o)) {
+        return null;
+      }
       Date date = DateUtils.parseDateTimeQuietly((String) o);
       if (date != null) {
         return date;
       }
       return DateUtils.parseDate((String) o);
     }
-    return null;
+    throw new IllegalArgumentException("Unsupported type for date: " + o.getClass());
   }
 
+  @CheckForNull
   public static Boolean toBoolean(@Nullable Object o) {
+    if (o == null) {
+      return null;
+    }
     if (o instanceof Boolean) {
       return (Boolean) o;
     }
     if (o instanceof String) {
+      if (StringUtils.isBlank((String)o)) {
+        return null;
+      }
       return Boolean.parseBoolean((String) o);
     }
-    return null;
+    throw new IllegalArgumentException("Unsupported type for boolean: " + o.getClass());
   }
 }

sonar-server/src/main/webapp/WEB-INF/app/controllers/api/issues_controller.rb

@@ -52,7 +52,6 @@ class Api::IssuesController < Api::ApiController
   # curl -v -u admin:admin 'http://localhost:9000/api/issues/transitions?issue=9b6f89c0-3347-46f6-a6d1-dd6c761240e0'
   #
   def transitions
-    # TODO deal with errors (404, ...)
     require_parameters :issue
     issue_key = params[:issue]
     transitions = Internal.issues.listTransitions(issue_key)
@@ -200,26 +199,33 @@ class Api::IssuesController < Api::ApiController
   #
   # -- Mandatory parameters
   # 'component' is the component key
-  # 'rule' includes the repository key and the rule key, for example 'squid:AvoidCycle'
+  # 'rule' is the rule key prefixed with "manual:", for example 'manual:performance'
   #
   # -- Optional parameters
   # 'severity' is in BLOCKER, CRITICAL, ... INFO. Default value is MAJOR.
   # 'line' starts at 1
-  # 'message' is the markdown message
+  # 'message' is the plain-text message
   #
   # -- Example
-  # curl -X POST -v -u admin:admin 'http://localhost:9000/api/issues/create?component=commons-io:commons-io:org.apache.commons.io.filefilter.OrFileFilter&rule=pmd:ConstructorCallsOverridableMethod&line=2&severity=BLOCKER'
+  # curl -X POST -v -u admin:admin 'http://localhost:9000/api/issues/create?component=commons-io:commons-io:org.apache.commons.io.filefilter.OrFileFilter&rule=manual:performance&line=2&severity=BLOCKER'
   #
   def create
     verify_post_request
-    access_denied unless logged_in?
-    require_parameters :component, :rule
 
-    issue = Internal.issues.create(params)
-    render :json => jsonp({:issue => Issue.to_hash(issue)})
+    issue_result = Internal.issues.create(params)
+
+    http_status = (issue_result.ok ? 200 : 400)
+    hash = result_to_hash(issue_result)
+    hash[:issue] = Issue.to_hash(issue_result.get) if issue_result.get
+
+    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 => issue_result.httpStatus }
+      format.xml { render :xml => hash.to_xml(:skip_types => true, :root => 'sonar', :status => http_status) }
+    end
   end
 
-  private
+  protected
 
   def component_to_hash(component)
     hash = {
@@ -239,4 +245,17 @@ class Api::IssuesController < Api::ApiController
       :pages => paging.pages
     }
   end
+
+  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

sonar-server/src/main/webapp/WEB-INF/app/controllers/issue_controller.rb

@@ -121,7 +121,6 @@ class IssueController < ApplicationController
   def create
     verify_post_request
     verify_ajax_request
-    require_parameters :rule, :component
 
     component_key = params[:component]
     if Api::Utils.is_integer?(component_key)
@@ -129,14 +128,16 @@ class IssueController < ApplicationController
       component_key = (component ? component.key : nil)
     end
 
-    issue = Internal.issues.create(params.merge({:component => component_key}))
-
-    @issue_results = Api.issues.find(issue.key)
-    render :partial => 'issue/issue', :locals => {:issue => @issue_results.issues.get(0)}
+    issue_result = Internal.issues.create(params.merge({:component => component_key}))
+    if issue_result.ok
+      @issue_results = Api.issues.find(issue_result.get.key)
+      render :partial => 'issue/issue', :locals => {:issue => @issue_results.issues.get(0)}
+    else
+      render :partial => 'shared/result_messages', :status => 500, :locals => {:result => issue_result}
+    end
   end
 
 
-
   #
   #
   # ACTIONS FROM THE ISSUES WIDGETS

sonar-server/src/main/webapp/WEB-INF/app/views/issue/_assign_form.html.erb

@@ -7,7 +7,7 @@
         <%= user_select_tag('assignee', :html_id => "assignee-#{params[:issue]}", :open => true) -%>
         <input type="button" value="<%= message('issue.assign.submit') -%>" onclick="submitIssueForm(this)">
         &nbsp;<%= link_to_function message('cancel'), 'closeIssueForm(this)' -%>&nbsp;
-        <span class="hidden"><%= image_tag 'loading.gif' -%></span>
+        <span class="loading hidden"></span>
       </td>
     </tr>
   </table>

sonar-server/src/main/webapp/WEB-INF/app/views/issue/_create_form.html.erb

@@ -2,40 +2,36 @@
    manual_rules = Rule.manual_rules
    is_admin = has_role?(:admin)
    form_id = "create-issue-#{params[:component]}-#{params[:line]}"
-   if manual_rules.empty? && !is_admin
+   rule_select_id = "#{form_id}-rules"
 %>
-  <div class="warning">
-    No rules. Please contact the administrator.
-    &nbsp;<%= link_to_function message('cancel'), 'closeMIF(this)' -%>
-  </div>
+<form action="" class="code-issue-create-form" id="<%= form_id -%>">
+  <%
+     if manual_rules.empty?
+  %>
+    <div class="warning" style="margin: 10px">
+
+      <%= message('issue.manual.no_rules') -%>
+      <% if is_admin %>
+        <a href="<%= ApplicationController.root_context -%>/manual_rules/index"><%= message('manage') -%></a>
+      <% else %>
+        <%= message('contact_admin') -%>
+      <% end %>
+
+      &nbsp;<%= link_to_function message('cancel'), 'closeCreateIssueForm(this)' -%>
+    </div>
+
+  <% else %>
 
-<% else %>
-  <form action="" class="code-issue-create-form" id="<%= form_id -%>">
     <input type="hidden" name="line" value="<%= params[:line] -%>"/>
     <input type="hidden" name="component" value="<%= params[:component] -%>"/>
 
     <div class="code-issue">
       <div class="code-issue-name">
-        <table class="width100">
-          <tr>
-            <td class="thin spacer-right">
-              <% unless manual_rules.empty? %>
-                <%= dropdown_tag 'rule',
-                                 options_for_select([[]].concat(manual_rules.map { |rule| [rule.name, rule.key] })),
-                                 {:show_search_box => true, :placeholder => 'Select a Rule'},
-                                 {:html_id => "#{form_id}-rules"} -%>
-              <% end %>
-            </td>
-            <td align="left">
-              <% if is_admin %>
-                <a href="<%= ApplicationController.root_context -%>/manual_rules/index">Manage Rules</a>
-              <% end %>
-            </td>
-          </tr>
-        </table>
-
+        <%= dropdown_tag 'rule',
+                         options_for_select([[]].concat(manual_rules.map { |rule| [rule.name, rule.key] })),
+                         {:show_search_box => true, :placeholder => 'Select a Rule'},
+                         {:html_id => rule_select_id} -%>
       </div>
-
       <div class="code-issue-msg">
         <table class="width100">
           <tr>
@@ -45,11 +41,14 @@
           </tr>
           <tr>
             <td>
-              <input type="submit" value="Create" onclick="return submitMIF(this);"> &nbsp;<%= link_to_function message('cancel'), 'closeMIF(this)' -%>
+              <input type="submit" value="Create" onclick="return submitCreateIssueForm(this);"> &nbsp;
+              <%= link_to_function message('cancel'), 'closeCreateIssueForm(this)' -%> &nbsp;
+              <span class="loading hidden"></span>
             </td>
           </tr>
         </table>
+        <div class="code-issue-errors hidden"></div>
       </div>
     </div>
-  </form>
-<% end %>
+  <% end %>
+</form>

sonar-server/src/main/webapp/WEB-INF/app/views/issue/_plan_form.html.erb

@@ -20,7 +20,7 @@
 
   <input type="button" value="<%= message('issue.plan.submit') -%>" onclick="submitIssueForm(this)">
   &nbsp;<%= link_to_function message('cancel'), 'closeIssueForm(this)' -%>&nbsp;
-  <span class="hidden"><%= image_tag 'loading.gif' -%></span>
+  <span class="loading hidden"></span>
 
 </form>
 

sonar-server/src/main/webapp/WEB-INF/app/views/shared/_result_messages.html.erb

@@ -0,0 +1,5 @@
+<% result.errors.each do |msg| %>
+  <div class="error"><%= h (msg.text ? msg.text : Api::Utils.message(msg.l10nKey, :params => msg.l10nParams)) -%></div>
+<% end %>
+
+

sonar-server/src/main/webapp/WEB-INF/app/views/shared/_source_violation_form.html.erb

@@ -1,3 +1 @@
-<td class="plus">
-    <a onclick="return openMIF(this, <%= resource_id -%>,<%= index + 1 -%>)"></a>
-</td>
+<td class="plus"><a onclick="return openCIF(this,<%= resource_id -%>,<%= index + 1 -%>)"></a></td>

sonar-server/src/main/webapp/javascripts/issue.js

@@ -158,7 +158,8 @@ function refreshIssue(elt) {
   return false;
 }
 
-function openMIF(elt, componentId, line) {
+/* Open form for creating a manual issue */
+function openCIF(elt, componentId, line) {
   // TODO check if form is already displayed (by using form id)
   $j.get(baseUrl + "/issue/create_form?component=" + componentId + "&line=" + line, function (html) {
     $j(elt).closest('tr').find('td.line').append($j(html));
@@ -166,24 +167,36 @@ function openMIF(elt, componentId, line) {
   return false;
 }
 
-function closeMIF(elt) {
+/* Close the form used for creating a manual issue */
+function closeCreateIssueForm(elt) {
   $j(elt).closest('.code-issue-create-form').remove();
   return false;
 }
 
-function submitMIF(elt) {
+/* Create a manual issue */
+function submitCreateIssueForm(elt) {
   var formElt = $j(elt).closest('form');
-  formElt.find('.loading').removeClass('hidden');
-  formElt.find(':submit').prop('disabled', true);
+  var loadingElt = formElt.find('.loading');
+
+  loadingElt.removeClass('hidden');
   $j.ajax({
       type: "POST",
       url: baseUrl + '/issue/create',
       data: formElt.serialize()}
-  ).success(function (data) {
-      formElt.html(data);
+  ).success(function (html) {
+      var replaced = $j(html);
+      formElt.replaceWith(replaced);
+
+      // enable the links opening modal popups
+      replaced.find('.open-modal').modal();
     }
-  ).fail(function (jqXHR, textStatus) {
-      alert(textStatus);
+  ).error(function (jqXHR, textStatus, errorThrown) {
+      var errorsElt = formElt.find('.code-issue-errors');
+      errorsElt.html(jqXHR.responseText);
+      errorsElt.removeClass('hidden');
+    }
+  ).always(function() {
+      loadingElt.addClass('hidden');
     });
   return false;
 }

sonar-server/src/main/webapp/stylesheets/style.css

@@ -897,7 +897,7 @@ span.rulename a:hover {
 }
 
 .code-global-issues {
-  padding: 10px;
+  padding: 0 0 10px 0;
 }
 
 .code-issues {

sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java

@@ -127,7 +127,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.updateActionPlan("ABCD", null);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("issues_action_plans.errors.action_plan_does_not_exists", "ABCD"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("issues_action_plans.errors.action_plan_does_not_exists", "ABCD"));
   }
 
   @Test
@@ -146,7 +146,7 @@ public class InternalRubyIssueServiceTest {
     Result result = internalRubyIssueService.deleteActionPlan("ABCD");
     verify(actionPlanService, never()).delete("ABCD");
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("issues_action_plans.errors.action_plan_does_not_exists", "ABCD"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("issues_action_plans.errors.action_plan_does_not_exists", "ABCD"));
   }
 
   @Test
@@ -175,7 +175,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("errors.cant_be_empty", "project"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "project"));
   }
 
   @Test
@@ -187,7 +187,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("errors.cant_be_empty", "name"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "name"));
   }
 
   @Test
@@ -199,7 +199,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("errors.is_too_long", "name", 200));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "name", 200));
   }
 
   @Test
@@ -211,7 +211,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("errors.is_too_long", "description", 1000));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "description", 1000));
   }
 
   @Test
@@ -224,7 +224,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("errors.is_not_valid", "date"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_not_valid", "date"));
   }
 
   @Test
@@ -237,7 +237,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("issues_action_plans.date_cant_be_in_past"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("issues_action_plans.date_cant_be_in_past"));
   }
 
   @Test
@@ -251,7 +251,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters, "Short term");
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("issues_action_plans.same_name_in_same_project"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("issues_action_plans.same_name_in_same_project"));
   }
 
   @Test
@@ -265,7 +265,7 @@ public class InternalRubyIssueServiceTest {
 
     Result result = internalRubyIssueService.createActionPlanResult(parameters);
     assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(new Result.Message("issues_action_plans.errors.project_does_not_exists", "org.sonar.Sample"));
+    assertThat(result.errors()).contains(Result.Message.ofL10n("issues_action_plans.errors.project_does_not_exists", "org.sonar.Sample"));
   }
   public String createLongString(int size) {
     String result = "";

sonar-server/src/test/java/org/sonar/server/issue/ResultTest.java

@@ -0,0 +1,99 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2013 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.issue;
+
+import org.junit.Test;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+public class ResultTest {
+  @Test
+  public void test_default_result() throws Exception {
+    Result<Object> result = Result.of();
+    assertThat(result.ok()).isTrue();
+    assertThat(result.errors()).isEmpty();
+    assertThat(result.httpStatus()).isEqualTo(200);
+    assertThat(result.get()).isNull();
+
+    Object obj = new Object();
+    result.set(obj);
+    assertThat(result.get()).isSameAs(obj);
+  }
+
+  @Test
+  public void test_error() throws Exception {
+    Result<Object> result = Result.of();
+    result.addError("Something goes wrong");
+
+    assertThat(result.ok()).isFalse();
+    assertThat(result.errors()).hasSize(1).contains(Result.Message.of("Something goes wrong"));
+    assertThat(result.httpStatus()).isEqualTo(400);
+    assertThat(result.get()).isNull();
+  }
+
+  @Test
+  public void test_l10n_errors() throws Exception {
+    Result<Object> result = Result.of();
+    Result.Message message = Result.Message.ofL10n("issue.error.123", "10");
+    result.addError(message);
+
+    assertThat(result.ok()).isFalse();
+    assertThat(result.errors()).hasSize(1).containsOnly(message);
+
+    message = result.errors().get(0);
+    assertThat(message.text()).isNull();
+    assertThat(message.l10nKey()).isEqualTo("issue.error.123");
+    assertThat(message.l10nParams()).hasSize(1);
+    assertThat(message.l10nParams()[0]).isEqualTo("10");
+  }
+
+  @Test
+  public void test_text_message() throws Exception {
+    Result.Message txtMessage = Result.Message.of("the error");
+    Result.Message sameMessage = Result.Message.of("the error");
+    Result.Message otherMessage = Result.Message.of("other");
+
+    assertThat(txtMessage.toString()).contains("the error");
+    assertThat(txtMessage).isEqualTo(txtMessage);
+    assertThat(txtMessage).isEqualTo(sameMessage);
+    assertThat(txtMessage.hashCode()).isEqualTo(txtMessage.hashCode());
+    assertThat(txtMessage.hashCode()).isEqualTo(sameMessage.hashCode());
+    assertThat(txtMessage).isNotEqualTo(otherMessage);
+    assertThat(txtMessage).isNotEqualTo("the error");
+  }
+
+  @Test
+  public void test_l10n_message() throws Exception {
+    Result.Message msg = Result.Message.ofL10n("issue.error.123", "10");
+    Result.Message sameMsg = Result.Message.ofL10n("issue.error.123", "10");
+    Result.Message msg2 = Result.Message.ofL10n("issue.error.123", "200");
+    Result.Message msg3 = Result.Message.ofL10n("issue.error.50");
+
+    assertThat(msg.toString()).contains("issue.error.123").contains("10");
+    assertThat(msg).isEqualTo(msg);
+    assertThat(msg).isEqualTo(sameMsg);
+    assertThat(msg.hashCode()).isEqualTo(msg.hashCode());
+    assertThat(msg.hashCode()).isEqualTo(sameMsg.hashCode());
+
+    assertThat(msg).isNotEqualTo(msg2);
+    assertThat(msg).isNotEqualTo(msg3);
+    assertThat(msg).isNotEqualTo("issue.error.123");
+  }
+}

sonar-server/src/test/java/org/sonar/server/util/RubyUtilsTest.java

@@ -20,9 +20,11 @@
 package org.sonar.server.util;
 
 import org.junit.Test;
+import org.sonar.api.utils.DateUtils;
 
 import static java.util.Arrays.asList;
 import static org.fest.assertions.Assertions.assertThat;
+import static org.fest.assertions.Fail.fail;
 
 public class RubyUtilsTest {
   @Test
@@ -34,13 +36,97 @@ public class RubyUtilsTest {
     assertThat(RubyUtils.toStrings(asList("foo", "bar"))).containsOnly("foo", "bar");
   }
 
+  @Test
+  public void toInteger() throws Exception {
+    assertThat(RubyUtils.toInteger(null)).isNull();
+    assertThat(RubyUtils.toInteger("")).isNull();
+    assertThat(RubyUtils.toInteger("   ")).isNull();
+    assertThat(RubyUtils.toInteger("123")).isEqualTo(123);
+    assertThat(RubyUtils.toInteger(123)).isEqualTo(123);
+    assertThat(RubyUtils.toInteger(123L)).isEqualTo(123);
+  }
+
+  @Test
+  public void toInteger_unexpected_class() throws Exception {
+    try {
+      RubyUtils.toInteger(1.2);
+      fail();
+    } catch (IllegalArgumentException e) {
+      // ok
+    }
+  }
+
+  @Test
+  public void toDouble() throws Exception {
+    assertThat(RubyUtils.toDouble(null)).isNull();
+    assertThat(RubyUtils.toDouble("")).isNull();
+    assertThat(RubyUtils.toDouble("  ")).isNull();
+    assertThat(RubyUtils.toDouble("123")).isEqualTo(123.0);
+    assertThat(RubyUtils.toDouble("3.14")).isEqualTo(3.14);
+    assertThat(RubyUtils.toDouble(3.14)).isEqualTo(3.14);
+    assertThat(RubyUtils.toDouble(123)).isEqualTo(123.0);
+    assertThat(RubyUtils.toDouble(123L)).isEqualTo(123.0);
+  }
+
+  @Test
+  public void toDouble_unexpected_class() throws Exception {
+    try {
+      RubyUtils.toDouble(true);
+      fail();
+    } catch (IllegalArgumentException e) {
+      // ok
+    }
+  }
+
   @Test
   public void toBoolean() throws Exception {
     assertThat(RubyUtils.toBoolean(null)).isNull();
+    assertThat(RubyUtils.toBoolean("")).isNull();
+    assertThat(RubyUtils.toBoolean("  ")).isNull();
     assertThat(RubyUtils.toBoolean("true")).isTrue();
     assertThat(RubyUtils.toBoolean(true)).isTrue();
     assertThat(RubyUtils.toBoolean("false")).isFalse();
     assertThat(RubyUtils.toBoolean(false)).isFalse();
+  }
+
+  @Test
+  public void toBoolean_unexpected_class() throws Exception {
+    try {
+      RubyUtils.toBoolean(333);
+      fail();
+    } catch (IllegalArgumentException e) {
+      // ok
+    }
+  }
 
+  @Test
+  public void toDate() throws Exception {
+    assertThat(RubyUtils.toDate(null)).isNull();
+    assertThat(RubyUtils.toDate("")).isNull();
+    assertThat(RubyUtils.toDate("   ")).isNull();
+    assertThat(RubyUtils.toDate("2013-01-18").getDate()).isEqualTo(18);
+    assertThat(RubyUtils.toDate("2013-01-18T15:38:19+0200").getDate()).isEqualTo(18);
+    assertThat(RubyUtils.toDate("2013-01-18T15:38:19+0200").getMinutes()).isEqualTo(38);
+    assertThat(RubyUtils.toDate(DateUtils.parseDate("2013-01-18")).getDate()).isEqualTo(18);
+  }
+
+  @Test
+  public void toDate_bad_format() throws Exception {
+    try {
+      RubyUtils.toDate("01/02/2013");
+      fail();
+    } catch (Exception e) {
+      // ok
+    }
+  }
+
+  @Test
+  public void toDate_unexpected_class() throws Exception {
+    try {
+      RubyUtils.toDate(333);
+      fail();
+    } catch (IllegalArgumentException e) {
+      // ok
+    }
   }
 }