Can't set parent issue when issue relations among child issues are present (#13654).

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

diff --git a/app/models/issue.rb b/app/models/issue.rb
index ba7a9b159e0bd30f7cc8e1691d55c59d942f2fb8..00a122c1b75ff2cf5cdc57a62190f7a0348f49d4 100644 (file)
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -660,7 +660,10 @@ 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))
+      elsif (@parent_issue != parent) && (
+          self.would_reschedule?(@parent_issue) ||
+          @parent_issue.self_and_ancestors.any? {|a| a.relations_from.any? {|r| r.relation_type == IssueRelation::TYPE_PRECEDES && r.issue_to.would_reschedule?(self)}}
+        )
         errors.add :parent_issue_id, :invalid
       elsif !new_record?
         # moving an existing issue
@@ -1035,101 +1038,38 @@ class Issue < ActiveRecord::Base
     IssueRelation.where("issue_to_id = ? OR issue_from_id = ?", id, id).find(relation_id)
   end
 
-  # Returns all the other issues that depend on the issue
-  # The algorithm is a modified breadth first search (bfs)
-  def all_dependent_issues(except=[])
-    # The found dependencies
-    dependencies = []
-
-    # The visited flag for every node (issue) used by the breadth first search
-    eNOT_DISCOVERED         = 0       # The issue is "new" to the algorithm, it has not seen it before.
-
-    ePROCESS_ALL            = 1       # The issue is added to the queue. Process both children and relations of
-                                      # the issue when it is processed.
-
-    ePROCESS_RELATIONS_ONLY = 2       # The issue was added to the queue and will be output as dependent issue,
-                                      # but its children will not be added to the queue when it is processed.
-
-    eRELATIONS_PROCESSED    = 3       # The related issues, the parent issue and the issue itself have been added to
-                                      # the queue, but its children have not been added.
-
-    ePROCESS_CHILDREN_ONLY  = 4       # The relations and the parent of the issue have been added to the queue, but
-                                      # the children still need to be processed.
-
-    eALL_PROCESSED          = 5       # The issue and all its children, its parent and its related issues have been
-                                      # added as dependent issues. It needs no further processing.
-
-    issue_status = Hash.new(eNOT_DISCOVERED)
-
-    # The queue
-    queue = []
-
-    # Initialize the bfs, add start node (self) to the queue
-    queue << self
-    issue_status[self] = ePROCESS_ALL
-
-    while (!queue.empty?) do
-      current_issue = queue.shift
-      current_issue_status = issue_status[current_issue]
-      dependencies << current_issue
-
-      # Add parent to queue, if not already in it.
-      parent = current_issue.parent
-      parent_status = issue_status[parent]
-
-      if parent && (parent_status == eNOT_DISCOVERED) && !except.include?(parent)
-        queue << parent
-        issue_status[parent] = ePROCESS_RELATIONS_ONLY
-      end
-
-      # Add children to queue, but only if they are not already in it and
-      # the children of the current node need to be processed.
-      if (current_issue_status == ePROCESS_CHILDREN_ONLY || current_issue_status == ePROCESS_ALL)
-        current_issue.children.each do |child|
-          next if except.include?(child)
-
-          if (issue_status[child] == eNOT_DISCOVERED)
-            queue << child
-            issue_status[child] = ePROCESS_ALL
-          elsif (issue_status[child] == eRELATIONS_PROCESSED)
-            queue << child
-            issue_status[child] = ePROCESS_CHILDREN_ONLY
-          elsif (issue_status[child] == ePROCESS_RELATIONS_ONLY)
-            queue << child
-            issue_status[child] = ePROCESS_ALL
-          end
-        end
-      end
-
-      # Add related issues to the queue, if they are not already in it.
-      current_issue.relations_from.map(&:issue_to).each do |related_issue|
-        next if except.include?(related_issue)
-
-        if (issue_status[related_issue] == eNOT_DISCOVERED)
-          queue << related_issue
-          issue_status[related_issue] = ePROCESS_ALL
-        elsif (issue_status[related_issue] == eRELATIONS_PROCESSED)
-          queue << related_issue
-          issue_status[related_issue] = ePROCESS_CHILDREN_ONLY
-        elsif (issue_status[related_issue] == ePROCESS_RELATIONS_ONLY)
-          queue << related_issue
-          issue_status[related_issue] = ePROCESS_ALL
-        end
-      end
-
-      # Set new status for current issue
-      if (current_issue_status == ePROCESS_ALL) || (current_issue_status == ePROCESS_CHILDREN_ONLY)
-        issue_status[current_issue] = eALL_PROCESSED
-      elsif (current_issue_status == ePROCESS_RELATIONS_ONLY)
-        issue_status[current_issue] = eRELATIONS_PROCESSED
-      end
-    end # while
-
-    # Remove the issues from the "except" parameter from the result array
-    dependencies -= except
-    dependencies.delete(self)
-
-    dependencies
+  # Returns true if this issue blocks the other issue, otherwise returns false
+  def blocks?(other)
+    all = [self]
+    last = [self]
+    while last.any?
+      current = last.map {|i| i.relations_from.where(:relation_type => IssueRelation::TYPE_BLOCKS).map(&:issue_to)}.flatten.uniq
+      current -= last
+      current -= all
+      return true if current.include?(other)
+      last = current
+      all += last
+    end
+    false
+  end
+
+  # Returns true if the other issue might be rescheduled if the start/due dates of this issue change
+  def would_reschedule?(other)
+    all = [self]
+    last = [self]
+    while last.any?
+      current = last.map {|i|
+        i.relations_from.where(:relation_type => IssueRelation::TYPE_PRECEDES).map(&:issue_to) +
+        i.leaves.to_a +
+        i.ancestors.map {|a| a.relations_from.where(:relation_type => IssueRelation::TYPE_PRECEDES).map(&:issue_to)}
+      }.flatten.uniq
+      current -= last
+      current -= all
+      return true if current.include?(other)
+      last = current
+      all += last
+    end
+    false
   end
 
   # Returns an array of issues that duplicate this one

