Fixed circular dependencies possibly introduced when using reverse relations, for instance "blocked by" relations (#8616).

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

app/models/issue_relation.rb

@@ -47,7 +47,12 @@ class IssueRelation < ActiveRecord::Base
     if issue_from && issue_to
       errors.add :issue_to_id, :invalid if issue_from_id == issue_to_id
       errors.add :issue_to_id, :not_same_project unless issue_from.project_id == issue_to.project_id || Setting.cross_project_issue_relations?
-      errors.add_to_base :circular_dependency if issue_to.all_dependent_issues.include? issue_from
+      #detect circular dependencies depending wether the relation should be reversed
+      if TYPES.has_key?(relation_type) && TYPES[relation_type][:reverse]
+        errors.add_to_base :circular_dependency if issue_from.all_dependent_issues.include? issue_to
+      else
+        errors.add_to_base :circular_dependency if issue_to.all_dependent_issues.include? issue_from
+      end
       errors.add_to_base :cant_link_an_issue_with_a_descendant if issue_from.is_descendant_of?(issue_to) || issue_from.is_ancestor_of?(issue_to)
     end
   end

test/functional/issue_relations_controller_test.rb

@@ -74,6 +74,8 @@ class IssueRelationsControllerTest < ActionController::TestCase
                  :relation => {:issue_to_id => '4', :relation_type => 'relates', :delay => ''}
     end
   end
+
+  should "prevent relation creation when there's a circular dependency"
   
   def test_destroy
     assert_difference 'IssueRelation.count', -1 do

test/unit/issue_relation_test.rb

@@ -44,6 +44,9 @@ class IssueRelationTest < ActiveSupport::TestCase
     assert_equal from, relation.issue_to
   end
   
+  # TODO : document why it shouldn't be reversed if validation fails : having
+  # relations reversed before the validation would allow simpler code for the
+  # validation
   def test_follows_relation_should_not_be_reversed_if_validation_fails
     from = Issue.find(1)
     to = Issue.find(2)
@@ -82,4 +85,13 @@ class IssueRelationTest < ActiveSupport::TestCase
     assert !r.save
     assert_not_nil r.errors.on(:base)
   end
+
+  def test_validates_circular_dependency_on_reverse_relations
+    IssueRelation.delete_all
+    assert IssueRelation.create!(:issue_from => Issue.find(1), :issue_to => Issue.find(3), :relation_type => IssueRelation::TYPE_BLOCKS)
+    assert IssueRelation.create!(:issue_from => Issue.find(1), :issue_to => Issue.find(2), :relation_type => IssueRelation::TYPE_BLOCKED)
+    r = IssueRelation.new(:issue_from => Issue.find(2), :issue_to => Issue.find(1), :relation_type => IssueRelation::TYPE_BLOCKED)
+    assert !r.save
+    assert_not_nil r.errors.on(:base)
+  end
 end