validates_date seems to break form helper #52

Closed
rbhitchcock opened this Issue Sep 1, 2011 · 24 comments

Comments

Projects
None yet
5 participants

If in my model I have something like

class Person
validates_date :birthday
end

And in view code something like

<%= form_for @person do |f| %>
<%= f.text_field :birthday %>:
<% end %>

The text field is not autopopulated with the existing value for :birthday. However, the following does work:

<%= form_for @person do |f| %>
<%= text_field_tag 'person[birthday]', @person.birthday %>:
<% end %>

Owner

adzap commented Sep 2, 2011

If you don't validate the date does the text_field version work?

adzap closed this Sep 2, 2011

adzap reopened this Sep 2, 2011

Yes. Removing the date validation results in the text_field being populated
with the existing value, if it exists.
On Sep 2, 2011 5:09 AM, "adzap" <
reply@reply.github.com>
wrote:

If you don't validate the date does the text_field version work?

Reply to this email directly or view it on GitHub:

#52 (comment)

Owner

adzap commented Sep 19, 2011

A failing test for this would be great help.

+1 for this issue, not really sure how to test this but I'll give it a stab in the next day or two.

Owner

adzap commented Sep 20, 2011

I have had a play and have seen the bug. It seems to only occur when the config.extend_orms = [:active_record] is set. Commenting it works ok.

Owner

adzap commented Sep 20, 2011

I've found the issue. It's to do with the before_type_cast implementation not returning the initial attribute value. Will work on a fix.

On 20/09/2011, at 9:12 AM, Thomasreply@reply.github.com wrote:

  • 1 for this issue, not really sure how to test this but I'll give it a stab in the next day or two.

Reply to this email directly or view it on GitHub:
#52 (comment)

@adzap adzap added a commit that referenced this issue Sep 21, 2011

