Display the bulk edit form with error messages when some issues can not be saved (#13943).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@11786 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/application_controller.rb

@@ -555,21 +555,6 @@ class ApplicationController < ActionController::Base
     flash[:warning] = l(:warning_attachments_not_saved, obj.unsaved_attachments.size) if obj.unsaved_attachments.present?
   end
 
-  # Sets the `flash` notice or error based the number of issues that did not save
-  #
-  # @param [Array, Issue] issues all of the saved and unsaved Issues
-  # @param [Array, Integer] unsaved_issue_ids the issue ids that were not saved
-  def set_flash_from_bulk_issue_save(issues, unsaved_issue_ids)
-    if unsaved_issue_ids.empty?
-      flash[:notice] = l(:notice_successful_update) unless issues.empty?
-    else
-      flash[:error] = l(:notice_failed_to_save_issues,
-                        :count => unsaved_issue_ids.size,
-                        :total => issues.size,
-                        :ids => '#' + unsaved_issue_ids.join(', #'))
-    end
-  end
-
   # Rescues an invalid query statement. Just in case...
   def query_statement_invalid(exception)
     logger.error "Query::StatementInvalid: #{exception.message}" if logger

app/controllers/issues_controller.rb

@@ -241,7 +241,6 @@ class IssuesController < ApplicationController
     end
 
     @safe_attributes = @issues.map(&:safe_attribute_names).reduce(:&)
-    render :layout => false if request.xhr?
   end
 
   def bulk_update
@@ -249,8 +248,8 @@ class IssuesController < ApplicationController
     @copy = params[:copy].present?
     attributes = parse_params_for_bulk_issue_attributes(params)
 
-    unsaved_issue_ids = []
+    unsaved_issues = []
-    moved_issues = []
+    saved_issues = []
 
     if @copy && params[:copy_subtasks].present?
       # Descendant issues will be copied with the parent task
@@ -270,23 +269,29 @@ class IssuesController < ApplicationController
       issue.safe_attributes = attributes
       call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
       if issue.save
-        moved_issues << issue
+        saved_issues << issue
       else
-        logger.info "issue could not be updated or copied: #{issue.errors.full_messages}" if logger && logger.info
+        unsaved_issues << issue
-        # Keep unsaved issue ids to display them in flash error
-        unsaved_issue_ids << issue.id
       end
     end
-    set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
 
-    if params[:follow]
+    if unsaved_issues.empty?
-      if @issues.size == 1 && moved_issues.size == 1
+      flash[:notice] = l(:notice_successful_update) unless saved_issues.empty?
-        redirect_to issue_path(moved_issues.first)
+      if params[:follow]
-      elsif moved_issues.map(&:project).uniq.size == 1
+        if @issues.size == 1 && saved_issues.size == 1
-        redirect_to project_issues_path(moved_issues.map(&:project).first)
+          redirect_to issue_path(saved_issues.first)
+        elsif saved_issues.map(&:project).uniq.size == 1
+          redirect_to project_issues_path(saved_issues.map(&:project).first)
+        end
+      else
+        redirect_back_or_default _project_issues_path(@project)
       end
     else
-      redirect_back_or_default _project_issues_path(@project)
+      @saved_issues = @issues
+      @unsaved_issues = unsaved_issues
+      @issues = Issue.visible.find_all_by_id(@unsaved_issues.map(&:id))
+      bulk_edit
+      render :action => 'bulk_edit'
     end
   end
 

app/helpers/issues_helper.rb

@@ -94,6 +94,20 @@ module IssuesHelper
     s.html_safe
   end
 
+  # Returns an array of error messages for bulk edited issues
+  def bulk_edit_error_messages(issues)
+    messages = {}
+    issues.each do |issue|
+      issue.errors.full_messages.each do |message|
+        messages[message] ||= []
+        messages[message] << issue
+      end
+    end
+    messages.map { |message, issues|
+      "#{message}: " + issues.map {|i| "##{i.id}"}.join(', ')
+    }
+ end
+
   # Returns a link for adding a new subtask to the given issue
   def link_to_new_subtask(issue)
     attrs = {

app/views/issues/bulk_edit.html.erb

@@ -1,5 +1,21 @@
 <h2><%= @copy ? l(:button_copy) : l(:label_bulk_edit_selected_issues) %></h2>
 
+<% if @saved_issues && @unsaved_issues.present? %>
+<div id="errorExplanation">
+  <span>
+    <%= l(:notice_failed_to_save_issues,
+        :count => @unsaved_issues.size,
+        :total => @saved_issues.size,
+        :ids => @unsaved_issues.map {|i| "##{i.id}"}.join(', ')) %>
+  </span>
+  <ul>
+  <% bulk_edit_error_messages(@unsaved_issues).each do |message| %>
+    <li><%= message %></li>
+  <% end %>
+  </ul>
+</div>
+<% end %>
+
 <ul id="bulk-selection">
 <% @issues.each do |issue| %>
   <%= content_tag 'li', link_to_issue(issue) %>

test/functional/issues_controller_test.rb

@@ -3588,13 +3588,32 @@ class IssuesControllerTest < ActionController::TestCase
     assert_redirected_to :controller => 'issues', :action => 'index', :project_id => Project.find(1).identifier
   end
 
-  def test_bulk_update_with_failure_should_set_flash
+  def test_bulk_update_with_all_failures_should_show_errors
     @request.session[:user_id] = 2
-    Issue.update_all("subject = ''", "id = 2") # Make it invalid
+    post :bulk_update, :ids => [1, 2], :issue => {:start_date => 'foo'}
-    post :bulk_update, :ids => [1, 2], :issue => {:priority_id => 6}
 
-    assert_redirected_to :controller => 'issues', :action => 'index', :project_id => 'ecookbook'
+    assert_response :success
-    assert_equal 'Failed to save 1 issue(s) on 2 selected: #2.', flash[:error]
+    assert_template 'bulk_edit'
+    assert_select '#errorExplanation span', :text => 'Failed to save 2 issue(s) on 2 selected: #1, #2.'
+    assert_select '#errorExplanation ul li', :text => 'Start date is not a valid date: #1, #2'
+
+    assert_equal [1, 2], assigns[:issues].map(&:id)
+  end
+
+  def test_bulk_update_with_some_failures_should_show_errors
+    issue1 = Issue.generate!(:start_date => '2013-05-12')
+    issue2 = Issue.generate!(:start_date => '2013-05-15')
+    issue3 = Issue.generate!
+
+    @request.session[:user_id] = 2
+    post :bulk_update, :ids => [issue1.id, issue2.id, issue3.id], :issue => {:due_date => '2013-05-01'}
+
+    assert_response :success
+    assert_template 'bulk_edit'
+    assert_select '#errorExplanation span', :text => "Failed to save 2 issue(s) on 3 selected: ##{issue1.id}, ##{issue2.id}."
+    assert_select '#errorExplanation ul li', :text => "Due date must be greater than start date: ##{issue1.id}, ##{issue2.id}"
+
+    assert_equal [issue1.id, issue2.id], assigns[:issues].map(&:id)
   end
 
   def test_get_bulk_copy