From 8b58617753cdef1f72220428b8ddda45a6f31d43 Mon Sep 17 00:00:00 2001
From: Eric Davis <edavis@littlestreamsoftware.com>
Date: Thu, 29 Jan 2009 09:57:47 -0800
Subject: [PATCH] Handle the StaleObjectError when two users edit the same issue. (#2621)

If a Issue fails saving due to a StaleObjectError, the instance should reload.
Otherwise the lock_version will never update and a user can't save their data
using the edit form.

* Added a list of the changes so the conflicting user can see what was changed
* Added StaleObject class to make it easy to find what attributes changed and
  print a friendly error message
* Allow a user to re-associate their file uploads during a conflict, so they
  don't have to reupload everything.

This reverts r2875 and #3846
---
 app/controllers/application_controller.rb       |   28 +++++--
 app/controllers/issues_controller.rb            |   18 +++-
 app/views/attachments/_upload_conflict.html.erb |   10 ++
 app/views/issues/_edit.rhtml                    |    4 +
 config/locales/en.yml                           |    2 +
 lib/stale_object.rb                             |   50 +++++++++++
 test/unit/lib/stale_object_test.rb              |  106 +++++++++++++++++++++++
 7 files changed, 208 insertions(+), 10 deletions(-)
 create mode 100644 app/views/attachments/_upload_conflict.html.erb
 create mode 100644 lib/stale_object.rb
 create mode 100644 test/unit/lib/stale_object_test.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index a794b3b..ca489da 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -218,13 +218,27 @@ class ApplicationController < ActionController::Base
     unsaved = []
     if attachments && attachments.is_a?(Hash)
       attachments.each_value do |attachment|
-        file = attachment['file']
-        next unless file && file.size > 0
-        a = Attachment.create(:container => obj, 
-                              :file => file,
-                              :description => attachment['description'].to_s.strip,
-                              :author => User.current)
-        a.new_record? ? (unsaved << a) : (attached << a)
+        # Attachment was already saved, but not associated with the
+        # journal (e.g. StaleObjectError)
+        if attachment['id'] && Attachment.find_by_id(attachment['id'])
+          saved_attachment = Attachment.find_by_id(attachment['id'])
+          if saved_attachment.container == obj
+            attached << saved_attachment
+          else
+            unsaved << saved_attachment
+          end
+        # New attachment
+        elsif file = attachment['file'] && attachment['file'].size > 0
+          file = attachment['file']
+          a = Attachment.create(:container => obj, 
+                                :file => file,
+                                :description => attachment['description'].to_s.strip,
+                                :author => User.current)
+          a.new_record? ? (unsaved << a) : (attached << a)
+        else
+          next
+        end
+
       end
       if unsaved.any?
         flash[:warning] = l(:warning_attachments_not_saved, unsaved.size)
diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index c997da9..a3a69bb 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -206,9 +206,21 @@ class IssuesController < ApplicationController
     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.each(&:destroy)
+    @issue.reload # Get new lock_version
+    @stale_object = StaleObject.new(@issue)
+    @attachments_not_on_journal = attachments
+
+    # Pull in updated attribute changes
+    # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
+    if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
+      attrs = params[:issue].dup
+      attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
+      attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
+      attrs.delete(:lock_version)
+      @issue.attributes = attrs
+    end
+
+    flash.now[:error] = l(:notice_locking_conflict) + @stale_object.difference_messages(@issue)
   end
 
   def reply
diff --git a/app/views/attachments/_upload_conflict.html.erb b/app/views/attachments/_upload_conflict.html.erb
new file mode 100644
index 0000000..6953eec
--- /dev/null
+++ b/app/views/attachments/_upload_conflict.html.erb
@@ -0,0 +1,10 @@
+    <div class="flash warning">
+      <ul>
+        <% attachments.each do |attachment| %>
+        <%= hidden_field_tag "attachments[#{attachment.id}][id]", h(attachment.id) %>
+        <li>
+          <%= l(:notice_file_saved_temporary, {:filename => h(attachment.filename)}) %>
+        </li>
+        <% end %>
+      </ul>
+    </div>
diff --git a/app/views/issues/_edit.rhtml b/app/views/issues/_edit.rhtml
index 413f217..ac4d57d 100644
--- a/app/views/issues/_edit.rhtml
+++ b/app/views/issues/_edit.rhtml
@@ -37,6 +37,10 @@
     <%= wikitoolbar_for 'notes' %>
     <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
     
+    <% if @attachments_not_on_journal && @attachments_not_on_journal.present? %>
+    <%= render :partial => 'attachments/upload_conflict', :locals => {:attachments => @attachments_not_on_journal } %>
+    <% end %>
+
     <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form' %></p>
     </fieldset>
     </div>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 184788e..4e2be7b 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -147,6 +147,7 @@ en:
   notice_account_pending: "Your account was created and is now pending administrator approval."
   notice_default_data_loaded: Default configuration successfully loaded.
   notice_unable_delete_version: Unable to delete version.
+  notice_file_saved_temporary: "File '{{filename}}' was saved.  To permanently save it with a note, please submit this form."
   
   error_can_t_load_default_data: "Default configuration could not be loaded: {{value}}"
   error_scm_not_found: "The entry or revision was not found in the repository."
@@ -823,6 +824,7 @@ en:
   text_wiki_page_nullify_children: "Keep child pages as root pages"
   text_wiki_page_destroy_children: "Delete child pages and all their descendants"
   text_wiki_page_reassign_children: "Reassign child pages to this parent page"
+  text_changed_to: "{{label}} changed to {{new}}"
   
   default_role_manager: Manager
   default_role_developper: Developer
diff --git a/lib/stale_object.rb b/lib/stale_object.rb
new file mode 100644
index 0000000..8419fe2
--- /dev/null
+++ b/lib/stale_object.rb
@@ -0,0 +1,50 @@
+class StaleObject
+  include Redmine::I18n
+  include ERB::Util
+  attr_accessor :attributes
+  attr_accessor :changes
+  
+  def initialize(stale_object)
+    raise ArgumentError.new("Call with an ActiveRecord object") unless stale_object.respond_to?(:attributes)
+    @attributes = stale_object.attributes.dup
+  end
+  
+  def difference_messages(fresh_object, options = { })
+    options = { 
+      :wrap => "ul",
+      :item => "li"
+    }.merge(options)
+
+    changes = self.changes(fresh_object)
+    
+    if changes.empty?
+      error_messages = ''
+    else
+      error_messages = "<#{options[:wrap]}>"
+      changes.each do |key,value|
+        if key.match(/(.*)(_id)$/)
+          association = fresh_object.class.reflect_on_association($1.to_sym)
+          if association
+            field = 'field_' + key.sub($2,'')
+            if value.nil?
+              data_value = l(:label_none)
+            else
+              data_value = association.klass.find(value)
+            end
+          end
+        end
+        field ||= 'field_' + key
+        data_value ||= value || l(:label_none)
+
+        error_messages << "<#{options[:item]}>#{l(:text_changed_to, :label => l(field.to_sym), :new => html_escape(data_value))}</#{options[:item]}>"
+      end
+      error_messages << "</#{options[:wrap]}>"
+    end
+    
+    return error_messages
+  end
+
+  def changes(fresh_object)
+    @changes ||= @attributes.diff(fresh_object.attributes).except('lock_version') || { }
+  end
+end
diff --git a/test/unit/lib/stale_object_test.rb b/test/unit/lib/stale_object_test.rb
new file mode 100644
index 0000000..ecb33fd
--- /dev/null
+++ b/test/unit/lib/stale_object_test.rb
@@ -0,0 +1,106 @@
+# Redmine - project management software
+# Copyright (C) 2006-2008  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+require File.dirname(__FILE__) + '/../../test_helper'
+
+class StaleObjectTest < Test::Unit::TestCase
+
+  # TODO: context
+  # initialize
+  def test_attributes_set
+    issue = Issue.find(:first)
+    stale = StaleObject.new(issue)
+    assert stale.attributes == issue.attributes
+  end
+
+  def test_invalid_stale_object_should_raise_an_exception
+    object = Object.new
+    assert_raise ArgumentError do
+      stale = StaleObject.new(object)
+    end
+  end
+  
+  # difference_messages
+  def test_difference_message_with_no_changes_should_be_an_empty_string
+    issue = Issue.find(:first)
+    stale = StaleObject.new(issue)
+    assert stale.difference_messages(issue).empty?
+  end
+
+  def test_difference_message_with_changes_on_fields_should_return_the_field_names_and_values
+    issue = Issue.find(:first)
+    stale = StaleObject.new(issue)
+    issue.subject = "Changed"
+    
+    message = stale.difference_messages(issue)
+    assert_match /subject/i, message
+    assert_match /changed/i, message
+  end
+
+  def test_difference_message_with_changes_on_associations_should_return_the_association_names
+    issue = Issue.find(:first)
+    stale = StaleObject.new(issue)
+    issue.status = IssueStatus.find(6)
+    
+    message = stale.difference_messages(issue)
+    assert_match /status/i, message
+    assert_match /new/i, message
+  end
+  
+  def test_difference_message_with_changes_on_associations_should_work_with_nils
+    issue = Issue.find(:first)
+    issue.category = IssueCategory.find(:last)
+    stale = StaleObject.new(issue)
+    issue.category = nil # Set association to nil
+    
+    message = stale.difference_messages(issue)
+    assert_match /category/i, message
+    assert_match /printing/i, message
+  end
+
+  def test_difference_message_with_nil_set_on_a_field_should_show_none
+    issue = Issue.find(:first)
+    issue.estimated_hours = 6
+    stale = StaleObject.new(issue)
+    issue.estimated_hours = nil
+    
+    message = stale.difference_messages(issue)
+    assert_match /est/i, message
+    assert_match /6/i, message
+  end
+
+  def test_difference_message_should_allow_for_a_different_wrap_tag
+    issue = Issue.find(:first)
+    stale = StaleObject.new(issue)
+    issue.subject = "Changed"
+    
+    message = stale.difference_messages(issue, :wrap => "span")
+    assert_match /<span>/i, message
+    assert_match /<\/span>/i, message
+  end
+
+  def test_difference_message_should_allow_for_a_different_item_tag
+    issue = Issue.find(:first)
+    stale = StaleObject.new(issue)
+    issue.subject = "Changed"
+    
+    message = stale.difference_messages(issue, :item => "span")
+    assert_match /<span>/i, message
+    assert_match /<\/span>/i, message
+  end
+end
+
-- 
1.6.5

