jro_apps/redmine Commit - r6004:f982c5b90d52

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

Jean-Baptiste Barth -

r6004:f982c5b90d52

parent child

Context file:

r6004:f982c5b90d52

Collapse all files

app/models/issue_relation.rb +6 -1

             # redMine - project management software
             # Copyright (C) 2006-2007  Jean-Philippe Lang
             #
             # This program is free software; you can redistribute it and/or
             # modify it under the terms of the GNU General Public License
             # as published by the Free Software Foundation; either version 2
             # of the License, or (at your option) any later version.
             #
             # This program is distributed in the hope that it will be useful,
             # but WITHOUT ANY WARRANTY; without even the implied warranty of
             # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
             # GNU General Public License for more details.
             #
             # You should have received a copy of the GNU General Public License
             # along with this program; if not, write to the Free Software
             # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
             class IssueRelation < ActiveRecord::Base
               belongs_to :issue_from, :class_name => 'Issue', :foreign_key => 'issue_from_id'
               belongs_to :issue_to, :class_name => 'Issue', :foreign_key => 'issue_to_id'
               TYPE_RELATES      = "relates"
               TYPE_DUPLICATES   = "duplicates"
               TYPE_DUPLICATED   = "duplicated"
               TYPE_BLOCKS       = "blocks"
               TYPE_BLOCKED      = "blocked"
               TYPE_PRECEDES     = "precedes"
               TYPE_FOLLOWS      = "follows"
               TYPES = { TYPE_RELATES =>     { :name => :label_relates_to, :sym_name => :label_relates_to, :order => 1, :sym => TYPE_RELATES },
                         TYPE_DUPLICATES =>  { :name => :label_duplicates, :sym_name => :label_duplicated_by, :order => 2, :sym => TYPE_DUPLICATED },
                         TYPE_DUPLICATED =>  { :name => :label_duplicated_by, :sym_name => :label_duplicates, :order => 3, :sym => TYPE_DUPLICATES, :reverse => TYPE_DUPLICATES },
                         TYPE_BLOCKS =>      { :name => :label_blocks, :sym_name => :label_blocked_by, :order => 4, :sym => TYPE_BLOCKED },
                         TYPE_BLOCKED =>     { :name => :label_blocked_by, :sym_name => :label_blocks, :order => 5, :sym => TYPE_BLOCKS, :reverse => TYPE_BLOCKS },
                         TYPE_PRECEDES =>    { :name => :label_precedes, :sym_name => :label_follows, :order => 6, :sym => TYPE_FOLLOWS },
                         TYPE_FOLLOWS =>     { :name => :label_follows, :sym_name => :label_precedes, :order => 7, :sym => TYPE_PRECEDES, :reverse => TYPE_PRECEDES }
                       }.freeze
               validates_presence_of :issue_from, :issue_to, :relation_type
               validates_inclusion_of :relation_type, :in => TYPES.keys
               validates_numericality_of :delay, :allow_nil => true
               validates_uniqueness_of :issue_to_id, :scope => :issue_from_id
               attr_protected :issue_from_id, :issue_to_id
               def validate
                 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
               def other_issue(issue)
                 (self.issue_from_id == issue.id) ? issue_to : issue_from
               end
               # Returns the relation type for +issue+
               def relation_type_for(issue)
                 if TYPES[relation_type]
                   if self.issue_from_id == issue.id
                     relation_type
                   else
                     TYPES[relation_type][:sym]
                   end
                 end
               end
               def label_for(issue)
                 TYPES[relation_type] ? TYPES[relation_type][(self.issue_from_id == issue.id) ? :name : :sym_name] : :unknow
               end
               def before_save
                 reverse_if_needed
                 if TYPE_PRECEDES == relation_type
                   self.delay ||= 0
                 else
                   self.delay = nil
                 end
                 set_issue_to_dates
               end
               def set_issue_to_dates
                 soonest_start = self.successor_soonest_start
                 if soonest_start && issue_to
                   issue_to.reschedule_after(soonest_start)
                 end
               end
               def successor_soonest_start
                 if (TYPE_PRECEDES == self.relation_type) && delay && issue_from && (issue_from.start_date || issue_from.due_date)
                   (issue_from.due_date || issue_from.start_date) + 1 + delay
                 end
               end
               def <=>(relation)
                 TYPES[self.relation_type][:order] <=> TYPES[relation.relation_type][:order]
               end
               private
               # Reverses the relation if needed so that it gets stored in the proper way
               def reverse_if_needed
                 if TYPES.has_key?(relation_type) && TYPES[relation_type][:reverse]
                   issue_tmp = issue_to
                   self.issue_to = issue_from
                   self.issue_from = issue_tmp
                   self.relation_type = TYPES[relation_type][:reverse]
                 end
               end
             end

