From: Eric Davis Date: Thu, 2 Sep 2010 17:39:56 +0000 (+0000) Subject: Refactor: move method, ProjectsController#save_activities to ProjectEnumerationsContr... X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=83b4343d2d403a21de4614016e165091e4c5914f;p=redminele%2Fredmine.git Refactor: move method, ProjectsController#save_activities to ProjectEnumerationsController#save git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@4053 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- diff --git a/app/controllers/project_enumerations_controller.rb b/app/controllers/project_enumerations_controller.rb new file mode 100644 index 00000000..c4b48e47 --- /dev/null +++ b/app/controllers/project_enumerations_controller.rb @@ -0,0 +1,18 @@ +class ProjectEnumerationsController < ApplicationController + before_filter :find_project + before_filter :authorize + + def save + if request.post? && params[:enumerations] + Project.transaction do + params[:enumerations].each do |id, activity| + @project.update_or_create_time_entry_activity(id, activity) + end + end + flash[:notice] = l(:notice_successful_update) + end + + redirect_to :controller => 'projects', :action => 'settings', :tab => 'activities', :id => @project + end + +end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 61c7a7c7..0670cb1e 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -238,19 +238,6 @@ class ProjectsController < ApplicationController @project = nil end - def save_activities - if request.post? && params[:enumerations] - Project.transaction do - params[:enumerations].each do |id, activity| - @project.update_or_create_time_entry_activity(id, activity) - end - end - flash[:notice] = l(:notice_successful_update) - end - - redirect_to :controller => 'projects', :action => 'settings', :tab => 'activities', :id => @project - end - def reset_activities @project.time_entry_activities.each do |time_entry_activity| time_entry_activity.destroy(time_entry_activity.parent) diff --git a/app/views/projects/settings/_activities.rhtml b/app/views/projects/settings/_activities.rhtml index d82bd96f..e39bf9b9 100644 --- a/app/views/projects/settings/_activities.rhtml +++ b/app/views/projects/settings/_activities.rhtml @@ -1,4 +1,4 @@ -<% form_tag({:controller => 'projects', :action => 'save_activities', :id => @project}, :class => "tabular") do %> +<% form_tag({:controller => 'project_enumerations', :action => 'save', :id => @project}, :class => "tabular") do %> diff --git a/config/routes.rb b/config/routes.rb index 9c8a2f6a..12411599 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -200,7 +200,7 @@ ActionController::Routing::Routes.draw do |map| project_actions.connect 'projects.:format', :action => 'add', :format => /xml/ project_actions.connect 'projects/:id/:action', :action => /edit|destroy|archive|unarchive/ project_actions.connect 'projects/:id/files/new', :controller => 'files', :action => 'new' - project_actions.connect 'projects/:id/activities/save', :action => 'save_activities' + project_actions.connect 'projects/:id/activities/save', :controller => 'project_enumerations', :action => 'save' end projects.with_options :conditions => {:method => :put} do |project_actions| diff --git a/lib/redmine.rb b/lib/redmine.rb index 745c6d75..fdd61225 100644 --- a/lib/redmine.rb +++ b/lib/redmine.rb @@ -87,7 +87,7 @@ Redmine::AccessControl.map do |map| map.permission :view_time_entries, :timelog => [:details, :report] map.permission :edit_time_entries, {:timelog => [:edit, :destroy]}, :require => :member map.permission :edit_own_time_entries, {:timelog => [:edit, :destroy]}, :require => :loggedin - map.permission :manage_project_activities, {:projects => [:save_activities, :reset_activities]}, :require => :member + map.permission :manage_project_activities, {:projects => [:reset_activities], :project_enumerations => [:save]}, :require => :member end map.project_module :news do |map| diff --git a/test/functional/project_enumerations_controller_test.rb b/test/functional/project_enumerations_controller_test.rb new file mode 100644 index 00000000..23da4237 --- /dev/null +++ b/test/functional/project_enumerations_controller_test.rb @@ -0,0 +1,142 @@ +require File.dirname(__FILE__) + '/../test_helper' + +class ProjectEnumerationsControllerTest < ActionController::TestCase + fixtures :all + + def setup + @request.session[:user_id] = nil + Setting.default_language = 'en' + end + + def test_save_to_override_system_activities + @request.session[:user_id] = 2 # manager + billable_field = TimeEntryActivityCustomField.find_by_name("Billable") + + post :save, :id => 1, :enumerations => { + "9"=> {"parent_id"=>"9", "custom_field_values"=>{"7" => "1"}, "active"=>"0"}, # Design, De-activate + "10"=> {"parent_id"=>"10", "custom_field_values"=>{"7"=>"0"}, "active"=>"1"}, # Development, Change custom value + "14"=>{"parent_id"=>"14", "custom_field_values"=>{"7"=>"1"}, "active"=>"1"}, # Inactive Activity, Activate with custom value + "11"=>{"parent_id"=>"11", "custom_field_values"=>{"7"=>"1"}, "active"=>"1"} # QA, no changes + } + + assert_response :redirect + assert_redirected_to 'projects/ecookbook/settings/activities' + + # Created project specific activities... + project = Project.find('ecookbook') + + # ... Design + design = project.time_entry_activities.find_by_name("Design") + assert design, "Project activity not found" + + assert_equal 9, design.parent_id # Relate to the system activity + assert_not_equal design.parent.id, design.id # Different records + assert_equal design.parent.name, design.name # Same name + assert !design.active? + + # ... Development + development = project.time_entry_activities.find_by_name("Development") + assert development, "Project activity not found" + + assert_equal 10, development.parent_id # Relate to the system activity + assert_not_equal development.parent.id, development.id # Different records + assert_equal development.parent.name, development.name # Same name + assert development.active? + assert_equal "0", development.custom_value_for(billable_field).value + + # ... Inactive Activity + previously_inactive = project.time_entry_activities.find_by_name("Inactive Activity") + assert previously_inactive, "Project activity not found" + + assert_equal 14, previously_inactive.parent_id # Relate to the system activity + assert_not_equal previously_inactive.parent.id, previously_inactive.id # Different records + assert_equal previously_inactive.parent.name, previously_inactive.name # Same name + assert previously_inactive.active? + assert_equal "1", previously_inactive.custom_value_for(billable_field).value + + # ... QA + assert_equal nil, project.time_entry_activities.find_by_name("QA"), "Custom QA activity created when it wasn't modified" + end + + def test_save_will_update_project_specific_activities + @request.session[:user_id] = 2 # manager + + project_activity = TimeEntryActivity.new({ + :name => 'Project Specific', + :parent => TimeEntryActivity.find(:first), + :project => Project.find(1), + :active => true + }) + assert project_activity.save + project_activity_two = TimeEntryActivity.new({ + :name => 'Project Specific Two', + :parent => TimeEntryActivity.find(:last), + :project => Project.find(1), + :active => true + }) + assert project_activity_two.save + + + post :save, :id => 1, :enumerations => { + project_activity.id => {"custom_field_values"=>{"7" => "1"}, "active"=>"0"}, # De-activate + project_activity_two.id => {"custom_field_values"=>{"7" => "1"}, "active"=>"0"} # De-activate + } + + assert_response :redirect + assert_redirected_to 'projects/ecookbook/settings/activities' + + # Created project specific activities... + project = Project.find('ecookbook') + assert_equal 2, project.time_entry_activities.count + + activity_one = project.time_entry_activities.find_by_name(project_activity.name) + assert activity_one, "Project activity not found" + assert_equal project_activity.id, activity_one.id + assert !activity_one.active? + + activity_two = project.time_entry_activities.find_by_name(project_activity_two.name) + assert activity_two, "Project activity not found" + assert_equal project_activity_two.id, activity_two.id + assert !activity_two.active? + end + + def test_save_when_creating_new_activities_will_convert_existing_data + assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size + + @request.session[:user_id] = 2 # manager + post :save, :id => 1, :enumerations => { + "9"=> {"parent_id"=>"9", "custom_field_values"=>{"7" => "1"}, "active"=>"0"} # Design, De-activate + } + assert_response :redirect + + # No more TimeEntries using the system activity + assert_equal 0, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size, "Time Entries still assigned to system activities" + # All TimeEntries using project activity + project_specific_activity = TimeEntryActivity.find_by_parent_id_and_project_id(9, 1) + assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(project_specific_activity.id, 1).size, "No Time Entries assigned to the project activity" + end + + def test_save_when_creating_new_activities_will_not_convert_existing_data_if_an_exception_is_raised + # TODO: Need to cause an exception on create but these tests + # aren't setup for mocking. Just create a record now so the + # second one is a dupicate + parent = TimeEntryActivity.find(9) + TimeEntryActivity.create!({:name => parent.name, :project_id => 1, :position => parent.position, :active => true}) + TimeEntry.create!({:project_id => 1, :hours => 1.0, :user => User.find(1), :issue_id => 3, :activity_id => 10, :spent_on => '2009-01-01'}) + + assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size + assert_equal 1, TimeEntry.find_all_by_activity_id_and_project_id(10, 1).size + + @request.session[:user_id] = 2 # manager + post :save, :id => 1, :enumerations => { + "9"=> {"parent_id"=>"9", "custom_field_values"=>{"7" => "1"}, "active"=>"0"}, # Design + "10"=> {"parent_id"=>"10", "custom_field_values"=>{"7"=>"0"}, "active"=>"1"} # Development, Change custom value + } + assert_response :redirect + + # TimeEntries shouldn't have been reassigned on the failed record + assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size, "Time Entries are not assigned to system activities" + # TimeEntries shouldn't have been reassigned on the saved record either + assert_equal 1, TimeEntry.find_all_by_activity_id_and_project_id(10, 1).size, "Time Entries are not assigned to system activities" + end +end diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index 9d1582af..152d0837 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -427,138 +427,6 @@ class ProjectsControllerTest < ActionController::TestCase assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size, "TimeEntries still assigned to project specific activity" end - def test_save_activities_to_override_system_activities - @request.session[:user_id] = 2 # manager - billable_field = TimeEntryActivityCustomField.find_by_name("Billable") - - post :save_activities, :id => 1, :enumerations => { - "9"=> {"parent_id"=>"9", "custom_field_values"=>{"7" => "1"}, "active"=>"0"}, # Design, De-activate - "10"=> {"parent_id"=>"10", "custom_field_values"=>{"7"=>"0"}, "active"=>"1"}, # Development, Change custom value - "14"=>{"parent_id"=>"14", "custom_field_values"=>{"7"=>"1"}, "active"=>"1"}, # Inactive Activity, Activate with custom value - "11"=>{"parent_id"=>"11", "custom_field_values"=>{"7"=>"1"}, "active"=>"1"} # QA, no changes - } - - assert_response :redirect - assert_redirected_to 'projects/ecookbook/settings/activities' - - # Created project specific activities... - project = Project.find('ecookbook') - - # ... Design - design = project.time_entry_activities.find_by_name("Design") - assert design, "Project activity not found" - - assert_equal 9, design.parent_id # Relate to the system activity - assert_not_equal design.parent.id, design.id # Different records - assert_equal design.parent.name, design.name # Same name - assert !design.active? - - # ... Development - development = project.time_entry_activities.find_by_name("Development") - assert development, "Project activity not found" - - assert_equal 10, development.parent_id # Relate to the system activity - assert_not_equal development.parent.id, development.id # Different records - assert_equal development.parent.name, development.name # Same name - assert development.active? - assert_equal "0", development.custom_value_for(billable_field).value - - # ... Inactive Activity - previously_inactive = project.time_entry_activities.find_by_name("Inactive Activity") - assert previously_inactive, "Project activity not found" - - assert_equal 14, previously_inactive.parent_id # Relate to the system activity - assert_not_equal previously_inactive.parent.id, previously_inactive.id # Different records - assert_equal previously_inactive.parent.name, previously_inactive.name # Same name - assert previously_inactive.active? - assert_equal "1", previously_inactive.custom_value_for(billable_field).value - - # ... QA - assert_equal nil, project.time_entry_activities.find_by_name("QA"), "Custom QA activity created when it wasn't modified" - end - - def test_save_activities_will_update_project_specific_activities - @request.session[:user_id] = 2 # manager - - project_activity = TimeEntryActivity.new({ - :name => 'Project Specific', - :parent => TimeEntryActivity.find(:first), - :project => Project.find(1), - :active => true - }) - assert project_activity.save - project_activity_two = TimeEntryActivity.new({ - :name => 'Project Specific Two', - :parent => TimeEntryActivity.find(:last), - :project => Project.find(1), - :active => true - }) - assert project_activity_two.save - - - post :save_activities, :id => 1, :enumerations => { - project_activity.id => {"custom_field_values"=>{"7" => "1"}, "active"=>"0"}, # De-activate - project_activity_two.id => {"custom_field_values"=>{"7" => "1"}, "active"=>"0"} # De-activate - } - - assert_response :redirect - assert_redirected_to 'projects/ecookbook/settings/activities' - - # Created project specific activities... - project = Project.find('ecookbook') - assert_equal 2, project.time_entry_activities.count - - activity_one = project.time_entry_activities.find_by_name(project_activity.name) - assert activity_one, "Project activity not found" - assert_equal project_activity.id, activity_one.id - assert !activity_one.active? - - activity_two = project.time_entry_activities.find_by_name(project_activity_two.name) - assert activity_two, "Project activity not found" - assert_equal project_activity_two.id, activity_two.id - assert !activity_two.active? - end - - def test_save_activities_when_creating_new_activities_will_convert_existing_data - assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size - - @request.session[:user_id] = 2 # manager - post :save_activities, :id => 1, :enumerations => { - "9"=> {"parent_id"=>"9", "custom_field_values"=>{"7" => "1"}, "active"=>"0"} # Design, De-activate - } - assert_response :redirect - - # No more TimeEntries using the system activity - assert_equal 0, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size, "Time Entries still assigned to system activities" - # All TimeEntries using project activity - project_specific_activity = TimeEntryActivity.find_by_parent_id_and_project_id(9, 1) - assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(project_specific_activity.id, 1).size, "No Time Entries assigned to the project activity" - end - - def test_save_activities_when_creating_new_activities_will_not_convert_existing_data_if_an_exception_is_raised - # TODO: Need to cause an exception on create but these tests - # aren't setup for mocking. Just create a record now so the - # second one is a dupicate - parent = TimeEntryActivity.find(9) - TimeEntryActivity.create!({:name => parent.name, :project_id => 1, :position => parent.position, :active => true}) - TimeEntry.create!({:project_id => 1, :hours => 1.0, :user => User.find(1), :issue_id => 3, :activity_id => 10, :spent_on => '2009-01-01'}) - - assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size - assert_equal 1, TimeEntry.find_all_by_activity_id_and_project_id(10, 1).size - - @request.session[:user_id] = 2 # manager - post :save_activities, :id => 1, :enumerations => { - "9"=> {"parent_id"=>"9", "custom_field_values"=>{"7" => "1"}, "active"=>"0"}, # Design - "10"=> {"parent_id"=>"10", "custom_field_values"=>{"7"=>"0"}, "active"=>"1"} # Development, Change custom value - } - assert_response :redirect - - # TimeEntries shouldn't have been reassigned on the failed record - assert_equal 3, TimeEntry.find_all_by_activity_id_and_project_id(9, 1).size, "Time Entries are not assigned to system activities" - # TimeEntries shouldn't have been reassigned on the saved record either - assert_equal 1, TimeEntry.find_all_by_activity_id_and_project_id(10, 1).size, "Time Entries are not assigned to system activities" - end - # A hook that is manually registered later class ProjectBasedTemplate < Redmine::Hook::ViewListener def view_layouts_base_html_head(context) diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index 66836c4f..74a0f7a7 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -185,7 +185,7 @@ class RoutingTest < ActionController::IntegrationTest should_route :post, "/projects/33/files/new", :controller => 'files', :action => 'new', :id => '33' should_route :post, "/projects/64/archive", :controller => 'projects', :action => 'archive', :id => '64' should_route :post, "/projects/64/unarchive", :controller => 'projects', :action => 'unarchive', :id => '64' - should_route :post, "/projects/64/activities/save", :controller => 'projects', :action => 'save_activities', :id => '64' + should_route :post, "/projects/64/activities/save", :controller => 'project_enumerations', :action => 'save', :id => '64' should_route :put, "/projects/1.xml", :controller => 'projects', :action => 'edit', :id => '1', :format => 'xml'