Bulk edit refactoring.

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

5 files changed, 53 insertions, 75 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 84dc92f11..accfae33c 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -260,29 +260,16 @@ class IssuesController < ApplicationController
   # Bulk edit a set of issues
   def bulk_edit
     if request.post?
-      tracker = params[:tracker_id].blank? ? nil : @project.trackers.find_by_id(params[:tracker_id])
-      status = params[:status_id].blank? ? nil : IssueStatus.find_by_id(params[:status_id])
-      priority = params[:priority_id].blank? ? nil : IssuePriority.find_by_id(params[:priority_id])
-      assigned_to = (params[:assigned_to_id].blank? || params[:assigned_to_id] == 'none') ? nil : User.find_by_id(params[:assigned_to_id])
-      category = (params[:category_id].blank? || params[:category_id] == 'none') ? nil : @project.issue_categories.find_by_id(params[:category_id])
-      fixed_version = (params[:fixed_version_id].blank? || params[:fixed_version_id] == 'none') ? nil : @project.shared_versions.find_by_id(params[:fixed_version_id])
-      custom_field_values = params[:custom_field_values] ? params[:custom_field_values].reject {|k,v| v.blank?} : nil
-
-      # Need to merge in the records found above for Issue#bulk_edit.
-      # Assuming this is done so the associations are only looked up once.
-      merged_params = params.merge({
-                                     :tracker => tracker,
-                                     :status => status,
-                                     :priority => priority,
-                                     :assigned_to => assigned_to,
-                                     :category => category,
-                                     :fixed_version => fixed_version,
-                                     :custom_field_values => custom_field_values
-                                   })
+      attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
+      attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
+      attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
       
-      unsaved_issue_ids = []      
+      unsaved_issue_ids = []
       @issues.each do |issue|
-        unless issue.bulk_edit(merged_params)
+        journal = issue.init_journal(User.current, params[:notes])
+        issue.safe_attributes = attributes
+        call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
+        unless issue.save
           # Keep unsaved issue ids to display them in flash error
           unsaved_issue_ids << issue.id
         end
diff --git a/app/helpers/custom_fields_helper.rb b/app/helpers/custom_fields_helper.rb
index 330b1aa93..40a4c7855 100644
--- a/app/helpers/custom_fields_helper.rb
+++ b/app/helpers/custom_fields_helper.rb
@@ -67,9 +67,9 @@ module CustomFieldsHelper
     custom_field_label_tag(name, custom_value) + custom_field_tag(name, custom_value)
   end
   
-  def custom_field_tag_for_bulk_edit(custom_field)
-    field_name = "custom_field_values[#{custom_field.id}]"
-    field_id = "custom_field_values_#{custom_field.id}"
+  def custom_field_tag_for_bulk_edit(name, custom_field)
+    field_name = "#{name}[custom_field_values][#{custom_field.id}]"
+    field_id = "#{name}_custom_field_values_#{custom_field.id}"
     case custom_field.field_format
       when "date"
         text_field_tag(field_name, '', :id => field_id, :size => 10) + 
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 8a1413600..63602dd21 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -136,24 +136,6 @@ class Issue < ActiveRecord::Base
     end
     return issue
   end
-
-  def bulk_edit(params)
-    journal = init_journal(User.current, params[:notes])
-    self.tracker = params[:tracker] if params[:tracker]
-    self.priority = params[:priority] if params[:priority]
-    self.assigned_to = params[:assigned_to] if params[:assigned_to] || params[:assigned_to_id] == 'none'
-    self.category = params[:category] if params[:category] || params[:category_id] == 'none'
-    self.fixed_version = params[:fixed_version] if params[:fixed_version] || params[:fixed_version_id] == 'none'
-    self.start_date = params[:start_date] unless params[:start_date].blank?
-    self.due_date = params[:due_date] unless params[:due_date].blank?
-    self.done_ratio = params[:done_ratio] unless params[:done_ratio].blank?
-    self.custom_field_values = params[:custom_field_values] if params[:custom_field_values] && !params[:custom_field_values].empty?
-    # TODO: Edit hook name
-    Redmine::Hook.call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => self })
-
-    # Don't save any change to the issue if the user is not authorized to apply the requested status
-    return (params[:status].nil? || (new_statuses_allowed_to(User.current).include?(params[:status]) && self.status = params[:status])) && save
-  end
   
   def priority_id=(pid)
     self.priority = nil
@@ -206,7 +188,13 @@ class Issue < ActiveRecord::Base
   # TODO: move workflow/permission checks from controllers to here
   def safe_attributes=(attrs, user=User.current)
     return if attrs.nil?