@adzap adzap Fix before_type_cast for non-dirty attributes (issue #52).
Only use before_type_cast implementation if earlier than 3.1.0 which has
it's own working version for date/time fields now.
86b7bc4
Owner

adzap commented Sep 21, 2011

v3.0.7 should fix this.

adzap closed this Sep 21, 2011

pkmiec commented Sep 22, 2011

I'm using Rails 3.1 and I don't see how this fix helps. Here is an example,

model.my_date = 'foo'
model.my_date                    # --> nil
model.my_date_before_type_cast   # --> also nil!

Looking at the code, it seems validates_timeliness defines my_date= and stores the original value in @timelines_cache. In validates_timeliness 3.0.6, this value was then returned by validates timeliness' version of _before_type_cast. In 3.0.7, Rails' version of _before_type_cast is used which doesn't know anything about @timeliness_cache ... so it obviously returns nil.

Owner

adzap commented Sep 22, 2011

Are you saying you are seeing the behaviour of your example using the current version of the plugin with Rails 3.1.0?

pkmiec commented Sep 22, 2011

Yes. I am updating an app to from Rails 3.0 to Rails 3.1 and using validates_timeliness 3.0.7.

The problem occurs with 3.0.6 as well which is puzzling because I thought 3.0.6 defined its own before_type_cast method. Perhaps I'm missing something with how before_type_cast works in Rails 3.1.

Owner

adzap commented Sep 22, 2011

Are you using ActiveRecord?

The before_type_cast method is only defined by the plugin if the a true value is passed to define_timeliness_before_type_cast method. The AR shim checks the version and passes a false if AR version is 3.1.0 or above. Therefore default AR before_type_cast implementation should be being used.

This issue still seems to exist when enabling the parser_plugin in rails 3.1.3:

ValidatesTimeliness.setup do |config|
  config.use_plugin_parser = true
end
model.my_date = 'foo'
model.my_date                  # --> nil
model.my_date_before_type_cast # --> nil!

When I turn off the parser_plugin it works as expected:

model.my_date = 'foo'
model.my_date                  # --> nil
model.my_date_before_type_cast # --> "foo"
Owner

adzap commented Jan 24, 2012

Ok, I see the problem. I will work on a fix.

pkmiec commented Jan 24, 2012

Thanks!

Thanks, good luck

adzap reopened this Jan 26, 2012

Owner

adzap commented Jan 26, 2012

I've pushed a fix which is a bit messy but it should work. Can you verify please. No gem update yet.

Unfortunately it doesn't work here, the whole parser-plugin seems to be disabled now:

version 3.0.8:

model.my_date = 'foo'
=> "foo"
model.valid?
=> false
model.errors[:my_date]
=> ["is not a valid date"]
ValidatesTimeliness.use_plugin_parser
=> true

New version from github:

model.my_date = 'foo'
=> "foo"
model.valid?
=> false # other errors in the model
model.errors[:my_date]
=> []
ValidatesTimeliness.use_plugin_parser
=> true
Owner

adzap commented Feb 1, 2012

This would be a symptom of the attribute value caching not working, rather than the parser. But nonetheless, it is a problem I will look into.

Owner

adzap commented Mar 26, 2012

I've released a new gem version. Please verify if it fixes this issue.

Hi,
I'm afraid this still doesn't resolve the issue. I'm using rails 3.2.0 and validates_timeliness 3.0.9. The output is the same again.

model.my_date = 'foo'
=> "foo"
model.valid?
=> false # other errors in the model
model.errors[:my_date]
=> []
ValidatesTimeliness.use_plugin_parser
=> true

In my model i have the line:
validates_date :my_date, allow_blank: true

If I change line 7 of active_record.rb to true. Everything seems to work as expected

# In active_record.rb of validates_timeliness 3.0.9
def self.use_plugin_cache?
  true
end
model.my_date = 'foo'
=> "foo"
model.valid?
=> false
model.errors[:my_date]
=> ["is not a valid date"]
model.my_date
=> nil
model.my_date_before_type_cast
=> "foo"
ValidatesTimeliness.use_plugin_parser
=> true

I hope this helps with the issue. Good luck

Owner

adzap commented Mar 27, 2012

Yes, it does thanks.

On 27/03/2012, at 8:08 PM, Stephan van Eijkelenburgreply@reply.github.com wrote:

Hi,
I'm afraid this still doesn't resolve the issue. I'm using rails 3.2.0 and validates_timeliness 3.0.9. The output is the same again.

model.my_date = 'foo'
=> "foo"
model.valid?
=> false # other errors in the model
model.errors[:my_date]
=> []
ValidatesTimeliness.use_plugin_parser
=> true

In my model i have the line:
validates_date :my_date, allow_blank: true

If I change line 7 of active_record.rb to true. Everything seems to work as expected

# In active_record.rb of validates_timeliness 3.0.9
def self.use_plugin_cache?
 true
end
model.my_date = 'foo'
=> "foo"
model.valid?
=> false
model.errors[:my_date]
=> ["is not a valid date"]
model.my_date
=> nil
model.my_date_before_type_cast
=> "foo"
ValidatesTimeliness.use_plugin_parser
=> true

I hope this helps with the issue. Good luck


Reply to this email directly or view it on GitHub:
#52 (comment)

Owner

adzap commented Mar 29, 2012

I have pushed a new release to fix this. Please give it a try.

Hi,
The fix seems to work for dates, but there are still some problems with times.

# entering an invalid time works as expected
model.my_time = '12:66'
=> "12:66"
model.my_time
=> nil
model.valid?
=> false
model.errors[:my_time]
=> ["is not a valid time"]

# entering an other string; doesn't work properly
model.my_time = 'foo'
=> "foo"
model.my_time
=> 2000-01-01 00:00:00 UTC
model.valid? 
=> false # other errors in the model
model.errors[:my_time]
=> []
Owner

adzap commented Aug 10, 2012

I found the issue for the invalid time being returned as a time object. It is an ActiveRecord bug in dummy_string_to_time. I will close this issue and open a new one, to see I can do a work around. Also will fix the bug in AR hopefully.

adzap closed this Aug 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment