From 2b797fa82fdd18ec2b3e1c67642278136be68184 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Lang <jp_lang@yahoo.fr>
Date: Fri, 12 Oct 2012 13:40:41 +0000
Subject: [PATCH] Fixed: No validation errors when entering an invalid "Parent
 task" (#11979).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@10615 e93f8b46-1217-0410-a6f0-8f06a7374b81
---
 app/models/issue.rb                       | 18 +++++++++-----
 test/functional/issues_controller_test.rb | 30 ++++++++++++++++++-----
 test/unit/issue_test.rb                   | 14 +++++++++++
 3 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/app/models/issue.rb b/app/models/issue.rb
index 520d2c3dc..13a21e07a 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -416,7 +416,9 @@ class Issue < ActiveRecord::Base
     end
 
     if attrs['parent_issue_id'].present?
-      attrs.delete('parent_issue_id') unless Issue.visible(user).exists?(attrs['parent_issue_id'].to_i)
+      unless Issue.visible(user).exists?(attrs['parent_issue_id'].to_i)
+        @invalid_parent_issue_id = attrs.delete('parent_issue_id')
+      end
     end
 
     if attrs['custom_field_values'].present?
@@ -550,7 +552,9 @@ class Issue < ActiveRecord::Base
     end
 
     # Checks parent issue assignment
-    if @parent_issue
+    if @invalid_parent_issue_id.present?
+      errors.add :parent_issue_id, :invalid
+    elsif @parent_issue
       if !valid_parent_project?(@parent_issue)
         errors.add :parent_issue_id, :invalid
       elsif !new_record?
@@ -947,17 +951,19 @@ class Issue < ActiveRecord::Base
   end
 
   def parent_issue_id=(arg)
-    parent_issue_id = arg.blank? ? nil : arg.to_i
-    if parent_issue_id && @parent_issue = Issue.find_by_id(parent_issue_id)
+    s = arg.to_s.strip.presence
+    if s && (m = s.match(%r{\A#?(\d+)\z})) && (@parent_issue = Issue.find_by_id(m[1]))
       @parent_issue.id
     else
       @parent_issue = nil
-      nil
+      @invalid_parent_issue_id = arg
     end
   end
 
   def parent_issue_id
-    if instance_variable_defined? :@parent_issue
+    if @invalid_parent_issue_id
+      @invalid_parent_issue_id
+    elsif instance_variable_defined? :@parent_issue
       @parent_issue.nil? ? nil : @parent_issue.id
     else
       parent_id
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 01eb26c96..b897c472e 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -1945,24 +1945,42 @@ class IssuesControllerTest < ActionController::TestCase
                  :issue => {:tracker_id => 1,
                             :subject => 'This is a child issue',
                             :parent_issue_id => 2}
+
+      assert_response 302
     end
     issue = Issue.find_by_subject('This is a child issue')
     assert_not_nil issue
     assert_equal Issue.find(2), issue.parent
   end
 
-  def test_post_create_subissue_with_non_numeric_parent_id
+  def test_post_create_subissue_with_non_visible_parent_id_should_not_validate
     @request.session[:user_id] = 2
 
-    assert_difference 'Issue.count' do
+    assert_no_difference 'Issue.count' do
       post :create, :project_id => 1,
                  :issue => {:tracker_id => 1,
                             :subject => 'This is a child issue',
-                            :parent_issue_id => 'ABC'}
+                            :parent_issue_id => '4'}
+
+      assert_response :success
+      assert_select 'input[name=?][value=?]', 'issue[parent_issue_id]', '4'
+      assert_error_tag :content => /Parent task is invalid/i
+    end
+  end
+
+  def test_post_create_subissue_with_non_numeric_parent_id_should_not_validate
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Issue.count' do
+      post :create, :project_id => 1,
+                 :issue => {:tracker_id => 1,
+                            :subject => 'This is a child issue',
+                            :parent_issue_id => '01ABC'}
+
+      assert_response :success
+      assert_select 'input[name=?][value=?]', 'issue[parent_issue_id]', '01ABC'
+      assert_error_tag :content => /Parent task is invalid/i
     end
-    issue = Issue.find_by_subject('This is a child issue')
-    assert_not_nil issue
-    assert_nil issue.parent
   end
 
   def test_post_create_private
diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb
index 0747f5c84..365ecad68 100644
--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -92,6 +92,20 @@ class IssueTest < ActiveSupport::TestCase
     end
   end
 
+  def test_create_with_parent_issue_id
+    issue = Issue.new(:project_id => 1, :tracker_id => 1, :author_id => 1, :subject => 'Group assignment', :parent_issue_id => 1)
+    assert_save issue
+    assert_equal 1, issue.parent_issue_id
+    assert_equal Issue.find(1), issue.parent
+  end
+
+  def test_create_with_invalid_parent_issue_id
+    issue = Issue.new(:project_id => 1, :tracker_id => 1, :author_id => 1, :subject => 'Group assignment', :parent_issue_id => '01ABC')
+    assert !issue.save
+    assert_equal '01ABC', issue.parent_issue_id
+    assert_include 'Parent task is invalid', issue.errors.full_messages
+  end
+
   def assert_visibility_match(user, issues)
     assert_equal issues.collect(&:id).sort, Issue.all.select {|issue| issue.visible?(user)}.collect(&:id).sort
   end