-    self.attributes = attrs.reject {|k,v| !SAFE_ATTRIBUTES.include?(k)}
+    attrs = attrs.reject {|k,v| !SAFE_ATTRIBUTES.include?(k)}
+    if attrs['status_id']
+      unless new_statuses_allowed_to(user).collect(&:id).include?(attrs['status_id'].to_i)
+        attrs.delete('status_id')
+      end
+    end
+    self.attributes = attrs
   end
   
   def done_ratio
diff --git a/app/views/issues/bulk_edit.rhtml b/app/views/issues/bulk_edit.rhtml
index b120f23c4..3cf9ec1b0 100644
--- a/app/views/issues/bulk_edit.rhtml
+++ b/app/views/issues/bulk_edit.rhtml
@@ -11,39 +11,39 @@
 <div class="splitcontentleft">
 <p>
 	<label><%= l(:field_tracker) %></label>
-	<%= select_tag('tracker_id', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(@project.trackers, :id, :name)) %>
+	<%= select_tag('issue[tracker_id]', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(@project.trackers, :id, :name)) %>
 </p>
 <% if @available_statuses.any? %>
 <p>
 	<label><%= l(:field_status) %></label>
-	<%= select_tag('status_id', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(@available_statuses, :id, :name)) %>
+	<%= select_tag('issue[status_id]', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(@available_statuses, :id, :name)) %>
 </p>
 <% end %>
 <p>
 	<label><%= l(:field_priority) %></label> 
-	<%= select_tag('priority_id', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(IssuePriority.all, :id, :name)) %>
+	<%= select_tag('issue[priority_id]', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(IssuePriority.all, :id, :name)) %>
 </p>
 <p>
 	<label><%= l(:field_assigned_to) %></label> 
