Fixed: issue optimistic locking broken by r3308 (#5280).

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

app/controllers/issues_controller.rb

@@ -212,12 +212,6 @@ class IssuesController < ApplicationController
         format.xml  { render :xml => @issue.errors, :status => :unprocessable_entity }
       end
     end
-    
-  rescue ActiveRecord::StaleObjectError
-    # Optimistic locking exception
-    flash.now[:error] = l(:notice_locking_conflict)
-    # Remove the previously added attachments if issue was not updated
-    attachments[:files].each(&:destroy) if attachments[:files]
   end
 
   def reply

app/models/issue.rb

@@ -212,6 +212,7 @@ class Issue < ActiveRecord::Base
     done_ratio
     estimated_hours
     custom_field_values
+    lock_version
   ) unless const_defined?(:SAFE_ATTRIBUTES)
   
   # Safely sets attributes
@@ -481,6 +482,7 @@ class Issue < ActiveRecord::Base
   end
 
   # 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
@@ -498,14 +500,20 @@ class Issue < ActiveRecord::Base
       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
+        end
+      rescue ActiveRecord::StaleObjectError
+        attachments[:files].each(&:destroy)
+        errors.add_to_base l(:notice_locking_conflict)
+        return false
       end
     end
-    # failure, returns false
-
   end
 
   # Unassigns issues from +version+ if it's no longer shared with issue's project

test/fixtures/issues.yml

@@ -37,6 +37,7 @@ issues_002:
   root_id: 2
   lft: 1
   rgt: 2
+  lock_version: 3
 issues_003: 
   created_on: 2006-07-19 21:07:27 +02:00
   project_id: 1

test/functional/issues_controller_test.rb

@@ -980,6 +980,29 @@ class IssuesControllerTest < ActionController::TestCase
     assert_redirected_to :controller => 'issues', :action => 'show', :id => issue.id
   end
   
+  def test_put_update_stale_issue
+    issue = Issue.find(2)
+    @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')}}
+      end
+    end
+    
+    assert_response :success
+    assert_template 'edit'
+    assert_tag :tag => 'div', :attributes => { :id => 'errorExplanation' },
+                              :content => /Data has been updated by another user/
+  end
+  
   def test_get_bulk_edit
     @request.session[:user_id] = 2
     get :bulk_edit, :ids => [1, 2]