Merge pull request #617 from opf/fix/concurrent_wp_edits

Fixed non-functional optimistic locking for

app/controllers/work_packages_controller.rb

@@ -179,6 +179,21 @@ class WorkPackagesController < ApplicationController
     else
       edit
     end
+  rescue ActiveRecord::StaleObjectError
+    error_message = l(:notice_locking_conflict)
+    render_attachment_warning_if_needed(work_package)
+
+    journals_since = work_package.journals.after(work_package.lock_version)
+    if journals_since.any?
+      changes = journals_since.map { |j| "#{j.user.name} (#{j.created_at.to_s(:short)})" }
+      error_message << " " << l(:notice_locking_conflict_additional_information, :users => changes.join(', '))
+    end
+
+    error_message << " " << l(:notice_locking_conflict_reload_page)
+
+    work_package.errors.add :base, error_message
+
+    edit
   end
 
   def index

app/models/journal.rb

@@ -131,7 +131,9 @@ class Journal < ActiveRecord::Base
   end
 
   def touch_journable
-    journable.touch unless journable.nil?
+    if journable && !journable.changed?
+      journable.touch
+    end
   end
 
   def get_changes

app/models/permitted_params.rb

@@ -144,7 +144,8 @@ class PermittedParams < Struct.new(:params, :user)
                                          :planning_element_status_comment,
                                          :planning_element_status_id,
                                          :parent_id,
-                                         :responsible_id)
+                                         :responsible_id,
+                                         :lock_version)
   end
 
   def board_move
@@ -198,6 +199,7 @@ class PermittedParams < Struct.new(:params, :user)
                                                      :category_id,
                                                      :status_id,
                                                      :notes,
+                                                     :lock_version,
                                                      { attachments: [:file, :description] },
                                                      Proc.new do |args|
                                                        # avoid costly allowed_to? if the param is not there at all

doc/CHANGELOG.md

@@ -29,9 +29,10 @@ See doc/COPYRIGHT.rdoc for more details.
 
 # Changelog
 
-* `#2566` [Timelines] Searching when selecting columns for a timeline configuration does not work
-* `#2631` [Timelines] Work package cannot be created out of timeline.
-* `#2686` [Work package tracking] Work package summary not displayed correctly
+* `#2566` Fix: [Timelines] Searching when selecting columns for a timeline configuration does not work
+* `#2631` Fix: [Timelines] Work package cannot be created out of timeline.
+* `#2686` Fix: [Work package tracking] Work package summary not displayed correctly
+* `#2687` Fix: [Work Package Tracking] No error for parallel editing
 
 ## 3.0.0pre27
 

features/work_packages/update.feature

@@ -108,6 +108,16 @@ Feature: Updating work packages
       | Description    | Desc2       |
     And the work package "pe2" should be shown as the parent
 
+  Scenario: Concurrent updates to work packages
+    When I go to the edit page of the work package called "pe1"
+    And I fill in the following:
+      | Start date     | 03-04-2013   |
+    And the work_package "pe1" is updated with the following:
+      | Start date | 04-04-2013 |
+    And I submit the form by the "Submit" button
+    Then I should see "Information has been updated by at least one other user in the meantime."
+    And I should see "The update(s) came from"
+
   Scenario: Adding a note
     When I go to the edit page of the work package called "pe1"
      And I fill in "Notes" with "Note message"

spec/models/permitted_params_spec.rb

@@ -427,6 +427,14 @@ describe PermittedParams do
       PermittedParams.new(params, user).update_work_package.should == hash
     end
 
+    it "should permit lock_version" do
+      hash = { "lock_version" => "1" }
+
+      params = ActionController::Parameters.new(:work_package => hash)
+
+      PermittedParams.new(params, user).update_work_package.should == hash
+    end
+
     it "should permit status_id" do
       hash = { "status_id" => "1" }