-	<%= select_tag('assigned_to_id', content_tag('option', l(:label_no_change_option), :value => '') +
+	<%= select_tag('issue[assigned_to_id]', content_tag('option', l(:label_no_change_option), :value => '') +
                                  content_tag('option', l(:label_nobody), :value => 'none') +
                                  options_from_collection_for_select(@project.assignable_users, :id, :name)) %>
 </p>
 <p>
 	<label><%= l(:field_category) %></label> 
-	<%= select_tag('category_id', content_tag('option', l(:label_no_change_option), :value => '') +
+	<%= select_tag('issue[category_id]', content_tag('option', l(:label_no_change_option), :value => '') +
                                 content_tag('option', l(:label_none), :value => 'none') +
                                 options_from_collection_for_select(@project.issue_categories, :id, :name)) %>
 </p>
 <p>
 	<label><%= l(:field_fixed_version) %></label> 
-	<%= select_tag('fixed_version_id', content_tag('option', l(:label_no_change_option), :value => '') +
+	<%= select_tag('issue[fixed_version_id]', content_tag('option', l(:label_no_change_option), :value => '') +
                                    content_tag('option', l(:label_none), :value => 'none') +
                                    version_options_for_select(@project.shared_versions.open)) %>
 </p>
 
 <% @custom_fields.each do |custom_field| %>
-	<p><label><%= h(custom_field.name) %></label> <%= custom_field_tag_for_bulk_edit(custom_field) %></p>
+	<p><label><%= h(custom_field.name) %></label> <%= custom_field_tag_for_bulk_edit('issue', custom_field) %></p>
 <% end %>
 
 <%= call_hook(:view_issues_bulk_edit_details_bottom, { :issues => @issues }) %>
@@ -52,16 +52,16 @@
 <div class="splitcontentright">
 <p>
 	<label><%= l(:field_start_date) %></label>
-	<%= text_field_tag 'start_date', '', :size => 10 %><%= calendar_for('start_date') %>
+	<%= text_field_tag 'issue[start_date]', '', :size => 10 %><%= calendar_for('issue_start_date') %>
 </p>
 <p>
 	<label><%= l(:field_due_date) %></label> 
-	<%= text_field_tag 'due_date', '', :size => 10 %><%= calendar_for('due_date') %>
+	<%= text_field_tag 'issue[due_date]', '', :size => 10 %><%= calendar_for('issue_due_date') %>
 </p>
 <% if Issue.use_field_for_done_ratio? %>
 <p>
 	<label><%= l(:field_done_ratio) %></label> 
-	<%= select_tag 'done_ratio', options_for_select([[l(:label_no_change_option), '']] + (0..10).to_a.collect {|r| ["#{r*10} %", r*10] }) %>
+	<%= select_tag 'issue[done_ratio]', options_for_select([[l(:label_no_change_option), '']] + (0..10).to_a.collect {|r| ["#{r*10} %", r*10] }) %>
 </p>
 <% end %>
 </div>
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 83a2824fb..94a74ba28 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -899,20 +899,21 @@ class IssuesControllerTest < ActionController::TestCase
     field = CustomField.find(9)
     assert !field.is_for_all?
     assert_equal 'date', field.field_format
-    assert_tag :input, :attributes => {:name => 'custom_field_values[9]'}
+    assert_tag :input, :attributes => {:name => 'issue[custom_field_values][9]'}
     
     # System wide custom field
     assert CustomField.find(1).is_for_all?
-    assert_tag :select, :attributes => {:name => 'custom_field_values[1]'}
+    assert_tag :select, :attributes => {:name => 'issue[custom_field_values][1]'}
   end
 
   def test_bulk_edit
     @request.session[:user_id] = 2
     # update issues priority
-    post :bulk_edit, :ids => [1, 2], :priority_id => 7,
-                                     :assigned_to_id => '',
-                                     :custom_field_values => {'2' => ''},
-                                     :notes => 'Bulk editing'
+    post :bulk_edit, :ids => [1, 2], :notes => 'Bulk editing',
+                                     :issue => {:priority_id => 7,
+                                                :assigned_to_id => '',
+                                                :custom_field_values => {'2' => ''}}
+                                     
     assert_response 302
     # check that the issues were updated
     assert_equal [7, 7], Issue.find_all_by_id([1, 2]).collect {|i| i.priority.id}
@@ -930,10 +931,12 @@ class IssuesControllerTest < ActionController::TestCase
     post(:bulk_edit,
          {
            :ids => [1, 2],
-           :priority_id => 7,
-           :assigned_to_id => '',
-           :custom_field_values => {'2' => ''},
-           :notes => 'Bulk editing'
+           :notes => 'Bulk editing',
+           :issue => {
+             :priority_id => 7,
+             :assigned_to_id => '',
+             :custom_field_values => {'2' => ''}
+           }
          })
 
     assert_response 302
@@ -943,10 +946,11 @@ class IssuesControllerTest < ActionController::TestCase
   def test_bulk_edit_status
     @request.session[:user_id] = 2
     # update issues priority
-    post :bulk_edit, :ids => [1, 2], :priority_id => '',
-                                     :assigned_to_id => '',
-                                     :status_id => '5',
-                                     :notes => 'Bulk editing status'
+    post :bulk_edit, :ids => [1, 2], :notes => 'Bulk editing status',
+                                     :issue => {:priority_id => '',
+                                                :assigned_to_id => '',
+                                                :status_id => '5'}
+                                     
     assert_response 302
     issue = Issue.find(1)
     assert issue.closed?
@@ -955,10 +959,11 @@ class IssuesControllerTest < ActionController::TestCase
   def test_bulk_edit_custom_field
     @request.session[:user_id] = 2
     # update issues priority
-    post :bulk_edit, :ids => [1, 2], :priority_id => '',
-                                     :assigned_to_id => '',
-                                     :custom_field_values => {'2' => '777'},
-                                     :notes => 'Bulk editing custom field'
+    post :bulk_edit, :ids => [1, 2], :notes => 'Bulk editing custom field',
+                                     :issue => {:priority_id => '',
+                                                :assigned_to_id => '',
+                                                :custom_field_values => {'2' => '777'}}
+                                     
     assert_response 302
     
     issue = Issue.find(1)
@@ -973,7 +978,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_not_nil Issue.find(2).assigned_to
     @request.session[:user_id] = 2
     # unassign issues
-    post :bulk_edit, :ids => [1, 2], :notes => 'Bulk unassigning', :assigned_to_id => 'none'
+  post :bulk_edit, :ids => [1, 2], :notes => 'Bulk unassigning', :issue => {:assigned_to_id => 'none'}
     assert_response 302
     # check that the issues were updated
     assert_nil Issue.find(2).assigned_to
@@ -982,9 +987,7 @@ class IssuesControllerTest < ActionController::TestCase
   def test_post_bulk_edit_should_allow_fixed_version_to_be_set_to_a_subproject
     @request.session[:user_id] = 2
 
-    post :bulk_edit,
-         :ids => [1,2],
-         :fixed_version_id => 4
+    post :bulk_edit, :ids => [1,2], :issue => {:fixed_version_id => 4}
 
     assert_response :redirect
     issues = Issue.find([1,2])