Fixed: journal details duplicated when an issue is saved twice (#3690).

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

app/models/issue.rb

@@ -58,7 +58,8 @@ class Issue < ActiveRecord::Base
   
   named_scope :open, :conditions => ["#{IssueStatus.table_name}.is_closed = ?", false], :include => :status
 
-  before_save :update_done_ratio_from_issue_status
+  before_create :default_assign
+  before_save :reschedule_following_issues, :close_duplicates, :update_done_ratio_from_issue_status
   after_save :create_journal
   
   # Returns true if usr or current user is allowed to view the issue
@@ -242,13 +243,6 @@ class Issue < ActiveRecord::Base
     end
   end
   
-  def before_create
-    # default assignment based on category
-    if assigned_to.nil? && category && category.assigned_to
-      self.assigned_to = category.assigned_to
-    end
-  end
-  
   # Set the done_ratio using the status if that setting is set.  This will keep the done_ratios
   # even if the user turns off the setting later
   def update_done_ratio_from_issue_status
@@ -257,27 +251,6 @@ class Issue < ActiveRecord::Base
     end
   end
   
-  def after_save
-    # Reload is needed in order to get the right status
-    reload
-    
-    # Update start/due dates of following issues
-    relations_from.each(&:set_issue_to_dates)
-    
-    # Close duplicates if the issue was closed
-    if @issue_before_change && !@issue_before_change.closed? && self.closed?
-      duplicates.each do |duplicate|
-        # Reload is need in case the duplicate was updated by a previous duplicate
-        duplicate.reload
-        # Don't re-close it if it's already closed
-        next if duplicate.closed?
-        # Same user and notes
-        duplicate.init_journal(@current_journal.user, @current_journal.notes)
-        duplicate.update_attribute :status, self.status
-      end
-    end
-  end
-  
   def init_journal(user, notes = "")
     @current_journal ||= Journal.new(:journalized => self, :user => user, :notes => notes)
     @issue_before_change = self.clone
@@ -305,6 +278,18 @@ class Issue < ActiveRecord::Base
     end
     false
   end
+
+  # Return true if the issue is being closed
+  def closing?
+    if !new_record? && status_id_changed?
+      status_was = IssueStatus.find_by_id(status_id_was)
+      status_new = IssueStatus.find_by_id(status_id)
+      if status_was && status_new && !status_was.is_closed? && status_new.is_closed?
+        return true
+      end
+    end
+    false
+  end
   
   # Returns true if the issue is overdue
   def overdue?
@@ -502,6 +487,39 @@ class Issue < ActiveRecord::Base
     journal.save
   end
   
+  # Default assignment based on category
+  def default_assign
+    if assigned_to.nil? && category && category.assigned_to
+      self.assigned_to = category.assigned_to
+    end
+  end
+
+  # Updates start/due dates of following issues
+  def reschedule_following_issues
+    if start_date_changed? || due_date_changed?
+      relations_from.each do |relation|
+        relation.set_issue_to_dates
+      end
+    end
+  end
+
+  # Closes duplicates if the issue is being closed
+  def close_duplicates
+    if closing?
+      duplicates.each do |duplicate|
+        # Reload is need in case the duplicate was updated by a previous duplicate
+        duplicate.reload
+        # Don't re-close it if it's already closed
+        next if duplicate.closed?
+        # Same user and notes
+        if @current_journal
+          duplicate.init_journal(@current_journal.user, @current_journal.notes)
+        end
+        duplicate.update_attribute :status, self.status
+      end
+    end
+  end
+  
   # Saves the changes in a Journal
   # Called after_save
   def create_journal
@@ -523,6 +541,8 @@ class Issue < ActiveRecord::Base
                                                       :value => c.value)
       }      
       @current_journal.save
+      # reset current journal
+      init_journal @current_journal.user, @current_journal.notes
     end
   end
 

test/unit/issue_test.rb

@@ -529,6 +529,32 @@ class IssueTest < ActiveSupport::TestCase
     end
     assert ActionMailer::Base.deliveries.empty?
   end
+  
+  def test_saving_twice_should_not_duplicate_journal_details
+    i = Issue.find(:first)
+    i.init_journal(User.find(2), 'Some notes')
+    # 2 changes
+    i.subject = 'New subject'
+    i.done_ratio = i.done_ratio + 10
+    assert_difference 'Journal.count' do
+      assert_difference 'JournalDetail.count', 2 do
+        assert i.save
+      end
+    end
+    # 1 more change
+    i.priority = IssuePriority.find(:first, :conditions => ["id <> ?", i.priority_id])
+    assert_no_difference 'Journal.count' do
+      assert_difference 'JournalDetail.count', 1 do
+        i.save
+      end
+    end
+    # no more change
+    assert_no_difference 'Journal.count' do
+      assert_no_difference 'JournalDetail.count' do
+        i.save
+      end
+    end
+  end
 
   context "#done_ratio" do
     setup do