]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-4394 Improve ui error handling
authorJulien Lancelot <julien.lancelot@gmail.com>
Thu, 27 Jun 2013 12:54:56 +0000 (14:54 +0200)
committerJulien Lancelot <julien.lancelot@gmail.com>
Thu, 27 Jun 2013 12:54:56 +0000 (14:54 +0200)
sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
sonar-server/src/main/webapp/WEB-INF/app/controllers/issues_controller.rb
sonar-server/src/main/webapp/WEB-INF/app/views/issues/_bulk_change_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_copy_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_edit_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_save_as_form.html.erb
sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_shared_form.html.erb
sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java

index 359574d837e21bd684f9bb3b75877a9a7a056b4e..22b0c9e86765deca9e7e9924aa6e79f54e145824 100644 (file)
@@ -459,7 +459,7 @@ public class InternalRubyIssueService implements ServerComponent {
    * Create issue filter
    */
   public Result<DefaultIssueFilter> createIssueFilter(Map<String, String> parameters) {
-    Result<DefaultIssueFilter> result = createIssueFilterResult(parameters, false);
+    Result<DefaultIssueFilter> result = createIssueFilterResultForNew(parameters);
     if (result.ok()) {
       try {
         result.set(issueFilterService.save(result.get(), UserSession.get()));
@@ -474,7 +474,7 @@ public class InternalRubyIssueService implements ServerComponent {
    * Update issue filter
    */
   public Result<DefaultIssueFilter> updateIssueFilter(Map<String, String> parameters) {
-    Result<DefaultIssueFilter> result = createIssueFilterResult(parameters, true);
+    Result<DefaultIssueFilter> result = createIssueFilterResultForUpdate(parameters);
     if (result.ok()) {
       try {
         result.set(issueFilterService.update(result.get(), UserSession.get()));
@@ -515,7 +515,7 @@ public class InternalRubyIssueService implements ServerComponent {
    * Copy issue filter
    */
   public Result<DefaultIssueFilter> copyIssueFilter(Long issueFilterIdToCopy, Map<String, String> parameters) {
-    Result<DefaultIssueFilter> result = createIssueFilterResult(parameters, false);
+    Result<DefaultIssueFilter> result = createIssueFilterResultForCopy(parameters);
     if (result.ok()) {
       try {
         result.set(issueFilterService.copy(issueFilterIdToCopy, result.get(), UserSession.get()));
@@ -527,7 +527,22 @@ public class InternalRubyIssueService implements ServerComponent {
   }
 
   @VisibleForTesting
-  Result<DefaultIssueFilter> createIssueFilterResult(Map<String, String> params, boolean isUpdate) {
+  Result<DefaultIssueFilter> createIssueFilterResultForNew(Map<String, String> params) {
+    return createIssueFilterResult(params, false, false);
+  }
+
+  @VisibleForTesting
+  Result<DefaultIssueFilter> createIssueFilterResultForUpdate(Map<String, String> params) {
+    return createIssueFilterResult(params, true, true);
+  }
+
+  @VisibleForTesting
+  Result<DefaultIssueFilter> createIssueFilterResultForCopy(Map<String, String> params) {
+    return createIssueFilterResult(params, false, false);
+  }
+
+  @VisibleForTesting
+  Result<DefaultIssueFilter> createIssueFilterResult(Map<String, String> params, boolean showCheckId, boolean showCheckUser) {
     Result<DefaultIssueFilter> result = Result.of();
 
     String id = params.get("id");
@@ -538,9 +553,12 @@ public class InternalRubyIssueService implements ServerComponent {
     Boolean sharedParam = RubyUtils.toBoolean(params.get("shared"));
     boolean shared = sharedParam != null ? sharedParam : false;
 
-    if (isUpdate) {
+    if (showCheckId) {
       checkMandatoryParameter(id, "id", result);
     }
+    if (showCheckUser) {
+      checkMandatoryParameter(user, "user", result);
+    }
     checkMandatorySizeParameter(name, "name", 100, result);
     checkOptionalSizeParameter(description, "description", 4000, result);
 
@@ -550,7 +568,7 @@ public class InternalRubyIssueService implements ServerComponent {
         .setShared(shared)
         .setUser(user)
         .setData(data);
-      if (isUpdate) {
+      if (!Strings.isNullOrEmpty(id)) {
         defaultIssueFilter.setId(Long.valueOf(id));
       }
 
index 1e2175edcb3f760af9455e60792330c8394de38a..36edab2df955a7b8e161f9eda48df5596b1f521d 100644 (file)
@@ -86,7 +86,7 @@ class IssuesController < ApplicationController
       render :text => @filter.id.to_s, :status => 200
     else
       @errors = filter_result.errors
-      render :partial => 'issues/filter_save_as_form', :status => 400
+      render :partial => 'issues/display_errors', :status => 400
     end
   end
 
@@ -131,7 +131,7 @@ class IssuesController < ApplicationController
       render :text => @filter.id.to_s, :status => 200
     else
       @errors = filter_result.errors
-      render :partial => 'issues/filter_edit_form', :status => 400
+      render :partial => 'issues/display_errors', :status => 400
     end
   end
 
@@ -154,7 +154,7 @@ class IssuesController < ApplicationController
       render :text => @filter.id.to_s, :status => 200
     else
       @errors = filter_result.errors
-      render :partial => 'issues/filter_copy_form', :status => 400
+      render :partial => 'issues/display_errors', :status => 400
     end
   end
 
@@ -212,7 +212,7 @@ class IssuesController < ApplicationController
       render :text => params[:criteria_params], :status => 200
     else
       @errors = result.errors
-      render :partial => 'issues/bulk_change_form', :status => 400
+      render :partial => 'issues/display_errors', :status => 400
     end
   end
 
index 812e29175c88c98ec73dd1466c0fac0511bdadc4..0d2ad525694f3b699c4b5bd6000f3c6caffe467f 100644 (file)
@@ -1,19 +1,12 @@
-<%
-   project = params[:project] || @project
-   issues = params[:issues].split(',') if params[:issues]
-   issues = @issues unless issues
-%>
 <form id="bulk-change-form" method="post" action="<%= ApplicationController.root_context -%>/issues/bulk_change">
-  <input type="hidden" name="issues" value="<%= params[:issues] || @issues.join(',') -%>">
-  <input type="hidden" name="criteria_params" value="<%= params[:criteria_params] || @criteria_params.to_query -%>">
-  <input type="hidden" name="issues_by_transitions" value="<%= @transitions_by_issues.to_query -%>">
-  <input type="hidden" name="project" value="<%= project -%>">
+  <input type="hidden" name="issues" value="<%= @issues.join(',') -%>">
+  <input type="hidden" name="criteria_params" value="<%= @criteria_params.to_query -%>">
   <fieldset>
     <div class="modal-head">
-      <h2><%= message('issue_filter.bulk_change.form.title', {:params => issues.size.to_s}) -%></h2>
+      <h2><%= message('issue_filter.bulk_change.form.title', {:params => @issues.size.to_s}) -%></h2>
     </div>
     <div class="modal-body">
-      <%= render :partial => 'display_errors' %>
+      <div class="bulk-change errors" style="display:none;" />
       <div class="modal-field">
         <label for="assignee">
           <input id="assign-action" name="actions[]" type="checkbox" value="assign"/>
@@ -23,8 +16,8 @@
           :include_choices => {'' => escape_javascript(message('unassign')), current_user.login => escape_javascript(message('assign_to_me'))}) -%>
       </div>
       <%
-         if project && !project.blank?
-           plans = Internal.issues.findOpenActionPlans(project)
+         if @project && !@project.blank?
+           plans = Internal.issues.findOpenActionPlans(@project)
            plan_options = ""
            unless plans.empty?
              first_plan = plans[0]
       </div>
     </div>
     <div class="modal-foot">
-      <input type="submit" value="<%= message('submit') -%>" id="bulk-change-submit"/>
+      <input type="submit" value="<%= message('submit') -%>" id="bulk-change-submit" class="bulk-change"/>
       <a href="#" onclick="return closeModalWindow()" id="bulk-change-cancel"><%= message('cancel') -%></a>
     </div>
   </fieldset>
 </form>
 <script>
-  $j("#bulk-change-form").modalForm({success: function (data) {
-    window.location = baseUrl + '/issues/search?' + data;
-  }});
+  $j("#bulk-change-form").modalForm({
+    success: function (data) {
+      window.location = baseUrl + '/issues/search?' + data;
+    },
+    error: function (xhr, textStatus, errorThrown) {
+      var htmlClass = 'bulk-change';
+      $j('.' + htmlClass + ' input[type=submit]').removeAttr('disabled');
+      $j('.' + htmlClass + '.errors').show();
+      $j('.' + htmlClass + '.errors').html(xhr.responseText);
+    }
+  });
 </script>
\ No newline at end of file
index 1979fc9356511737cf825d5d3ba3a31ad993148e..bc21f7ab96e3e75d03617f4d253ec8e3f8ec272d 100644 (file)
@@ -1,18 +1,26 @@
 <form id="copy-filter-form" method="post" action="<%= ApplicationController.root_context -%>/issues/copy">
-  <input type="hidden" name="id" value="<%= params[:id] || @filter.id -%>">
+  <input type="hidden" name="id" value="<%= @filter.id -%>">
   <fieldset>
     <div class="modal-head">
       <h2><%= message('issue_filter.copy_filter') -%></h2>
     </div>
     <%= render :partial => 'filter_shared_form' %>
     <div class="modal-foot">
-      <input type="submit" value="<%= message('copy') -%>" id="copy-submit"/>
+      <input type="submit" value="<%= message('copy') -%>" id="copy-submit" class="issue-filter"/>
       <a href="#" onclick="return closeModalWindow()" id="copy-cancel"><%= h message('cancel') -%></a>
     </div>
   </fieldset>
 </form>
 <script>
-  $j("#copy-filter-form").modalForm({success: function (data) {
-    window.location = baseUrl + '/issues/filter/' + data;
-  }});
+  $j("#copy-filter-form").modalForm({
+    success: function (data) {
+      window.location = baseUrl + '/issues/filter/' + data;
+    },
+    error: function (xhr, textStatus, errorThrown) {
+      var htmlClass = 'issue-filter';
+      $j('.' + htmlClass + ' input[type=submit]').removeAttr('disabled');
+      $j('.' + htmlClass + '.errors').show();
+      $j('.' + htmlClass + '.errors').html(xhr.responseText);
+    }
+  });
 </script>
\ No newline at end of file
index 115320895baded23fed536ca9603d9ae9a83d8b9..658fa0b9773a98be295fcc61c103e032609f5938 100644 (file)
@@ -1,18 +1,26 @@
 <form id="edit-filter-form" method="post" action="<%= ApplicationController.root_context -%>/issues/edit">
-  <input type="hidden" name="id" value="<%= params[:id] || @filter.id -%>">
+  <input type="hidden" name="id" value="<%= @filter.id -%>">
   <fieldset>
     <div class="modal-head">
       <h2><%= message('issue_filter.edit_filter') -%></h2>
     </div>
     <%= render :partial => 'filter_shared_form', :locals => {:display_owner => true} %>
     <div class="modal-foot">
-      <input type="submit" value="<%= message('save') -%>" id="save-submit"/>
+      <input type="submit" value="<%= message('save') -%>" id="save-submit" class="issue-filter"/>
       <a href="#" onclick="return closeModalWindow()" id="save-cancel"><%= message('cancel') -%></a>
     </div>
   </fieldset>
 </form>
 <script>
-  $j("#edit-filter-form").modalForm({success: function (data) {
-    window.location = baseUrl + '/issues/filter/' + data;
-  }});
+  $j("#edit-filter-form").modalForm({
+    success: function (data) {
+      window.location = baseUrl + '/issues/filter/' + data;
+    },
+    error: function (xhr, textStatus, errorThrown) {
+      var htmlClass = 'issue-filter';
+      $j('.' + htmlClass + ' input[type=submit]').removeAttr('disabled');
+      $j('.' + htmlClass + '.errors').show();
+      $j('.' + htmlClass + '.errors').html(xhr.responseText);
+    }
+  });
 </script>
\ No newline at end of file
index 4390f1c476347b8ef70fdfcea76e3a1bc04e98ad..49139576b3f243c237069aad6d59355b13060d39 100644 (file)
@@ -1,18 +1,26 @@
 <form id="save-as-filter-form" method="post" action="<%= ApplicationController.root_context -%>/issues/save_as">
-  <input type="hidden" name="data" value="<%= params[:data] || u(@filter_query_serialized) -%>">
+  <input type="hidden" name="data" value="<%= u(@filter_query_serialized) -%>">
   <fieldset>
     <div class="modal-head">
       <h2><%= message('issue_filter.save_filter') -%></h2>
     </div>
     <%= render :partial => 'filter_shared_form' %>
     <div class="modal-foot">
-      <input type="submit" value="<%= message('save') -%>" id="save-as-submit"/>
+      <input type="submit" value="<%= message('save') -%>" id="save-as-submit" class="issue-filter"/>
       <a href="#" onclick="return closeModalWindow()" id="save-as-cancel"><%= message('cancel') -%></a>
     </div>
   </fieldset>
 </form>
 <script>
-  $j("#save-as-filter-form").modalForm({success:function (data) {
-    window.location = baseUrl + '/issues/filter/' + data;
-  }});
+  $j("#save-as-filter-form").modalForm({
+    success: function (data) {
+      window.location = baseUrl + '/issues/filter/' + data;
+    },
+    error: function (xhr, textStatus, errorThrown) {
+      var htmlClass = 'issue-filter';
+      $j('.' + htmlClass + ' input[type=submit]').removeAttr('disabled');
+      $j('.' + htmlClass + '.errors').show();
+      $j('.' + htmlClass + '.errors').html(xhr.responseText);
+    }
+  });
 </script>
\ No newline at end of file
index f9574560297e75791860053f26480212c9c2c3f6..7dfaf2e776fab6b1c8ef3ce7b016ea14dde05b8b 100644 (file)
@@ -2,14 +2,14 @@
      display_owner = false
    end %>
 <div class="modal-body">
-  <%= render :partial => 'display_errors' %>
+  <div class="issue-filter errors" style="display:none;" />
   <div class="modal-field">
     <label for="name"><%= message('issue_filter.form.name') -%> <em class="mandatory">*</em></label>
-    <input id="name" name="name" type="text" size="50" maxlength="100" value="<%= params[:name] || (h(@filter.name) if @filter) -%>" autofocus="autofocus"/>
+    <input id="name" name="name" type="text" size="50" maxlength="100" value="<%= h(@filter.name) if @filter -%>" autofocus="autofocus"/>
   </div>
   <div class="modal-field">
     <label for="description"><%= message('issue_filter.form.description') -%></label>
-    <input id="description" name="description" type="text" size="50" maxlength="4000" value="<%= params[:description] || (h(@filter.description) if @filter)  -%>"/>
+    <input id="description" name="description" type="text" size="50" maxlength="4000" value="<%= h(@filter.description) if @filter  -%>"/>
   </div>
   <% if display_owner && can_be_reassigned_by(current_user, @filter) %>
     <% filter_owner = Api.users.findByLogin(@filter.user) %>
       <label for="user"><%= h message('issue_filter.form.owner') -%></label>
       <%= user_select_tag('user', :html_id => 'select-filter-owner', :selected_user => filter_owner) -%>
     </div>
+  <% else %>
+    <input id="user" name="user" type="hidden" value="<%= h(@filter.user) if @filter -%>"/>
   <% end %>
   <div class="modal-field">
     <label for="shared"><%= message('issue_filter.form.share') -%></label>
-    <input id="shared" name="shared" type="checkbox" value="true" <%= 'checked' if (params[:shared] || (@filter && @filter.shared)) -%>/>
+    <input id="shared" name="shared" type="checkbox" value="true" <%= 'checked' if (@filter && @filter.shared) -%>/>
   </div>
 </div>
\ No newline at end of file
index e2fe1a1ae6197594b99eff65e34f49ce737f298f..296a64778c7abf3c032d4d80ab5bb66b1677089c 100644 (file)
@@ -320,6 +320,7 @@ public class InternalRubyIssueServiceTest {
     Map<String, String> parameters = newHashMap();
     parameters.put("name", "Long term");
     parameters.put("description", "Long term issues");
+    parameters.put("user", "John");
 
     doThrow(new RuntimeException("Error")).when(issueFilterService).save(any(DefaultIssueFilter.class), any(UserSession.class));
     Result result = service.createIssueFilter(parameters);
@@ -333,6 +334,7 @@ public class InternalRubyIssueServiceTest {
     parameters.put("id", "10");
     parameters.put("name", "Long term");
     parameters.put("description", "Long term issues");
+    parameters.put("user", "John");
 
     Result<DefaultIssueFilter> result = service.updateIssueFilter(parameters);
     assertThat(result.ok()).isTrue();
@@ -358,6 +360,7 @@ public class InternalRubyIssueServiceTest {
     parameters.put("id", "10");
     parameters.put("name", "Long term");
     parameters.put("description", "Long term issues");
+    parameters.put("user", "John");
 
     doThrow(new RuntimeException("Error")).when(issueFilterService).update(any(DefaultIssueFilter.class), any(UserSession.class));
     Result result = service.updateIssueFilter(parameters);
@@ -414,6 +417,7 @@ public class InternalRubyIssueServiceTest {
     Map<String, String> parameters = newHashMap();
     parameters.put("name", "Copy of Long term");
     parameters.put("description", "Copy of Long term issues");
+    parameters.put("user", "John");
 
     doThrow(new RuntimeException("Error")).when(issueFilterService).copy(anyLong(), any(DefaultIssueFilter.class), any(UserSession.class));
     Result result = service.copyIssueFilter(1L, parameters);
@@ -422,50 +426,79 @@ public class InternalRubyIssueServiceTest {
   }
 
   @Test
-  public void should_get_error_on_issue_filter_result_when_no_name() {
+  public void should_get_error_on_create_issue_filter_result_when_no_name() {
     Map<String, String> parameters = newHashMap();
     parameters.put("name", null);
     parameters.put("description", "Long term issues");
+    parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResult(parameters, false);
+    Result result = service.createIssueFilterResultForNew(parameters);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "name"));
   }
 
   @Test
-  public void should_get_error_on_issue_filter_result_when_name_is_too_long() {
+  public void should_get_error_on_create_issue_filter_result_when_name_is_too_long() {
     Map<String, String> parameters = newHashMap();
     parameters.put("name", createLongString(101));
     parameters.put("description", "Long term issues");
+    parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResult(parameters, false);
+    Result result = service.createIssueFilterResultForNew(parameters);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "name", 100));
   }
 
   @Test
-  public void should_get_error_on_issue_filter_result_when_description_is_too_long() {
+  public void should_get_error_on_create_issue_filter_result_when_description_is_too_long() {
     Map<String, String> parameters = newHashMap();
     parameters.put("name", "Long term");
     parameters.put("description", createLongString(4001));
+    parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResult(parameters, false);
+    Result result = service.createIssueFilterResultForNew(parameters);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "description", 4000));
   }
 
   @Test
-  public void should_get_error_on_issue_filter_result_when_id_is_null_on_update() {
+  public void should_get_error_on_create_issue_filter_result_when_id_is_null_on_update() {
     Map<String, String> parameters = newHashMap();
     parameters.put("id", null);
     parameters.put("name", "Long term");
     parameters.put("description", "Long term issues");
+    parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResult(parameters, true);
+    Result result = service.createIssueFilterResultForUpdate(parameters);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "id"));
   }
 
+  @Test
+  public void should_get_error_on_create_issue_filter_result_when_user_is_null_on_update() {
+    Map<String, String> parameters = newHashMap();
+    parameters.put("id", "10");
+    parameters.put("name", "All Open Issues");
+    parameters.put("description", "Long term issues");
+    parameters.put("user", null);
+
+    Result result = service.createIssueFilterResultForUpdate(parameters);
+    assertThat(result.ok()).isFalse();
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "user"));
+  }
+
+  @Test
+  public void should_get_no_error_on_issue_filter_result_when_id_and_user_are_null_on_copy() {
+    Map<String, String> parameters = newHashMap();
+    parameters.put("id", null);
+    parameters.put("name", "Long term");
+    parameters.put("description", "Long term issues");
+    parameters.put("user", null);
+
+    Result result = service.createIssueFilterResultForCopy(parameters);
+    assertThat(result.ok()).isTrue();
+  }
+
   @Test
   public void should_execute_issue_filter_from_issue_query() {
     service.execute(Maps.<String, Object>newHashMap());