Better handling of update failures when bulk editing time entries.

git-svn-id: http://svn.redmine.org/redmine/trunk@16477 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/timelog_controller.rb

@@ -172,19 +172,33 @@ class TimelogController < ApplicationController
   def bulk_update
     attributes = parse_params_for_bulk_update(params[:time_entry])
 
-    unsaved_time_entry_ids = []
+    unsaved_time_entries = []
+    saved_time_entries = []
+
     @time_entries.each do |time_entry|
       time_entry.reload
       time_entry.safe_attributes = attributes
       call_hook(:controller_time_entries_bulk_edit_before_save, { :params => params, :time_entry => time_entry })
-      unless time_entry.save
-        logger.info "time entry could not be updated: #{time_entry.errors.full_messages}" if logger && logger.info?
-        # Keep unsaved time_entry ids to display them in flash error
-        unsaved_time_entry_ids << time_entry.id
+      if time_entry.save
+        saved_time_entries << time_entry
+      else
+        unsaved_time_entries << time_entry
       end
     end
-    set_flash_from_bulk_time_entry_save(@time_entries, unsaved_time_entry_ids)
-    redirect_back_or_default project_time_entries_path(@projects.first)
+
+    if unsaved_time_entries.empty?
+      flash[:notice] = l(:notice_successful_update) unless saved_time_entries.empty?
+      redirect_back_or_default project_time_entries_path(@projects.first)
+    else
+      @saved_time_entries = @time_entries
+      @unsaved_time_entries = unsaved_time_entries
+      @time_entries = TimeEntry.where(:id => unsaved_time_entries.map(&:id)).
+        preload(:project => :time_entry_activities).
+        preload(:user).to_a
+
+      bulk_edit
+      render :action => 'bulk_edit'
+    end
   end
 
   def destroy
@@ -243,17 +257,6 @@ private
     render_404
   end
 
-  def set_flash_from_bulk_time_entry_save(time_entries, unsaved_time_entry_ids)
-    if unsaved_time_entry_ids.empty?
-      flash[:notice] = l(:notice_successful_update) unless time_entries.empty?
-    else
-      flash[:error] = l(:notice_failed_to_save_time_entries,
-                        :count => unsaved_time_entry_ids.size,
-                        :total => time_entries.size,
-                        :ids => '#' + unsaved_time_entry_ids.join(', #'))
-    end
-  end
-
   def find_optional_issue
     if params[:issue_id].present?
       @issue = Issue.find(params[:issue_id])

app/views/timelog/bulk_edit.html.erb

@@ -1,5 +1,21 @@
 <h2><%= l(:label_bulk_edit_selected_time_entries) %></h2>
 
+<% if @unsaved_time_entries.present? %>
+<div id="errorExplanation">
+  <span>
+    <%= l(:notice_failed_to_save_time_entries,
+        :count => @unsaved_time_entries.size,
+        :total => @saved_time_entries.size,
+        :ids => @unsaved_time_entries.map {|i| "##{i.id}"}.join(', ')) %>
+  </span>
+  <ul>
+  <% bulk_edit_error_messages(@unsaved_time_entries).each do |message| %>
+    <li><%= message %></li>
+  <% end %>
+  </ul>
+</div>
+<% end %>
+
 <ul id="bulk-selection">
 <% @time_entries.each do |entry| %>
   <%= content_tag 'li',
@@ -12,29 +28,29 @@
 <div class="box tabular">
   <div>
     <p>
-      <label><%= l(:field_issue) %></label>
+      <label for="time_entry_issue_id"><%= l(:field_issue) %></label>
       <%= text_field :time_entry, :issue_id, :size => 6 %>
     </p>
 
     <p>
-      <label><%= l(:field_spent_on) %></label>
+      <label for="time_entry_spent_on"><%= l(:field_spent_on) %></label>
       <%= date_field :time_entry, :spent_on, :size => 10 %><%= calendar_for('time_entry_spent_on') %>
     </p>
 
     <p>
-      <label><%= l(:field_hours) %></label>
+      <label for="time_entry_hours"><%= l(:field_hours) %></label>
       <%= text_field :time_entry, :hours, :size => 6 %>
     </p>
 
     <% if @available_activities.any? %>
     <p>
-      <label><%= l(:field_activity) %></label>
+      <label for="time_entry_activity_id"><%= l(:field_activity) %></label>
       <%= select_tag('time_entry[activity_id]', content_tag('option', l(:label_no_change_option), :value => '') + options_from_collection_for_select(@available_activities, :id, :name)) %>
     </p>
     <% end %>
 
     <p>
-      <label><%= l(:field_comments) %></label>
+      <label for="time_entry_comments"><%= l(:field_comments) %></label>
       <%= text_field(:time_entry, :comments, :size => 100) %>
     </p>
 

test/functional/timelog_controller_test.rb

@@ -582,8 +582,8 @@ class TimelogControllerTest < Redmine::ControllerTest
     @request.session[:user_id] = 2
     post :bulk_update, :params => {:ids => [1, 2], :time_entry => { :hours => 'A'}}
 
-    assert_response 302
-    assert_match /Failed to save 2 time entrie/, flash[:error]
+    assert_response :success
+    assert_select_error /Failed to save 2 time entrie/
   end
 
   def test_bulk_update_on_different_projects

test/ui/timelog_test_ui.rb

@@ -20,7 +20,8 @@ require File.expand_path('../base', __FILE__)
 class Redmine::UiTest::TimelogTest < Redmine::UiTest::Base
   fixtures :projects, :users, :email_addresses, :roles, :members, :member_roles,
            :trackers, :projects_trackers, :enabled_modules, :issue_statuses, :issues,
-           :enumerations, :custom_fields, :custom_values, :custom_fields_trackers
+           :enumerations, :custom_fields, :custom_values, :custom_fields_trackers,
+           :time_entries
 
   def test_changing_project_should_update_activities
     project = Project.find(1)
@@ -41,4 +42,31 @@ class Redmine::UiTest::TimelogTest < Redmine::UiTest::Base
       assert !has_content?('Design')
     end
   end
+
+  def test_bulk_edit
+    log_user 'jsmith', 'jsmith'
+    visit '/time_entries/bulk_edit?ids[]=1&ids[]=2&ids[]=3'
+    fill_in 'Hours', :with => '8.5'
+    select 'QA', :from => 'Activity'
+    page.first(:button, 'Submit').click
+
+    entries = TimeEntry.where(:id => [1,2,3]).to_a
+    assert entries.all? {|entry| entry.hours == 8.5}
+    assert entries.all? {|entry| entry.activity.name == 'QA'}
+  end
+
+  def test_bulk_edit_with_failure
+    log_user 'jsmith', 'jsmith'
+    visit '/time_entries/bulk_edit?ids[]=1&ids[]=2&ids[]=3'
+    fill_in 'Hours', :with => 'A'
+    page.first(:button, 'Submit').click
+
+    assert page.has_css?('#errorExplanation')
+    fill_in 'Hours', :with => '7'
+    page.first(:button, 'Submit').click
+
+    assert_equal "/projects/ecookbook/time_entries", current_path
+    entries = TimeEntry.where(:id => [1,2,3]).to_a
+    assert entries.all? {|entry| entry.hours == 7.0}
+  end
 end