diff --git a/app/models/issue_relation.rb b/app/models/issue_relation.rb
index 9df4e2b71b3aefa893bc4955b7c1f9eae7fc80f2..3e7e4235c71b508e762eaafb9e3fea8170386813 100644 (file)
--- a/app/models/issue_relation.rb
+++ b/app/models/issue_relation.rb
@@ -101,11 +101,8 @@ class IssueRelation < ActiveRecord::Base
                 Setting.cross_project_issue_relations?
         errors.add :issue_to_id, :not_same_project
       end
-      # detect circular dependencies depending wether the relation should be reversed
-      if TYPES.has_key?(relation_type) && TYPES[relation_type][:reverse]
-        errors.add :base, :circular_dependency if issue_from.all_dependent_issues.include? issue_to
-      else
-        errors.add :base, :circular_dependency if issue_to.all_dependent_issues.include? issue_from
+      if circular_dependency?
+        errors.add :base, :circular_dependency
       end
       if issue_from.is_descendant_of?(issue_to) || issue_from.is_ancestor_of?(issue_to)
         errors.add :base, :cant_link_an_issue_with_a_descendant
@@ -197,6 +194,22 @@ class IssueRelation < ActiveRecord::Base
     end
   end
 
+  # Returns true if the relation would create a circular dependency
+  def circular_dependency?
+    case relation_type
+    when 'follows'
+      issue_from.would_reschedule? issue_to
+    when 'precedes'
+      issue_to.would_reschedule? issue_from
+    when 'blocked'
+      issue_from.blocks? issue_to
+    when 'blocks'
+      issue_to.blocks? issue_from
+    else
+      false
+    end
+  end
+
   def call_issues_relation_added_callback
     call_issues_callback :relation_added
   end

diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb
index 8527554734db10f9fae52eb7e4f9e7f1d1e6c84f..f4f2398342a975fe5d5f9cb7d796db7a2b8d2f39 100644 (file)
--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -1848,20 +1848,49 @@ class IssueTest < ActiveSupport::TestCase
     end
   end
 
-  def test_setting_parent_to_a_dependent_issue_should_not_validate
+  def test_setting_parent_to_a_an_issue_that_precedes_should_not_validate
+    # tests that 3 cannot have 1 as parent:
+    #
+    # 1 -> 2 -> 3
+    #
     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)
