When a specific TimeEntryActivity are change, associated TimeEntries will be

re-assigned to the correct record.

* Renamed build to create since the methods now create objects.
* Removed call to Project#save that isn't needed since objects are saved directly now.
* Wrapped the activity creation and updates in a SQL transaction so TimeEntries
  will remain in a consistent state if there is an error.
* When a Project's TimeEntryActivities are destroyed, they are now
  reassigned to the parent TimeEntryActivity.

  #4077

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

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index e36d9bfdf47630b70defca2aa45cc6f7691dde04..32326d45dfe52d9d49b382708f2d23ec570d1d72 100644 (file)
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -234,10 +234,11 @@ class ProjectsController < ApplicationController
 
   def save_activities
     if request.post? && params[:enumerations]
-      params[:enumerations].each do |id, activity|
-        @project.update_or_build_time_entry_activity(id, activity)
+      Project.transaction do
+        params[:enumerations].each do |id, activity|
+          @project.update_or_create_time_entry_activity(id, activity)
+        end
       end
-      @project.save
     end
     
     redirect_to :controller => 'projects', :action => 'settings', :tab => 'activities', :id => @project
@@ -245,7 +246,7 @@ class ProjectsController < ApplicationController
 
   def reset_activities
     @project.time_entry_activities.each do |time_entry_activity|
-      time_entry_activity.destroy
+      time_entry_activity.destroy(time_entry_activity.parent)
     end
     redirect_to :controller => 'projects', :action => 'settings', :tab => 'activities', :id => @project
   end

diff --git a/app/models/project.rb b/app/models/project.rb
index db8c6711e5582f1e12097d6343740aa31476097f..668238fc880ffde3d1e9a03881e38627effcf3a7 100644 (file)
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -170,19 +170,24 @@ class Project < ActiveRecord::Base
     end
   end
 
-  # Will build a new Project specific Activity or update an existing one
-  def update_or_build_time_entry_activity(id, activity_hash)
+  # Will create a new Project specific Activity or update an existing one
+  #
+  # This will raise a ActiveRecord::Rollback if the TimeEntryActivity
+  # does not successfully save.
+  def update_or_create_time_entry_activity(id, activity_hash)
     if activity_hash.respond_to?(:has_key?) && activity_hash.has_key?('parent_id')
-      self.build_time_entry_activity_if_needed(activity_hash)
+      self.create_time_entry_activity_if_needed(activity_hash)
     else
       activity = project.time_entry_activities.find_by_id(id.to_i)
       activity.update_attributes(activity_hash) if activity
     end
   end
   
-  # Builds new activity
-  def build_time_entry_activity_if_needed(activity)
-    # Only new override activities are built
+  # Create a new TimeEntryActivity if it overrides a system TimeEntryActivity
+  #
+  # This will raise a ActiveRecord::Rollback if the TimeEntryActivity
+  # does not successfully save.
+  def create_time_entry_activity_if_needed(activity)
     if activity['parent_id']
     
       parent_activity = TimeEntryActivity.find(activity['parent_id'])
@@ -190,7 +195,13 @@ class Project < ActiveRecord::Base
       activity['position'] = parent_activity.position
 
       if Enumeration.overridding_change?(activity, parent_activity)
-        self.time_entry_activities.build(activity)
+        project_activity = self.time_entry_activities.create(activity)
+
+        if project_activity.new_record?
+          raise ActiveRecord::Rollback, "Overridding TimeEntryActivity was not successfully saved"
+        else
+          self.time_entries.update_all("activity_id = #{project_activity.id}", ["activity_id = ?", parent_activity.id])
+        end
       end
     end
   end

diff --git a/test/fixtures/time_entries.yml b/test/fixtures/time_entries.yml
index a44c0938f0fda881aa15a5695159b7066d7ef02b..f56b05aa57c27fb5e0926a5eef4da8266acaeccf 100644 (file)
--- a/test/fixtures/time_entries.yml
+++ b/test/fixtures/time_entries.yml
@@ -55,4 +55,4 @@ time_entries_004:
   hours: 7.65
   user_id: 1
   tyear: 2007
-  
\ No newline at end of file
+  

diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb
index 9736ce056fe8cec9739b8caa06578ad718da27dd..dcbcb77eb39c50475d0ca69e560328d1003871f3 100644 (file)
--- a/test/functional/projects_controller_test.rb
+++ b/test/functional/projects_controller_test.rb
@@ -24,7 +24,7 @@ class ProjectsController; def rescue_action(e) raise e end; end
 class ProjectsControllerTest < ActionController::TestCase
   fixtures :projects, :versions, :users, :roles, :members, :member_roles, :issues, :journals, :journal_details,
            :trackers, :projects_trackers, :issue_statuses, :enabled_modules, :enumerations, :boards, :messages,
-           :attachments, :custom_fields, :custom_values
+           :attachments, :custom_fields, :custom_values, :time_entries
 
   def setup
     @controller = ProjectsController.new
@@ -586,6 +586,27 @@ class ProjectsControllerTest < ActionController::TestCase
     assert_nil TimeEntryActivity.find_by_id(project_activity_two.id)
   end
   
+  def test_reset_activities_should_reassign_time_entries_back_to_the_system_activity
+    @request.session[:user_id] = 2 # manager
+    project_activity = TimeEntryActivity.new({
+                                               :name => 'Project Specific Design',
+                                               :parent => TimeEntryActivity.find(9),
+                                               :project => Project.find(1),
+                                               :active => true
+                                             })
+    assert project_activity.save
+    assert TimeEntry.update_all("activity_id = '#{project_activity.id}'", ["project_id = ? AND activity_id = ?", 1, 9])
+    assert 3, TimeEntry.find_all_by_activity_id_and_project_id(project_activity.id, 1).size
+    
+    delete :reset_activities, :id => 1
+    assert_response :redirect
+    assert_redirected_to 'projects/ecookbook/settings/activities'
+
+    assert_nil TimeEntryActivity.find_by_id(project_activity.id)
+    assert_equal 0, TimeEntry.find_all_by_activity_id_and_project_id(project_activity.id, 1).size, "TimeEntries still assigned to project specific activity"
+    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_routing
     assert_routing({:method => :post, :path => 'projects/64/activities/save'},
                    :controller => 'projects', :action => 'save_activities', :id => '64')
@@ -683,6 +704,46 @@ class ProjectsControllerTest < ActionController::TestCase
     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)