Fixed that IssueRelation should not be responsible for calling Issue#init_journal (#18237).

git-svn-id: http://svn.redmine.org/redmine/trunk@13534 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/issue_relations_controller.rb

@@ -45,6 +45,7 @@ class IssueRelationsController < ApplicationController
     if params[:relation] && m = params[:relation][:issue_to_id].to_s.strip.match(/^#?(\d+)$/)
       @relation.issue_to = Issue.visible.find_by_id(m[1].to_i)
     end
+    @relation.init_journals(User.current)
     saved = @relation.save
 
     respond_to do |format|
@@ -64,6 +65,7 @@ class IssueRelationsController < ApplicationController
 
   def destroy
     raise Unauthorized unless @relation.deletable?
+    @relation.init_journals(User.current)
     @relation.destroy
 
     respond_to do |format|

app/controllers/issues_controller.rb

@@ -406,6 +406,7 @@ class IssuesController < ApplicationController
   def build_new_issue_from_params
     if params[:id].blank?
       @issue = Issue.new
+      @issue.init_journal(User.current)
       if params[:copy_from]
         begin
           @copy_from = Issue.visible.find(params[:copy_from])

app/models/issue.rb

@@ -667,6 +667,11 @@ class Issue < ActiveRecord::Base
     @current_journal
   end
 
+	# Returns the current journal or nil if it's not initialized
+  def current_journal
+    @current_journal
+  end
+
   # Returns the id of the last journal or nil
   def last_journal_id
     if new_record?
@@ -1308,6 +1313,9 @@ class Issue < ActiveRecord::Base
     return unless copy? && !@after_create_from_copy_handled
 
     if (@copied_from.project_id == project_id || Setting.cross_project_issue_relations?) && @copy_options[:link] != false
+      if @current_journal
+        @copied_from.init_journal(@current_journal.user)
+      end
       relation = IssueRelation.new(:issue_from => @copied_from, :issue_to => self, :relation_type => IssueRelation::TYPE_COPIED_TO)
       unless relation.save
         logger.error "Could not create relation while copying ##{@copied_from.id} to ##{id} due to validation errors: #{relation.errors.full_messages.join(', ')}" if logger
@@ -1328,6 +1336,9 @@ class Issue < ActiveRecord::Base
           next
         end
         copy = Issue.new.copy_from(child, copy_options)
+        if @current_journal
+          copy.init_journal(@current_journal.user)
+        end
         copy.author = author
         copy.project = project
         copy.parent_issue_id = copied_issue_ids[child.parent_id]
@@ -1477,6 +1488,30 @@ class Issue < ActiveRecord::Base
     end
   end
 
+  # Called after a relation is added
+  def relation_added(relation)
+    if @current_journal
+      @current_journal.details << JournalDetail.new(
+        :property  => 'relation',
+        :prop_key  => relation.relation_type_for(self),
+        :value => relation.other_issue(self).try(:id)
+      )
+      @current_journal.save
+    end
+  end
+
+  # Called after a relation is removed
+  def relation_removed(relation)
+    if @current_journal
+      @current_journal.details << JournalDetail.new(
+        :property  => 'relation',
+        :prop_key  => relation.relation_type_for(self),
+        :old_value => relation.other_issue(self).try(:id)
+      )
+      @current_journal.save
+    end
+  end
+
   # Default assignment based on category
   def default_assign
     if assigned_to.nil? && category && category.assigned_to

app/models/issue_relation.rb

@@ -72,8 +72,8 @@ class IssueRelation < ActiveRecord::Base
 
   attr_protected :issue_from_id, :issue_to_id
   before_save :handle_issue_order
-  after_create  :create_journal_after_create
-  after_destroy :create_journal_after_delete
+  after_create  :call_issues_relation_added_callback
+  after_destroy :call_issues_relation_removed_callback
 
   def visible?(user=User.current)
     (issue_from.nil? || issue_from.visible?(user)) && (issue_to.nil? || issue_to.visible?(user))
@@ -168,6 +168,11 @@ class IssueRelation < ActiveRecord::Base
     r == 0 ? id <=> relation.id : r
   end
 
+  def init_journals(user)
+    issue_from.init_journal(user) if issue_from
+    issue_to.init_journal(user) if issue_to
+  end
+
   private
 
   # Reverses the relation if needed so that it gets stored in the proper way
@@ -182,29 +187,19 @@ class IssueRelation < ActiveRecord::Base
     end
   end
 
-  def create_journal_after_create
-    journal = issue_from.init_journal(User.current)
-    journal.details << JournalDetail.new(:property => 'relation',
-                                         :prop_key => relation_type_for(issue_from),
-                                         :value    => issue_to.id)
-    journal.save
-    journal = issue_to.init_journal(User.current)
-    journal.details << JournalDetail.new(:property => 'relation',
-                                         :prop_key => relation_type_for(issue_to),
-                                         :value    => issue_from.id)
-    journal.save
+  def call_issues_relation_added_callback
+    call_issues_callback :relation_added
   end
 
-  def create_journal_after_delete
-    journal = issue_from.init_journal(User.current)
-    journal.details << JournalDetail.new(:property  => 'relation',
-                                         :prop_key  => relation_type_for(issue_from),
-                                         :old_value => issue_to.id)
-    journal.save
-    journal = issue_to.init_journal(User.current)
-    journal.details << JournalDetail.new(:property  => 'relation',
-                                         :prop_key  => relation_type_for(issue_to),
-                                         :old_value => issue_from.id)
-    journal.save
+  def call_issues_relation_removed_callback
+    call_issues_callback :relation_removed
+  end
+
+  def call_issues_callback(name)
+    [issue_from, issue_to].each do |issue|
+      if issue
+        issue.send name, self
+      end
+    end
   end
 end

test/unit/issue_relation_test.rb

@@ -169,13 +169,14 @@ class IssueRelationTest < ActiveSupport::TestCase
     assert_not_equal [], r.errors[:base]
   end
 
-  def test_create_should_make_journal_entry
+  def test_create_with_initialized_journals_should_create_journals
     from = Issue.find(1)
     to   = Issue.find(2)
     from_journals = from.journals.size
     to_journals   = to.journals.size
     relation = IssueRelation.new(:issue_from => from, :issue_to => to,
                                  :relation_type => IssueRelation::TYPE_PRECEDES)
+    relation.init_journals User.find(1)
     assert relation.save
     from.reload
     to.reload
@@ -192,12 +193,13 @@ class IssueRelationTest < ActiveSupport::TestCase
     assert_nil   to.journals.last.details.last.old_value
   end
 
-  def test_delete_should_make_journal_entry
+  def test_destroy_with_initialized_journals_should_create_journals
     relation = IssueRelation.find(1)
     from = relation.issue_from
     to   = relation.issue_to
     from_journals = from.journals.size
     to_journals   = to.journals.size
+    relation.init_journals User.find(1)
     assert relation.destroy
     from.reload
     to.reload

test/unit/journal_test.rb

@@ -207,17 +207,15 @@ class JournalTest < ActiveSupport::TestCase
   def test_visible_details_should_include_relations_to_visible_issues_only
     issue = Issue.generate!
     visible_issue = Issue.generate!
-    IssueRelation.create!(:issue_from => issue, :issue_to => visible_issue, :relation_type => 'relates')
     hidden_issue = Issue.generate!(:is_private => true)
-    IssueRelation.create!(:issue_from => issue, :issue_to => hidden_issue, :relation_type => 'relates')
-    issue.reload
-    assert_equal 1, issue.journals.size
-    journal = issue.journals.first
-    assert_equal 2, journal.details.size
+
+    journal = Journal.new
+    journal.details << JournalDetail.new(:property => 'relation', :prop_key => 'relates', :value => visible_issue.id)
+    journal.details << JournalDetail.new(:property => 'relation', :prop_key => 'relates', :value => hidden_issue.id)
 
     visible_details = journal.visible_details(User.anonymous)
     assert_equal 1, visible_details.size
-    assert_equal visible_issue.id.to_s, visible_details.first.value
+    assert_equal visible_issue.id.to_s, visible_details.first.value.to_s
 
     visible_details = journal.visible_details(User.find(2))
     assert_equal 2, visible_details.size

test/unit/mailer_test.rb

@@ -410,6 +410,7 @@ class MailerTest < ActiveSupport::TestCase
 
   def test_issue_edit_with_relation_should_notify_users_who_can_see_the_related_issue
     issue = Issue.generate!
+    issue.init_journal(User.find(1))
     private_issue = Issue.generate!(:is_private => true)
     IssueRelation.create!(:issue_from => issue, :issue_to => private_issue, :relation_type => 'relates')
     issue.reload