Fixes Issue#save_issue_with_child_records so that time entry do not get saved if issue save fails.

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

app/models/issue.rb

@@ -484,34 +484,35 @@ class Issue < ActiveRecord::Base
   # Saves an issue, time_entry, attachments, and a journal from the parameters
   # Returns false if save fails
   def save_issue_with_child_records(params, existing_time_entry=nil)
-    if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, project)
+    Issue.transaction do
-      @time_entry = existing_time_entry || TimeEntry.new
+      if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, project)
-      @time_entry.project = project
+        @time_entry = existing_time_entry || TimeEntry.new
-      @time_entry.issue = self
+        @time_entry.project = project
-      @time_entry.user = User.current
+        @time_entry.issue = self
-      @time_entry.spent_on = Date.today
+        @time_entry.user = User.current
-      @time_entry.attributes = params[:time_entry]
+        @time_entry.spent_on = Date.today
-      self.time_entries << @time_entry
+        @time_entry.attributes = params[:time_entry]
-    end
+        self.time_entries << @time_entry
-
+      end
-    if valid?
+  
-      attachments = Attachment.attach_files(self, params[:attachments])
+      if valid?
-
+        attachments = Attachment.attach_files(self, params[:attachments])
-      attachments[:files].each {|a| @current_journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
+  
-      # TODO: Rename hook
+        attachments[:files].each {|a| @current_journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
-      Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
+        # TODO: Rename hook
-      begin
+        Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
-        if save
+        begin
-          # TODO: Rename hook
+          if save
-          Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
+            # TODO: Rename hook
-          return true
+            Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
-        else
+          else
-          return false
+            raise ActiveRecord::Rollback
+          end
+        rescue ActiveRecord::StaleObjectError
+          attachments[:files].each(&:destroy)
+          errors.add_to_base l(:notice_locking_conflict)
+          raise ActiveRecord::Rollback
         end
-      rescue ActiveRecord::StaleObjectError
-        attachments[:files].each(&:destroy)
-        errors.add_to_base l(:notice_locking_conflict)
-        return false
       end
     end
   end

test/functional/issues_controller_test.rb

@@ -827,7 +827,7 @@ class IssuesControllerTest < ActionController::TestCase
       put :update,
            :id => 1,
            :notes => '2.5 hours added',
-           :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first }
+           :time_entry => { :hours => '2.5', :comments => 'test_put_update_with_note_and_spent_time', :activity_id => TimeEntryActivity.first }
     end
     assert_redirected_to :action => 'show', :id => '1'
     
@@ -837,7 +837,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_equal '2.5 hours added', j.notes
     assert_equal 0, j.details.size
     
-    t = issue.time_entries.find(:first, :order => 'id DESC')
+    t = issue.time_entries.find_by_comments('test_put_update_with_note_and_spent_time')
     assert_not_nil t
     assert_equal 2.5, t.hours
     assert_equal spent_hours_before + 2.5, issue.spent_hours
@@ -985,15 +985,18 @@ class IssuesControllerTest < ActionController::TestCase
     @request.session[:user_id] = 2
 
     assert_no_difference 'Journal.count' do
-      assert_no_difference 'Attachment.count' do
+      assert_no_difference 'TimeEntry.count' do
-        put :update,
+        assert_no_difference 'Attachment.count' do
-              :id => issue.id,
+          put :update,
-              :issue => {
+                :id => issue.id,
-                :fixed_version_id => 4,
+                :issue => {
-                :lock_version => (issue.lock_version - 1)
+                  :fixed_version_id => 4,
-              },
+                  :lock_version => (issue.lock_version - 1)
-              :notes => '',
+                },
-              :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain')}}
+                :notes => '',
+                :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain')}},
+                :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first }
+        end
       end
     end