+    IssueRelation.create!(:issue_from => issue2, :issue_to => issue3, :relation_type => IssueRelation::TYPE_PRECEDES)
     issue3.reload
-    issue3.parent_issue_id = issue2.id
+    issue3.parent_issue_id = issue1.id
     assert !issue3.valid?
     assert_include 'Parent task is invalid', issue3.errors.full_messages
   end
 
-  def test_setting_parent_should_not_allow_circular_dependency
+  def test_setting_parent_to_a_an_issue_that_follows_should_not_validate
+    # tests that 1 cannot have 3 as parent:
+    #
+    # 1 -> 2 -> 3
+    #
+    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 => issue2, :issue_to => issue3, :relation_type => IssueRelation::TYPE_PRECEDES)
+    issue1.reload
+    issue1.parent_issue_id = issue3.id
+    assert !issue1.valid?
+    assert_include 'Parent task is invalid', issue1.errors.full_messages
+  end
+
+  def test_setting_parent_to_a_an_issue_that_precedes_through_hierarchy_should_not_validate
+    # tests that 4 cannot have 1 as parent:
+    # changing the due date of 4 would update the end date of 1 which would reschedule 2
+    # which would change the end date of 3 which would reschedule 4 and so on...
+    #
+    #      3 -> 4
+    #      ^
+    # 1 -> 2
+    #
     set_language_if_valid 'en'
     issue1 = Issue.generate!
     issue2 = Issue.generate!
@@ -1878,6 +1907,62 @@ class IssueTest < ActiveSupport::TestCase
     assert_include 'Parent task is invalid', issue4.errors.full_messages
   end
 
+  def test_issue_and_following_issue_should_be_able_to_be_moved_to_the_same_parent
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    relation = IssueRelation.create!(:issue_from => issue2, :issue_to => issue1, :relation_type => IssueRelation::TYPE_FOLLOWS)
+    parent = Issue.generate!
+    issue1.reload.parent_issue_id = parent.id
+    assert_save issue1
+    parent.reload
+    issue2.reload.parent_issue_id = parent.id
+    assert_save issue2
+    assert IssueRelation.exists?(relation.id)
+  end
+
+  def test_issue_and_preceding_issue_should_be_able_to_be_moved_to_the_same_parent
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    relation = IssueRelation.create!(:issue_from => issue2, :issue_to => issue1, :relation_type => IssueRelation::TYPE_PRECEDES)
+    parent = Issue.generate!
+    issue1.reload.parent_issue_id = parent.id
+    assert_save issue1
+    parent.reload
+    issue2.reload.parent_issue_id = parent.id
+    assert_save issue2
+    assert IssueRelation.exists?(relation.id)
+  end
+
+  def test_issue_and_blocked_issue_should_be_able_to_be_moved_to_the_same_parent
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    relation = IssueRelation.create!(:issue_from => issue2, :issue_to => issue1, :relation_type => IssueRelation::TYPE_BLOCKED)
+    parent = Issue.generate!
+    issue1.reload.parent_issue_id = parent.id
+    assert_save issue1
+    parent.reload
+    issue2.reload.parent_issue_id = parent.id
+    assert_save issue2
+    assert IssueRelation.exists?(relation.id)
+  end
+
+  def test_issue_and_blocking_issue_should_be_able_to_be_moved_to_the_same_parent
+    set_language_if_valid 'en'
+    issue1 = Issue.generate!
+    issue2 = Issue.generate!
+    relation = IssueRelation.create!(:issue_from => issue2, :issue_to => issue1, :relation_type => IssueRelation::TYPE_BLOCKS)
+    parent = Issue.generate!
+    issue1.reload.parent_issue_id = parent.id
+    assert_save issue1
+    parent.reload
+    issue2.reload.parent_issue_id = parent.id
+    assert_save issue2
+    assert IssueRelation.exists?(relation.id)
+  end
+
   def test_overdue
     assert Issue.new(:due_date => 1.day.ago.to_date).overdue?
     assert !Issue.new(:due_date => Date.today).overdue?
