From f06054abe6da7104e8e73f37eaea44d7b70d3852 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 29 Apr 2010 13:40:54 -0400 Subject: [PATCH] Back porting fix for #4432 for rails 2-3-stable branch fix form builder and form helpers inconsistencies [#4432 state:resolved] * datetime_select and select_datetime should be consistent as much as possible * date_select and select_date should be consistent as much as possible * time_select and select_time should be consistent as much as possible --- actionpack/CHANGELOG | 2 + actionpack/lib/action_view/helpers/date_helper.rb | 79 +++++++--------- actionpack/test/template/date_helper_test.rb | 99 ++++++++++++++++----- 3 files changed, 114 insertions(+), 66 deletions(-) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index ed1e24d..9787304 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *2.3.6 (unreleased)* +* Fixed inconsistencies in form builder and view helpers #4432 [Neeraj Singh] + * JSON: set Base.include_root_in_json = true to include a root value in the JSON: {"post": {"title": ...}}. Mirrors the Active Record option. #2584 [Matthew Moore, Joe Martinez, Elad Meidar, Santiago Pastorino] * Ruby 1.9: ERB template encoding using a magic comment at the top of the file. [Jeremy Kemper] diff --git a/actionpack/lib/action_view/helpers/date_helper.rb b/actionpack/lib/action_view/helpers/date_helper.rb index f556488..1cd6517 100644 --- a/actionpack/lib/action_view/helpers/date_helper.rb +++ b/actionpack/lib/action_view/helpers/date_helper.rb @@ -589,56 +589,50 @@ module ActionView @options = options.dup @html_options = html_options.dup @datetime = datetime + @options[:datetime_separator] ||= ' — ' + @options[:time_separator] ||= ' : ' end def select_datetime - # TODO: Remove tag conditional - # Ideally we could just join select_date and select_date for the tag case - if @options[:tag] && @options[:ignore_date] - select_time - elsif @options[:tag] - order = date_order.dup - order -= [:hour, :minute, :second] - - @options[:discard_year] ||= true unless order.include?(:year) - @options[:discard_month] ||= true unless order.include?(:month) - @options[:discard_day] ||= true if @options[:discard_month] || !order.include?(:day) - @options[:discard_minute] ||= true if @options[:discard_hour] - @options[:discard_second] ||= true unless @options[:include_seconds] && !@options[:discard_minute] - - # If the day is hidden and the month is visible, the day should be set to the 1st so all month choices are - # valid (otherwise it could be 31 and february wouldn't be a valid date) - if @datetime && @options[:discard_day] && !@options[:discard_month] - @datetime = @datetime.change(:day => 1) - end + order = date_order.dup + order -= [:hour, :minute, :second] + @options[:discard_year] ||= true unless order.include?(:year) + @options[:discard_month] ||= true unless order.include?(:month) + @options[:discard_day] ||= true if @options[:discard_month] || !order.include?(:day) + @options[:discard_minute] ||= true if @options[:discard_hour] + @options[:discard_second] ||= true unless @options[:include_seconds] && !@options[:discard_minute] + + # If the day is hidden and the month is visible, the day should be set to the 1st so all month choices are + # valid (otherwise it could be 31 and february wouldn't be a valid date) + if @datetime && @options[:discard_day] && !@options[:discard_month] + @datetime = @datetime.change(:day => 1) + end + if @options[:ignore_date] + select_time + else [:day, :month, :year].each { |o| order.unshift(o) unless order.include?(o) } order += [:hour, :minute, :second] unless @options[:discard_hour] build_selects_from_types(order) - else - "#{select_date}#{@options[:datetime_separator]}#{select_time}".html_safe end end def select_date order = date_order.dup - # TODO: Remove tag conditional - if @options[:tag] - @options[:discard_hour] = true - @options[:discard_minute] = true - @options[:discard_second] = true + @options[:discard_hour] = true + @options[:discard_minute] = true + @options[:discard_second] = true - @options[:discard_year] ||= true unless order.include?(:year) - @options[:discard_month] ||= true unless order.include?(:month) - @options[:discard_day] ||= true if @options[:discard_month] || !order.include?(:day) + @options[:discard_year] ||= true unless order.include?(:year) + @options[:discard_month] ||= true unless order.include?(:month) + @options[:discard_day] ||= true if @options[:discard_month] || !order.include?(:day) - # If the day is hidden and the month is visible, the day should be set to the 1st so all month choices are - # valid (otherwise it could be 31 and february wouldn't be a valid date) - if @datetime && @options[:discard_day] && !@options[:discard_month] - @datetime = @datetime.change(:day => 1) - end + # If the day is hidden and the month is visible, the day should be set to the 1st so all month choices are + # valid (otherwise it could be 31 and february wouldn't be a valid date) + if @datetime && @options[:discard_day] && !@options[:discard_month] + @datetime = @datetime.change(:day => 1) end [:day, :month, :year].each { |o| order.unshift(o) unless order.include?(o) } @@ -649,15 +643,12 @@ module ActionView def select_time order = [] - # TODO: Remove tag conditional - if @options[:tag] - @options[:discard_month] = true - @options[:discard_year] = true - @options[:discard_day] = true - @options[:discard_second] ||= true unless @options[:include_seconds] + @options[:discard_month] = true + @options[:discard_year] = true + @options[:discard_day] = true + @options[:discard_second] ||= true unless @options[:include_seconds] - order += [:year, :month, :day] unless @options[:ignore_date] - end + order += [:year, :month, :day] unless @options[:ignore_date] order += [:hour, :minute] order << :second if @options[:include_seconds] @@ -937,10 +928,8 @@ module ActionView options[:include_position] = true options[:prefix] ||= @object_name options[:index] = @auto_index if defined?(@auto_index) && @auto_index && !options.has_key?(:index) - options[:datetime_separator] ||= ' — ' - options[:time_separator] ||= ' : ' - DateTimeSelector.new(datetime, options.merge(:tag => true), html_options) + DateTimeSelector.new(datetime, options, html_options) end def default_datetime(options) diff --git a/actionpack/test/template/date_helper_test.rb b/actionpack/test/template/date_helper_test.rb index 9a2d490..1013cee 100644 --- a/actionpack/test/template/date_helper_test.rb +++ b/actionpack/test/template/date_helper_test.rb @@ -669,18 +669,10 @@ class DateHelperTest < ActionView::TestCase end def test_select_date_with_incomplete_order - # NOTE: modified this test because of minimal API change - expected = %(\n" - - expected << %(\n" - - expected << %(\n" + # Since the order is incomplete nothing will be shown + expected = %(\n) + expected << %(\n) + expected << %(\n) assert_dom_equal expected, select_date(Time.mktime(2003, 8, 16), :start_year => 2003, :end_year => 2005, :prefix => "date[first]", :order => [:day]) end @@ -903,10 +895,14 @@ class DateHelperTest < ActionView::TestCase expected << %(\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n) expected << "\n" + expected << " — " + expected << %(\n" + expected << " : " + expected << %(\n" @@ -955,10 +951,14 @@ class DateHelperTest < ActionView::TestCase expected << %(\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n) expected << "\n" + expected << " — " + expected << %(\n" + expected << " : " + expected << %(\n" @@ -979,10 +979,14 @@ class DateHelperTest < ActionView::TestCase expected << %(\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n) expected << "\n" + expected << " — " + expected << %(\n" + expected << " : " + expected << %(\n" @@ -1039,10 +1043,14 @@ class DateHelperTest < ActionView::TestCase expected << %(\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n) expected << "\n" + expected << " — " + expected << %(\n" + expected << " : " + expected << %(\n" @@ -1052,7 +1060,6 @@ class DateHelperTest < ActionView::TestCase end def test_select_datetime_with_custom_prompt - expected = %(\n" @@ -1065,10 +1072,14 @@ class DateHelperTest < ActionView::TestCase expected << %(\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n) expected << "\n" + expected << " — " + expected << %(\n" + expected << " : " + expected << %(\n" @@ -1078,10 +1089,16 @@ class DateHelperTest < ActionView::TestCase end def test_select_time - expected = %(\n) + expected << %(\n) + expected << %(\n) + + expected << %(\n" + expected << " : " + expected << %(\n" @@ -1091,7 +1108,10 @@ class DateHelperTest < ActionView::TestCase end def test_select_time_with_separator - expected = %(\n) + expected << %(\n) + expected << %(\n) + expected << %(\n" @@ -1106,14 +1126,22 @@ class DateHelperTest < ActionView::TestCase end def test_select_time_with_seconds - expected = %(\n) + expected << %(\n) + expected << %(\n) + + expected << %(\n" + expected << ' : ' + expected << %(\n" + expected << ' : ' + expected << %(\n" @@ -1122,7 +1150,11 @@ class DateHelperTest < ActionView::TestCase end def test_select_time_with_seconds_and_separator - expected = %(\n) + expected << %(\n) + expected << %(\n) + + expected << %(\n" @@ -1142,10 +1174,16 @@ class DateHelperTest < ActionView::TestCase end def test_select_time_with_html_options - expected = %(\n) + expected << %(\n) + expected << %(\n) + + expected << %(\n" + expected << " : " + expected << %(\n" @@ -1159,14 +1197,22 @@ class DateHelperTest < ActionView::TestCase end def test_select_time_with_default_prompt - expected = %(\n) + expected << %(\n) + expected << %(\n) + + expected << %(\n" + + expected << " : " expected << %(\n" + expected << " : " + expected << %(\n" @@ -1175,15 +1221,22 @@ class DateHelperTest < ActionView::TestCase end def test_select_time_with_custom_prompt - - expected = %(\n) + expected << %(\n) + expected << %(\n) + + expected << %(\n" + expected << " : " + expected << %(\n" + expected << " : " + expected << %(\n" @@ -2006,10 +2059,14 @@ class DateHelperTest < ActionView::TestCase expected << %(\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n) expected << "\n" + expected << " — " + expected << %(\n" + expected << " : " + expected << %(\n" -- 1.6.5.2