From 38b3e045cf221e8d1aad1ea7fd9e2a86ea0ff5f6 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 17 Mar 2013 15:02:57 +0000 Subject: [PATCH] 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 | 14 +++++++++++ test/functional/issues_controller_test.rb | 2 ++ test/unit/issue_nested_set_test.rb | 16 ------------ test/unit/issue_relation_test.rb | 22 +++++++++++++++++ test/unit/issue_test.rb | 30 +++++++++++++++++++++++ 5 files changed, 68 insertions(+), 16 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index aed727ae0..3b4c79def 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 @@ -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 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 905d5483e..50f9e0437 100644 --- a/test/unit/issue_test.rb +++ b/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? -- 2.39.5