From 6f42fab84d023994494fa1bb8e0b8792565521b6 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Fri, 17 May 2013 16:34:18 +0200 Subject: [PATCH] SONAR-4304 better exception handling/UI --- .../resources/org/sonar/l10n/core.properties | 4 +- .../sonar/core/rule/DefaultRuleFinder.java | 4 + .../java/org/sonar/api/rules/RuleFinder.java | 4 + .../issue/InternalRubyIssueService.java | 70 ++++++++----- .../org/sonar/server/issue/IssueService.java | 42 ++++++-- .../java/org/sonar/server/issue/Result.java | 74 +++++++++++--- .../java/org/sonar/server/util/RubyUtils.java | 47 +++++++-- .../app/controllers/api/issues_controller.rb | 37 +++++-- .../app/controllers/issue_controller.rb | 13 +-- .../app/views/issue/_assign_form.html.erb | 2 +- .../app/views/issue/_create_form.html.erb | 57 ++++++----- .../app/views/issue/_plan_form.html.erb | 2 +- .../views/shared/_result_messages.html.erb | 5 + .../shared/_source_violation_form.html.erb | 4 +- .../src/main/webapp/javascripts/issue.js | 31 ++++-- .../src/main/webapp/stylesheets/style.css | 2 +- .../issue/InternalRubyIssueServiceTest.java | 20 ++-- .../org/sonar/server/issue/ResultTest.java | 99 +++++++++++++++++++ .../org/sonar/server/util/RubyUtilsTest.java | 86 ++++++++++++++++ 19 files changed, 482 insertions(+), 121 deletions(-) create mode 100644 sonar-server/src/main/webapp/WEB-INF/app/views/shared/_result_messages.html.erb create mode 100644 sonar-server/src/test/java/org/sonar/server/issue/ResultTest.java diff --git a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties index 92b4a6af2c2..f0cd0148d0d 100644 --- a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties +++ b/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 diff --git a/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java b/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java index 667549554be..c7399c00b9c 100644 --- a/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java +++ b/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( diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleFinder.java b/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleFinder.java index 8ea05be6c07..d978622e79d 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleFinder.java +++ b/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); 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 8a7c2ae3dc3..0c74050f444 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 @@ -109,19 +109,43 @@ public class InternalRubyIssueService implements ServerComponent { return commentService.findComment(commentKey); } - public Issue create(Map 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 create(Map params) { + Result 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 findOpenActionPlans(String issueKey) { @@ -153,7 +177,7 @@ public class InternalRubyIssueService implements ServerComponent { DefaultActionPlan existingActionPlan = (DefaultActionPlan) actionPlanService.findByKey(key); if (existingActionPlan == null) { Result 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 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 createResultForExistingActionPlan(String actionPlanKey) { Result 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; } diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java index 8bb17347e1a..14ea05ed66a 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java +++ b/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. + *

+ * Never return null, but return an empty list if the issue does not exist. + */ public List listTransitions(String issueKey) { DefaultIssue issue = loadIssue(issueKey); + if (issue == null) { + return Collections.emptyList(); + } List transitions = workflow.outTransitions(issue); Collections.sort(transitions, new Comparator() { @Override @@ -72,7 +86,13 @@ public class IssueService implements ServerComponent { return transitions; } - public List listTransitions(Issue issue) { + /** + * Never return null, but an empty list if the issue does not exist. + */ + public List listTransitions(@Nullable Issue issue) { + if (issue == null) { + return Collections.emptyList(); + } List transitions = workflow.outTransitions(issue); Collections.sort(transitions, new Comparator() { @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); } diff --git a/sonar-server/src/main/java/org/sonar/server/issue/Result.java b/sonar-server/src/main/java/org/sonar/server/issue/Result.java index 587114e8b3a..9f03d82f4db 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/Result.java +++ b/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 { private T object = null; - private List errors = newArrayList(); - ; + private final List errors = newArrayList(); private Result(@Nullable T object) { this.object = object; @@ -55,35 +57,69 @@ public class Result { return object; } - public Result addError(String text, Object... params) { - Message message = new Message(text, params); + public Result addError(String text) { + return addError(Message.of(text)); + } + + public Result addError(Message message) { errors.add(message); return this; } + /** + * List of error messages. Empty if there are no errors. + */ public List 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 { 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(); + } } } diff --git a/sonar-server/src/main/java/org/sonar/server/util/RubyUtils.java b/sonar-server/src/main/java/org/sonar/server/util/RubyUtils.java index 45257af5844..8ac178283d0 100644 --- a/sonar-server/src/main/java/org/sonar/server/util/RubyUtils.java +++ b/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()); } } diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/issues_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/issues_controller.rb index 8c4b99062a6..7620b02b60d 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/issues_controller.rb +++ b/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 diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/issue_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/issue_controller.rb index e25ba57f6db..83a806f7a88 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/issue_controller.rb +++ b/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 diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_assign_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_assign_form.html.erb index 05ec1f1b998..2d8be33f28f 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_assign_form.html.erb +++ b/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) -%>  <%= link_to_function message('cancel'), 'closeIssueForm(this)' -%>  - + diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_create_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_create_form.html.erb index 95e12c5d73d..9f6d311bfcf 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_create_form.html.erb +++ b/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" %> -

- No rules. Please contact the administrator. -  <%= link_to_function message('cancel'), 'closeMIF(this)' -%> -
+
+ <% + if manual_rules.empty? + %> +
+ + <%= message('issue.manual.no_rules') -%> + <% if is_admin %> + <%= message('manage') -%> + <% else %> + <%= message('contact_admin') -%> + <% end %> + +  <%= link_to_function message('cancel'), 'closeCreateIssueForm(this)' -%> +
+ + <% else %> -<% else %> -
- - - - - -
- <% 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 %> - - <% if is_admin %> - Manage Rules - <% end %> -
- + <%= 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} -%>
-
@@ -45,11 +41,14 @@
-  <%= link_to_function message('cancel'), 'closeMIF(this)' -%> +   + <%= link_to_function message('cancel'), 'closeCreateIssueForm(this)' -%>   +
+
-
-<% end %> \ No newline at end of file + <% end %> + \ No newline at end of file diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_plan_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_plan_form.html.erb index 244f36b4b29..0749fc032e5 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_plan_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_plan_form.html.erb @@ -20,7 +20,7 @@  <%= link_to_function message('cancel'), 'closeIssueForm(this)' -%>  - + diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_result_messages.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_result_messages.html.erb new file mode 100644 index 00000000000..9f8a57d72d2 --- /dev/null +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_result_messages.html.erb @@ -0,0 +1,5 @@ +<% result.errors.each do |msg| %> +
<%= h (msg.text ? msg.text : Api::Utils.message(msg.l10nKey, :params => msg.l10nParams)) -%>
+<% end %> + + diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_source_violation_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_source_violation_form.html.erb index caec3157b91..f94f5bdf557 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_source_violation_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/shared/_source_violation_form.html.erb @@ -1,3 +1 @@ - - - + diff --git a/sonar-server/src/main/webapp/javascripts/issue.js b/sonar-server/src/main/webapp/javascripts/issue.js index bbccd800575..e4db7200716 100644 --- a/sonar-server/src/main/webapp/javascripts/issue.js +++ b/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; } diff --git a/sonar-server/src/main/webapp/stylesheets/style.css b/sonar-server/src/main/webapp/stylesheets/style.css index 6f9c6471e23..1d0db20f278 100644 --- a/sonar-server/src/main/webapp/stylesheets/style.css +++ b/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 { 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 d13d50de187..e450e449ed9 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 @@ -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 = ""; diff --git a/sonar-server/src/test/java/org/sonar/server/issue/ResultTest.java b/sonar-server/src/test/java/org/sonar/server/issue/ResultTest.java new file mode 100644 index 00000000000..af0613ae7ef --- /dev/null +++ b/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 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 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 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"); + } +} diff --git a/sonar-server/src/test/java/org/sonar/server/util/RubyUtilsTest.java b/sonar-server/src/test/java/org/sonar/server/util/RubyUtilsTest.java index afd8fcdda3a..ba49d0dc81f 100644 --- a/sonar-server/src/test/java/org/sonar/server/util/RubyUtilsTest.java +++ b/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 + } } } -- 2.39.5