Fixed: Circular loop when using relations and subtasks (#8794).

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

app/models/issue.rb

@@ -576,6 +576,8 @@ class Issue < ActiveRecord::Base
     elsif @parent_issue
       if !valid_parent_project?(@parent_issue)
         errors.add :parent_issue_id, :invalid
+      elsif (@parent_issue != parent) && (all_dependent_issues.include?(@parent_issue) || @parent_issue.all_dependent_issues.include?(self))
+        errors.add :parent_issue_id, :invalid
       elsif !new_record?
         # moving an existing issue
         if @parent_issue.root_id != root_id
@@ -860,6 +862,18 @@ class Issue < ActiveRecord::Base
         dependencies += relation.issue_to.all_dependent_issues(except)
       end
     end
+    unless leaf?
+      children.each do |child|
+        if !except.include?(child)
+          dependencies << child
+          dependencies += child.all_dependent_issues(except)
+        end
+      end
+    end
+    if parent && !except.include?(parent)
+      dependencies << parent
+      dependencies += parent.all_dependent_issues(except)
+    end
     dependencies
   end
 

test/functional/issues_controller_test.rb

@@ -3474,6 +3474,8 @@ class IssuesControllerTest < ActionController::TestCase
   end
 
   def test_bulk_update_parent_id
+    IssueRelation.delete_all
+
     @request.session[:user_id] = 2
     post :bulk_update, :ids => [1, 3],
       :notes => 'Bulk editing parent',

test/unit/issue_nested_set_test.rb

@@ -166,22 +166,6 @@ class IssueNestedSetTest < ActiveSupport::TestCase
     assert_not_nil child.errors[:parent_issue_id]
   end
 
-  def test_moving_an_issue_should_keep_valid_relations_only
-    issue1 = Issue.generate!
-    issue2 = Issue.generate!
-    issue3 = Issue.generate!(:parent_issue_id => issue2.id)
-    issue4 = Issue.generate!
-    r1 = IssueRelation.create!(:issue_from => issue1, :issue_to => issue2, :relation_type => IssueRelation::TYPE_PRECEDES)
-    r2 = IssueRelation.create!(:issue_from => issue1, :issue_to => issue3, :relation_type => IssueRelation::TYPE_PRECEDES)
-    r3 = IssueRelation.create!(:issue_from => issue2, :issue_to => issue4, :relation_type => IssueRelation::TYPE_PRECEDES)
-    issue2.reload
-    issue2.parent_issue_id = issue1.id
-    issue2.save!
-    assert !IssueRelation.exists?(r1.id)
-    assert !IssueRelation.exists?(r2.id)
-    assert IssueRelation.exists?(r3.id)
-  end
-
   def test_destroy_should_destroy_children
     issue1 = Issue.generate!
     issue2 = Issue.generate!

test/unit/issue_relation_test.rb

@@ -30,6 +30,8 @@ class IssueRelationTest < ActiveSupport::TestCase
            :enumerations,
            :trackers
 
+  include Redmine::I18n
+
   def test_create
     from = Issue.find(1)
     to = Issue.find(2)
@@ -115,6 +117,26 @@ class IssueRelationTest < ActiveSupport::TestCase
     assert_not_nil r.errors[:base]
   end
 
+  def test_validates_circular_dependency_of_subtask
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    IssueRelation.create!(
+      :issue_from => issue1, :issue_to => issue2,
+      :relation_type => IssueRelation::TYPE_PRECEDES
+    )
+    child = Issue.generate!(:parent_issue_id => issue2.id)
+    issue1.reload
+    child.reload
+
+    r = IssueRelation.new(
+          :issue_from => child, :issue_to => issue1,
+          :relation_type => IssueRelation::TYPE_PRECEDES
+        )
+    assert !r.save
+    assert_include 'This relation would create a circular dependency', r.errors.full_messages
+  end
+
   def test_validates_circular_dependency_on_reverse_relations
     IssueRelation.delete_all
     assert IssueRelation.create!(

test/unit/issue_test.rb

@@ -1531,6 +1531,36 @@ class IssueTest < ActiveSupport::TestCase
     assert child.save
   end
 
+  def test_setting_parent_to_a_dependent_issue_should_not_validate
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    issue3 = Issue.generate!
+    IssueRelation.create!(:issue_from => issue1, :issue_to => issue2, :relation_type => IssueRelation::TYPE_PRECEDES)
+    IssueRelation.create!(:issue_from => issue3, :issue_to => issue1, :relation_type => IssueRelation::TYPE_PRECEDES)
+    issue3.reload
+    issue3.parent_issue_id = issue2.id
+    assert !issue3.valid?
+    assert_include 'Parent task is invalid', issue3.errors.full_messages
+  end
+
+  def test_setting_parent_should_not_allow_circular_dependency
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    IssueRelation.create!(:issue_from => issue1, :issue_to => issue2, :relation_type => IssueRelation::TYPE_PRECEDES)
+    issue3 = Issue.generate!
+    issue2.reload
+    issue2.parent_issue_id = issue3.id
+    issue2.save!
+    issue4 = Issue.generate!
+    IssueRelation.create!(:issue_from => issue3, :issue_to => issue4, :relation_type => IssueRelation::TYPE_PRECEDES)
+    issue4.reload
+    issue4.parent_issue_id = issue1.id
+    assert !issue4.valid?
+    assert_include 'Parent task is invalid', issue4.errors.full_messages
+  end
+
   def test_overdue
     assert Issue.new(:due_date => 1.day.ago.to_date).overdue?
     assert !Issue.new(:due_date => Date.today).overdue?