test/functional/issue_relations_controller_test.rb +2 0

             require File.expand_path('../../test_helper', __FILE__)
             require 'issue_relations_controller'
             # Re-raise errors caught by the controller.
             class IssueRelationsController; def rescue_action(e) raise e end; end
             class IssueRelationsControllerTest < ActionController::TestCase
               fixtures :projects,
                        :users,
                        :roles,
                        :members,
                        :member_roles,
                        :issues,
                        :issue_statuses,
                        :issue_relations,
                        :enabled_modules,
                        :enumerations,
                        :trackers
               def setup
                 @controller = IssueRelationsController.new
                 @request    = ActionController::TestRequest.new
                 @response   = ActionController::TestResponse.new
                 User.current = nil
               end
               def test_new
                 assert_difference 'IssueRelation.count' do
                   @request.session[:user_id] = 3
                   post :new, :issue_id => 1,
                              :relation => {:issue_to_id => '2', :relation_type => 'relates', :delay => ''}
                 end
               end
               def test_new_xhr
                 assert_difference 'IssueRelation.count' do
                   @request.session[:user_id] = 3
                   xhr :post, :new,
                     :issue_id => 3,
                     :relation => {:issue_to_id => '1', :relation_type => 'relates', :delay => ''}
                   assert_select_rjs 'relations' do
                     assert_select 'table', 1
                     assert_select 'tr', 2 # relations
                   end
                 end
               end
               def test_new_should_accept_id_with_hash
                 assert_difference 'IssueRelation.count' do
                   @request.session[:user_id] = 3
                   post :new, :issue_id => 1,
                              :relation => {:issue_to_id => '#2', :relation_type => 'relates', :delay => ''}
                 end
               end
               def test_new_should_not_break_with_non_numerical_id
                 assert_no_difference 'IssueRelation.count' do
                   assert_nothing_raised do
                     @request.session[:user_id] = 3
                     post :new, :issue_id => 1,
                                :relation => {:issue_to_id => 'foo', :relation_type => 'relates', :delay => ''}
                   end
                 end
               end
               def test_should_create_relations_with_visible_issues_only
                 Setting.cross_project_issue_relations = '1'
                 assert_nil Issue.visible(User.find(3)).find_by_id(4)
                 assert_no_difference 'IssueRelation.count' do
                   @request.session[:user_id] = 3
                   post :new, :issue_id => 1,
                              :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
                   @request.session[:user_id] = 3
                   post :destroy, :id => '2', :issue_id => '3'
                 end
               end
               def test_destroy_xhr
                 IssueRelation.create!(:relation_type => IssueRelation::TYPE_RELATES) do |r|
                   r.issue_from_id = 3
                   r.issue_to_id = 1
                 end
                 assert_difference 'IssueRelation.count', -1 do
                   @request.session[:user_id] = 3
                   xhr :post, :destroy, :id => '2', :issue_id => '3'
                   assert_select_rjs 'relations' do
                     assert_select 'table', 1
                     assert_select 'tr', 1 # relation left
                   end
                 end
               end
             end

test/unit/issue_relation_test.rb +12 0

             # Redmine - project management software
             # Copyright (C) 2006-2009  Jean-Philippe Lang
             #
             # This program is free software; you can redistribute it and/or
             # modify it under the terms of the GNU General Public License
             # as published by the Free Software Foundation; either version 2
             # of the License, or (at your option) any later version.
             #
             # This program is distributed in the hope that it will be useful,
             # but WITHOUT ANY WARRANTY; without even the implied warranty of
             # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
             # GNU General Public License for more details.
             #
             # You should have received a copy of the GNU General Public License
             # along with this program; if not, write to the Free Software
             # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
             require File.expand_path('../../test_helper', __FILE__)
             class IssueRelationTest < ActiveSupport::TestCase
               fixtures :issue_relations, :issues
               def test_create
                 from = Issue.find(1)
                 to = Issue.find(2)
                 relation = IssueRelation.new :issue_from => from, :issue_to => to, :relation_type => IssueRelation::TYPE_PRECEDES
                 assert relation.save
                 relation.reload
                 assert_equal IssueRelation::TYPE_PRECEDES, relation.relation_type
                 assert_equal from, relation.issue_from
                 assert_equal to, relation.issue_to
               end
               def test_follows_relation_should_be_reversed
                 from = Issue.find(1)
                 to = Issue.find(2)
                 relation = IssueRelation.new :issue_from => from, :issue_to => to, :relation_type => IssueRelation::TYPE_FOLLOWS
                 assert relation.save
                 relation.reload
                 assert_equal IssueRelation::TYPE_PRECEDES, relation.relation_type
                 assert_equal to, relation.issue_from
                 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)
                 relation = IssueRelation.new :issue_from => from, :issue_to => to, :relation_type => IssueRelation::TYPE_FOLLOWS, :delay => 'xx'
                 assert !relation.save
                 assert_equal IssueRelation::TYPE_FOLLOWS, relation.relation_type
                 assert_equal from, relation.issue_from
                 assert_equal to, relation.issue_to
               end
               def test_relation_type_for
                 from = Issue.find(1)
                 to = Issue.find(2)
                 relation = IssueRelation.new :issue_from => from, :issue_to => to, :relation_type => IssueRelation::TYPE_PRECEDES
                 assert_equal IssueRelation::TYPE_PRECEDES, relation.relation_type_for(from)
                 assert_equal IssueRelation::TYPE_FOLLOWS, relation.relation_type_for(to)
               end
               def test_set_issue_to_dates_without_issue_to
                 r = IssueRelation.new(:issue_from => Issue.new(:start_date => Date.today), :relation_type => IssueRelation::TYPE_PRECEDES, :delay => 1)
                 assert_nil r.set_issue_to_dates
               end
               def test_set_issue_to_dates_without_issues
                 r = IssueRelation.new(:relation_type => IssueRelation::TYPE_PRECEDES, :delay => 1)
                 assert_nil r.set_issue_to_dates
               end
               def test_validates_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.new(:issue_from => Issue.find(3), :issue_to => Issue.find(1), :relation_type => IssueRelation::TYPE_PRECEDES)
                 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

General Comments 0

Write
Preview

You need to be logged in to leave comments. Login now

No TODOs yet

	Site-wide shortcuts
/	Use quick search box
g h	Goto home page
g g	Goto my private gists page
g G	Goto my public gists page
g 0-9	Goto bookmarked items from 0-9
n r	New repository page
n g	New gist page

	Repositories
g s	Goto summary page
g c	Goto changelog page
g f	Goto files page
g F	Goto files page with file search activated
g p	Goto pull requests page
g o	Goto repository settings
g O	Goto repository access permissions settings
t s	Toggle sidebar on some pages