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

4 files changed, 38 insertions, 12 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index b97937a48..1a2f96d56 100644
--- a/app/controllers/issues_controller.rb
+++ b/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
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 012661cad..e2c85e1db 100644
--- a/app/models/issue.rb
+++ b/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})
-      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
+      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
diff --git a/test/fixtures/issues.yml b/test/fixtures/issues.yml
index 6ec671e58..3adda3121 100644
--- a/test/fixtures/issues.yml
+++ b/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
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 757f36c19..37561ec69 100644
--- a/test/functional/issues_controller_test.rb
+++ b/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]