From: Jean-Philippe Lang Date: Sun, 10 Jan 2016 15:12:28 +0000 (+0000) Subject: Can't set parent issue when issue relations among child issues are present (#13654). X-Git-Tag: 3.3.0~314 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=3a48f09d4f11b0da737007900c16d7fd95eee767;p=redmine.git 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 ba7a9b159..00a122c1b 100644 --- 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 9df4e2b71..3e7e4235c 100644 --- 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 852755473..f4f239834 100644 --- 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)