Merged r11641 and r11642 from trunk (#8794).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/branches/2.3-stable@11656 e93f8b46-1217-0410-a6f0-8f06a7374b81

5 files changed, 63 insertions, 23 deletions

diff --git a/app/models/issue.rb b/app/models/issue.rb
index ec108a016..d93cca8bb 100644
--- a/app/models/issue.rb
+++ b/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
@@ -851,16 +853,16 @@ class Issue < ActiveRecord::Base
     IssueRelation.find(relation_id, :conditions => ["issue_to_id = ? OR issue_from_id = ?", id, id])
   end
 
+  # Returns all the other issues that depend on the issue
   def all_dependent_issues(except=[])
     except << self
     dependencies = []
-    relations_from.each do |relation|
-      if relation.issue_to && !except.include?(relation.issue_to)
-        dependencies << relation.issue_to
-        dependencies += relation.issue_to.all_dependent_issues(except)
-      end
-    end
-    dependencies
+    dependencies += relations_from.map(&:issue_to)
+    dependencies += children unless leaf?
+    dependencies << parent
+    dependencies.compact!
+    dependencies -= except
+    dependencies + dependencies.map {|issue| issue.all_dependent_issues(except)}.flatten
   end
 
   # Returns an array of issues that duplicate this one
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index a9cf46140..59f7a858a 100644
--- a/test/functional/issues_controller_test.rb
+++ b/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',
diff --git a/test/unit/issue_nested_set_test.rb b/test/unit/issue_nested_set_test.rb
index dc0c3d3aa..c3120e82a 100644
--- a/test/unit/issue_nested_set_test.rb
+++ b/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!
diff --git a/test/unit/issue_relation_test.rb b/test/unit/issue_relation_test.rb
index ab67592e0..f15a74c34 100644
--- a/test/unit/issue_relation_test.rb
+++ b/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!(
diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb
index d0f44140b..1b35c0595 100644
--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -1528,6 +1528,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?