From e24d6cc2237d4898e7ad2520e34328f8dc0d5935 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Tue, 23 Feb 2010 21:26:29 +0000 Subject: [PATCH] Bulk edit refactoring. git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@3478 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/issues_controller.rb | 29 ++++---------- app/helpers/custom_fields_helper.rb | 6 +-- app/models/issue.rb | 26 ++++--------- app/views/issues/bulk_edit.rhtml | 20 +++++----- test/functional/issues_controller_test.rb | 47 ++++++++++++----------- 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 @@

- <%= select_tag('tracker_id', "" + options_from_collection_for_select(@project.trackers, :id, :name)) %> + <%= select_tag('issue[tracker_id]', "" + options_from_collection_for_select(@project.trackers, :id, :name)) %>

<% if @available_statuses.any? %>

- <%= select_tag('status_id', "" + options_from_collection_for_select(@available_statuses, :id, :name)) %> + <%= select_tag('issue[status_id]', "" + options_from_collection_for_select(@available_statuses, :id, :name)) %>

<% end %>

- <%= select_tag('priority_id', "" + options_from_collection_for_select(IssuePriority.all, :id, :name)) %> + <%= select_tag('issue[priority_id]', "" + options_from_collection_for_select(IssuePriority.all, :id, :name)) %>

- <%= 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)) %>

- <%= 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)) %>

- <%= 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)) %>

<% @custom_fields.each do |custom_field| %> -

<%= custom_field_tag_for_bulk_edit(custom_field) %>

+

<%= custom_field_tag_for_bulk_edit('issue', custom_field) %>

<% end %> <%= call_hook(:view_issues_bulk_edit_details_bottom, { :issues => @issues }) %> @@ -52,16 +52,16 @@

- <%= text_field_tag 'start_date', '', :size => 10 %><%= calendar_for('start_date') %> + <%= text_field_tag 'issue[start_date]', '', :size => 10 %><%= calendar_for('issue_start_date') %>

- <%= text_field_tag 'due_date', '', :size => 10 %><%= calendar_for('due_date') %> + <%= text_field_tag 'issue[due_date]', '', :size => 10 %><%= calendar_for('issue_due_date') %>

<% if Issue.use_field_for_done_ratio? %>

- <%= 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] }) %>

<% end %>
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])