@@ -2163,193 +2248,6 @@ class IssueTest < ActiveSupport::TestCase
     end
   end
 
-  def test_all_dependent_issues
-    IssueRelation.delete_all
-    assert IssueRelation.create!(:issue_from => Issue.find(1),
-                                 :issue_to   => Issue.find(2),
-                                 :relation_type => IssueRelation::TYPE_PRECEDES)
-    assert IssueRelation.create!(:issue_from => Issue.find(2),
-                                 :issue_to   => Issue.find(3),
-                                 :relation_type => IssueRelation::TYPE_PRECEDES)
-    assert IssueRelation.create!(:issue_from => Issue.find(3),
-                                 :issue_to   => Issue.find(8),
-                                 :relation_type => IssueRelation::TYPE_PRECEDES)
-
-    assert_equal [2, 3, 8], Issue.find(1).all_dependent_issues.collect(&:id).sort
-  end
-
-  def test_all_dependent_issues_with_subtask
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue = Issue.generate!(:project => project)
-    childIssue1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue.id)
-    childIssue2 = Issue.generate!(:project => project, :parent_issue_id => parentIssue.id)
-
-    assert_equal [childIssue1.id, childIssue2.id].sort, parentIssue.all_dependent_issues.collect(&:id).uniq.sort
-  end
-
-  def test_all_dependent_issues_does_not_include_self
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue = Issue.generate!(:project => project)
-    childIssue = Issue.generate!(:project => project, :parent_issue_id => parentIssue.id)
-
-    assert_equal [childIssue.id], parentIssue.all_dependent_issues.collect(&:id)
-  end
-
-  def test_all_dependent_issues_with_parenttask_and_sibling
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue = Issue.generate!(:project => project)
-    childIssue1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue.id)
-    childIssue2 = Issue.generate!(:project => project, :parent_issue_id => parentIssue.id)
-
-    assert_equal [parentIssue.id].sort, childIssue1.all_dependent_issues.collect(&:id)
-  end
-
-  def test_all_dependent_issues_with_relation_to_leaf_in_other_tree
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue1 = Issue.generate!(:project => project)
-    childIssue1_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue1.id)
-    childIssue1_2 = Issue.generate!(:project => project, :parent_issue_id => parentIssue1.id)
-
-    parentIssue2 = Issue.generate!(:project => project)
-    childIssue2_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue2.id)
-    childIssue2_2 = Issue.generate!(:project => project, :parent_issue_id => parentIssue2.id)
-
-
-    assert IssueRelation.create(:issue_from => parentIssue1,
-                                :issue_to   => childIssue2_2,
-                                :relation_type => IssueRelation::TYPE_BLOCKS)
-
-    assert_equal [childIssue1_1.id, childIssue1_2.id, parentIssue2.id, childIssue2_2.id].sort,
-                 parentIssue1.all_dependent_issues.collect(&:id).uniq.sort
-  end
-
-  def test_all_dependent_issues_with_relation_to_parent_in_other_tree
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue1 = Issue.generate!(:project => project)
-    childIssue1_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue1.id)
-    childIssue1_2 = Issue.generate!(:project => project, :parent_issue_id => parentIssue1.id)
-
-    parentIssue2 = Issue.generate!(:project => project)
-    childIssue2_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue2.id)
-    childIssue2_2 = Issue.generate!(:project => project, :parent_issue_id => parentIssue2.id)
-
-
-    assert IssueRelation.create(:issue_from => parentIssue1,
-                                :issue_to   => parentIssue2,
-                                :relation_type => IssueRelation::TYPE_BLOCKS)
-
-    assert_equal [childIssue1_1.id, childIssue1_2.id, parentIssue2.id, childIssue2_1.id, childIssue2_2.id].sort,
-                 parentIssue1.all_dependent_issues.collect(&:id).uniq.sort
-  end
-
-  def test_all_dependent_issues_with_transitive_relation
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue1 = Issue.generate!(:project => project)
-    childIssue1_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue1.id)
-
-    parentIssue2 = Issue.generate!(:project => project)
-    childIssue2_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue2.id)
-
-    independentIssue = Issue.generate!(:project => project)
-
-    assert IssueRelation.create(:issue_from => parentIssue1,
-                                :issue_to   => childIssue2_1,
-                                :relation_type => IssueRelation::TYPE_RELATES)
-
-    assert IssueRelation.create(:issue_from => childIssue2_1,
-                                :issue_to   => independentIssue,
-                                :relation_type => IssueRelation::TYPE_RELATES)
-
-    assert_equal [childIssue1_1.id, parentIssue2.id, childIssue2_1.id, independentIssue.id].sort,
-                 parentIssue1.all_dependent_issues.collect(&:id).uniq.sort
-  end
-
-  def test_all_dependent_issues_with_transitive_relation2
-    IssueRelation.delete_all
-
-    project = Project.generate!(:name => "testproject")
-
-    parentIssue1 = Issue.generate!(:project => project)
-    childIssue1_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue1.id)
-
-    parentIssue2 = Issue.generate!(:project => project)
-    childIssue2_1 = Issue.generate!(:project => project, :parent_issue_id => parentIssue2.id)
-
-    independentIssue = Issue.generate!(:project => project)
-
-    assert IssueRelation.create(:issue_from => parentIssue1,
-                                :issue_to   => independentIssue,
-                                :relation_type => IssueRelation::TYPE_RELATES)
-
-    assert IssueRelation.create(:issue_from => independentIssue,
-                                :issue_to   => childIssue2_1,
-                                :relation_type => IssueRelation::TYPE_RELATES)
-
-    assert_equal [childIssue1_1.id, parentIssue2.id, childIssue2_1.id, independentIssue.id].sort,
-                 parentIssue1.all_dependent_issues.collect(&:id).uniq.sort
-
-  end
-
-  def test_all_dependent_issues_with_persistent_circular_dependency
-    IssueRelation.delete_all
-    assert IssueRelation.create!(:issue_from => Issue.find(1),
-                                 :issue_to   => Issue.find(2),
-                                 :relation_type => IssueRelation::TYPE_PRECEDES)
-    assert IssueRelation.create!(:issue_from => Issue.find(2),
-                                 :issue_to   => Issue.find(3),
-                                 :relation_type => IssueRelation::TYPE_PRECEDES)
-
-    r = IssueRelation.create!(:issue_from => Issue.find(3),
-                             :issue_to   => Issue.find(7),
-                             :relation_type => IssueRelation::TYPE_PRECEDES)
-    IssueRelation.where(["id = ?", r.id]).update_all("issue_to_id = 1")
-
-    assert_equal [2, 3], Issue.find(1).all_dependent_issues.collect(&:id).sort
-  end
-
-  def test_all_dependent_issues_with_persistent_multiple_circular_dependencies
-    IssueRelation.delete_all
-    assert IssueRelation.create!(:issue_from => Issue.find(1),
-                                 :issue_to   => Issue.find(2),
-                                 :relation_type => IssueRelation::TYPE_RELATES)
-    assert IssueRelation.create!(:issue_from => Issue.find(2),
-                                 :issue_to   => Issue.find(3),
-                                 :relation_type => IssueRelation::TYPE_RELATES)
-    assert IssueRelation.create!(:issue_from => Issue.find(3),
-                                 :issue_to   => Issue.find(8),
-                                 :relation_type => IssueRelation::TYPE_RELATES)
-
-    r = IssueRelation.create!(:issue_from => Issue.find(8),
-                             :issue_to   => Issue.find(7),
-                             :relation_type => IssueRelation::TYPE_RELATES)
-    IssueRelation.where(["id = ?", r.id]).update_all("issue_to_id = 2")
-
-    r = IssueRelation.create!(:issue_from => Issue.find(3),
-                             :issue_to   => Issue.find(7),
-                             :relation_type => IssueRelation::TYPE_RELATES)
-    IssueRelation.where(["id = ?", r.id]).update_all("issue_to_id = 1")
-
-    assert_equal [2, 3, 8], Issue.find(1).all_dependent_issues.collect(&:id).sort
-  end
-
   test "#done_ratio should use the issue_status according to Setting.issue_done_ratio" do
     @issue = Issue.find(1)
     @issue_status = IssueStatus.find(1)