diff --git a/app/models/issue.rb b/app/models/issue.rb index e2c85e1d..263cae13 100644 --- a/app/models/issue.rb +++ b/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) - @time_entry = existing_time_entry || TimeEntry.new - @time_entry.project = project - @time_entry.issue = self - @time_entry.user = User.current - @time_entry.spent_on = Date.today - @time_entry.attributes = params[:time_entry] - self.time_entries << @time_entry - end - - 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 - Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal}) - begin - if save - # TODO: Rename hook - Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal}) - return true - else - return false + Issue.transaction do + if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, project) + @time_entry = existing_time_entry || TimeEntry.new + @time_entry.project = project + @time_entry.issue = self + @time_entry.user = User.current + @time_entry.spent_on = Date.today + @time_entry.attributes = params[:time_entry] + self.time_entries << @time_entry + end + + 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 + Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal}) + begin + if save + # TODO: Rename hook + Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal}) + else + 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 diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 37561ec6..9c7d4f1c 100644 --- a/test/functional/issues_controller_test.rb +++ b/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 - put :update, - :id => issue.id, - :issue => { - :fixed_version_id => 4, - :lock_version => (issue.lock_version - 1) - }, - :notes => '', - :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain')}} + assert_no_difference 'TimeEntry.count' do + assert_no_difference 'Attachment.count' do + put :update, + :id => issue.id, + :issue => { + :fixed_version_id => 4, + :lock_version => (issue.lock_version - 1) + }, + :notes => '', + :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain')}}, + :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first } + end end end