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

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

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)

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 %>
 
 <table class="list">
   <thead><tr>

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|

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|

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

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)

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'