From ead4776b82f838ee0630770d1852e8b02ac0f923 Mon Sep 17 00:00:00 2001
From: neumayr <neumayr@users.noreply.github.com>
Date: Thu, 9 Nov 2017 17:37:06 +0100
Subject: Fix field_error_proc wrap form select optgroup and divider option tag
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

### Summary

The [`:field_error_proc`](https://github.com/rails/rails/blob/master/actionview/lib/action_view/base.rb#L145) is responsible for decorating input tags that refer to attributes with errors. This default build-in rails feature wrap invalid form elements with additional markup: `<div class="field_with_errors">[…]</div>`.

* Fix for `field_error_proc` wraps form select `optgroup`
* Fix for `field_error_proc` wraps form select divider `option`
* Add tests for uncovered elements with errors

[Fixes #31088]

#### Test coverage
* `test_select_grouped_options_with_errors`
* `test_time_zone_select_with_priority_zones_and_errors`

#### Extend test coverage
* `test_collection_select_with_errors`
* `test_label_with_errors`
* `test_check_box_with_errors`
* `test_check_boxes_with_errors`
* `test_radio_button_with_errors`
* `test_radio_buttons_with_errors`
* `test_collection_check_boxes_with_errors`
* `test_collection_radio_buttons_with_errors`
---
 actionview/CHANGELOG.md                            |  6 ++
 .../lib/action_view/helpers/active_model_helper.rb |  8 ++-
 .../test/template/active_model_helper_test.rb      | 74 +++++++++++++++++++++-
 .../test/template/form_options_helper_test.rb      | 19 ++++++
 4 files changed, 104 insertions(+), 3 deletions(-)

(limited to 'actionview')

diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md
index e3227103d6..c700cb72ec 100644
--- a/actionview/CHANGELOG.md
+++ b/actionview/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Fix issues with `field_error_proc` wrapping `optgroup` and select divider `option`.
+
+    Fixes #31088
+
+    *Matthias Neumayr*
+
 *   Remove deprecated Erubis ERB handler.
 
     *Rafael Mendonça França*
diff --git a/actionview/lib/action_view/helpers/active_model_helper.rb b/actionview/lib/action_view/helpers/active_model_helper.rb
index f1ef715710..e41a95d2ce 100644
--- a/actionview/lib/action_view/helpers/active_model_helper.rb
+++ b/actionview/lib/action_view/helpers/active_model_helper.rb
@@ -17,8 +17,8 @@ module ActionView
         end
       end
 
-      def content_tag(*)
-        error_wrapping(super)
+      def content_tag(type, options, *)
+        select_markup_helper?(type) ? super : error_wrapping(super)
       end
 
       def tag(type, options, *)
@@ -43,6 +43,10 @@ module ActionView
           object.respond_to?(:errors) && object.errors.respond_to?(:[]) && error_message.present?
         end
 
+        def select_markup_helper?(type)
+          ["optgroup", "option"].include?(type)
+        end
+
         def tag_generate_errors?(options)
           options["type"] != "hidden"
         end
diff --git a/actionview/test/template/active_model_helper_test.rb b/actionview/test/template/active_model_helper_test.rb
index a929d8dc3d..36afed6dd6 100644
--- a/actionview/test/template/active_model_helper_test.rb
+++ b/actionview/test/template/active_model_helper_test.rb
@@ -6,7 +6,7 @@ class ActiveModelHelperTest < ActionView::TestCase
   tests ActionView::Helpers::ActiveModelHelper
 
   silence_warnings do
-    Post = Struct.new(:author_name, :body, :updated_at) do
+    Post = Struct.new(:author_name, :body, :category, :published, :updated_at) do
       include ActiveModel::Conversion
       include ActiveModel::Validations
 
@@ -22,10 +22,14 @@ class ActiveModelHelperTest < ActionView::TestCase
     @post = Post.new
     @post.errors[:author_name] << "can't be empty"
     @post.errors[:body] << "foo"
+    @post.errors[:category] << "must exist"
+    @post.errors[:published] << "must be accepted"
     @post.errors[:updated_at] << "bar"
 
     @post.author_name = ""
     @post.body        = "Back to the hill and over it again!"
+    @post.category    = "rails"
+    @post.published   = false
     @post.updated_at  = Date.new(2004, 6, 15)
   end
 
@@ -56,6 +60,25 @@ class ActiveModelHelperTest < ActionView::TestCase
     assert_dom_equal(expected_dom, select("post", "author_name", [:a, :b], prompt: "Choose one..."))
   end
 
+  def test_select_grouped_options_with_errors
+    grouped_options = [
+      ["A", [["A1"], ["A2"]]],
+      ["B", [["B1"], ["B2"]]],
+    ]
+
+    assert_dom_equal(
+      %(<div class="field_with_errors"><select name="post[category]" id="post_category"><optgroup label="A"><option value="A1">A1</option>\n<option value="A2">A2</option></optgroup><optgroup label="B"><option value="B1">B1</option>\n<option value="B2">B2</option></optgroup></select></div>),
+      select("post", "category", grouped_options)
+    )
+  end
+
+  def test_collection_select_with_errors
+    assert_dom_equal(
+      %(<div class="field_with_errors"><select name="post[author_name]" id="post_author_name"><option value="a">a</option>\n<option value="b">b</option></select></div>),
+      collection_select("post", "author_name", [:a, :b], :to_s, :to_s)
+    )
+  end
+
   def test_date_select_with_errors
     assert_dom_equal(
       %(<div class="field_with_errors"><select id="post_updated_at_1i" name="post[updated_at(1i)]">\n<option selected="selected" value="2004">2004</option>\n<option value="2005">2005</option>\n</select>\n<input id="post_updated_at_2i" name="post[updated_at(2i)]" type="hidden" value="6" />\n<input id="post_updated_at_3i" name="post[updated_at(3i)]" type="hidden" value="1" />\n</div>),
@@ -77,6 +100,55 @@ class ActiveModelHelperTest < ActionView::TestCase
     )
   end
 
+  def test_label_with_errors
+    assert_dom_equal(
+      %(<div class="field_with_errors"><label for="post_body">Body</label></div>),
+      label("post", "body")
+    )
+  end
+
+  def test_check_box_with_errors
+    assert_dom_equal(
+      %(<input name="post[published]" type="hidden" value="0" /><div class="field_with_errors"><input type="checkbox" value="1" name="post[published]" id="post_published" /></div>),
+      check_box("post", "published")
+    )
+  end
+
+  def test_check_boxes_with_errors
+    assert_dom_equal(
+      %(<input name="post[published]" type="hidden" value="0" /><div class="field_with_errors"><input type="checkbox" value="1" name="post[published]" id="post_published" /></div><input name="post[published]" type="hidden" value="0" /><div class="field_with_errors"><input type="checkbox" value="1" name="post[published]" id="post_published" /></div>),
+      check_box("post", "published") + check_box("post", "published")
+    )
+  end
+
+  def test_radio_button_with_errors
+    assert_dom_equal(
+      %(<div class="field_with_errors"><input type="radio" value="rails" checked="checked" name="post[category]" id="post_category_rails" /></div>),
+      radio_button("post", "category", "rails")
+    )
+  end
+
+  def test_radio_buttons_with_errors
+    assert_dom_equal(
+      %(<div class="field_with_errors"><input type="radio" value="rails" checked="checked" name="post[category]" id="post_category_rails" /></div><div class="field_with_errors"><input type="radio" value="java" name="post[category]" id="post_category_java" /></div>),
+      radio_button("post", "category", "rails") + radio_button("post", "category", "java")
+    )
+  end
+
+  def test_collection_check_boxes_with_errors
+    assert_dom_equal(
+      %(<input type="hidden" name="post[category][]" value="" /><div class="field_with_errors"><input type="checkbox" value="ruby" name="post[category][]" id="post_category_ruby" /></div><label for="post_category_ruby">ruby</label><div class="field_with_errors"><input type="checkbox" value="java" name="post[category][]" id="post_category_java" /></div><label for="post_category_java">java</label>),
+      collection_check_boxes("post", "category", [:ruby, :java], :to_s, :to_s)
+    )
+  end
+
+  def test_collection_radio_buttons_with_errors
+    assert_dom_equal(
+      %(<input type="hidden" name="post[category]" value="" /><div class="field_with_errors"><input type="radio" value="ruby" name="post[category]" id="post_category_ruby" /></div><label for="post_category_ruby">ruby</label><div class="field_with_errors"><input type="radio" value="java" name="post[category]" id="post_category_java" /></div><label for="post_category_java">java</label>),
+      collection_radio_buttons("post", "category", [:ruby, :java], :to_s, :to_s)
+    )
+  end
+
   def test_hidden_field_does_not_render_errors
     assert_dom_equal(
       %(<input id="post_author_name" name="post[author_name]" type="hidden" value="" />),
diff --git a/actionview/test/template/form_options_helper_test.rb b/actionview/test/template/form_options_helper_test.rb
index a66db2f3dc..f0eed1e290 100644
--- a/actionview/test/template/form_options_helper_test.rb
+++ b/actionview/test/template/form_options_helper_test.rb
@@ -1251,6 +1251,25 @@ class FormOptionsHelperTest < ActionView::TestCase
                  html
   end
 
+  def test_time_zone_select_with_priority_zones_and_errors
+    @firm = Firm.new("D")
+    @firm.extend ActiveModel::Validations
+    @firm.errors[:time_zone] << "invalid"
+    zones = [ ActiveSupport::TimeZone.new("A"), ActiveSupport::TimeZone.new("D") ]
+    html = time_zone_select("firm", "time_zone", zones)
+    assert_dom_equal "<div class=\"field_with_errors\">" \
+                 "<select id=\"firm_time_zone\" name=\"firm[time_zone]\">" \
+                 "<option value=\"A\">A</option>\n" \
+                 "<option value=\"D\" selected=\"selected\">D</option>" \
+                 "<option value=\"\" disabled=\"disabled\">-------------</option>\n" \
+                 "<option value=\"B\">B</option>\n" \
+                 "<option value=\"C\">C</option>\n" \
+                 "<option value=\"E\">E</option>" \
+                 "</select>" \
+                 "</div>",
+                 html
+  end
+
   def test_time_zone_select_with_default_time_zone_and_nil_value
     @firm = Firm.new()
     @firm.time_zone = nil
-- 
cgit v1.2.3