Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Time select does not work with time field in the database #634

Closed
lukaselmer opened this Issue Jul 25, 2011 · 7 comments

Comments

Projects
None yet
2 participants
f.input :pickup_at, :as => :time

Produces an error if the field :pickup_at is a time field in the database on ActiveRecord::Base#save.

Error

ActiveRecord::MultiparameterAssignmentErrors in PickupsController#update
1 error(s) on assignment of multiparameter attributes

Params

{"utf8"=>"",
 "_method"=>"put",
 "authenticity_token"=>"CcN5gSGZgZ4wNibQCSH5iwi9dBxW3+ERO7cQE0EijoE=",
 "pickup"=>{"pickup_at(4i)"=>"8",
 "pickup_at(5i)"=>"11",
 "description"=>"Lorem ipsum ipsum ipsum ipsum ipsum ipsum"},
 "id"=>"7"}

Schema

create_table "pickups", :force => true do |t|
    t.time     "pickup_at"
    t.text     "description"
    t.datetime "created_at"
    t.datetime "updated_at"
end
Owner

justinfrench commented Aug 15, 2011

@lukaselmer could you do some testing for me to help narrow this down? :as => :time is based off what Rails' own time_select does when the :ignore_date option is used. Could you confirm that Rails mass assignment works with the following?

f.time_select(:pickup_at, :ignore_date => true)

If that fails, could you also try this?

f.time_select(:pickup_at)

If the same "bug" exists in Rails, I'm inclined to keep this behaviour consistent. If the bug is only with Formtastic, would love to see a params hash that does work, to compare to the one Formtastic is generating (my tests look the same as Rails' tests at first glance).

Would love to get this sorted out for 2.0, would love your help.

Ok, no problem!

f.time_select(:pickup_at, :ignore_date => true)

fails while

f.time_select(:pickup_at)

works just fine. I tried it with :datetime, :timestamp and :time on SQLite3. Notice that the error occurs not during the generation of the view but when the controller tries to save the model.

I think the :ignore_date => true option is more like a special case, therefore the option is set to false by default. Maybe the default :ignore_date => false could be used in Formtastic with time fields? What do you think?

My ENV looks like this:

Ruby version 1.9.3 (x86_64-linux)
RubyGems version 1.8.6
Rack version 1.3
Rails version 3.2.0.beta
JavaScript Runtime Node.js (V8)
Active Record version 3.2.0.beta
Action Pack version 3.2.0.beta
Active Resource version 3.2.0.beta
Action Mailer version 3.2.0.beta
Active Support version 3.2.0.beta
Middleware
ActionDispatch::Static
Rack::Lock
#ActiveSupport::Cache::Strategy::LocalCache::Middleware:0x00000003840348
Rack::Runtime
Rack::MethodOverride
Rails::Rack::Logger
ActionDispatch::ShowExceptions
ActionDispatch::RemoteIp
ActionDispatch::Reloader
ActionDispatch::Callbacks
ActiveRecord::ConnectionAdapters::ConnectionManagement
ActiveRecord::QueryCache
ActionDispatch::Cookies
ActionDispatch::Session::CookieStore
ActionDispatch::Flash
ActionDispatch::ParamsParser
ActionDispatch::Head
Rack::ConditionalGet
Rack::ETag
ActionDispatch::BestStandardsSupport
Application root /home/luke/workspace_netbeans/formtastic_time_test_refs_634
Environment development
Database adapter sqlite3
Database schema version 20110815104945

Owner

justinfrench commented Aug 16, 2011

Thanks for that, big help.

Looking at the docs for time_select: "This method will also generate 3 input hidden tags, for the actual year, month and day unless the option :ignore_date is set to true."

Looking at our own specs, it looks like I was trying to replicate this. There's test coverage for the edge case (:ignore_date => true), but not for the default case (effectively :ignore_date => false). As soon as I added coverage for this case, the bug was exposed.

Who knows if it's been like that forever, or if it's a recent regression, but now I have a test case to fix. Many thanks.

Owner

justinfrench commented Aug 16, 2011

Looks like a regression during the refactor.

Great, thx! :D

Owner

justinfrench commented Aug 16, 2011

@lukaselmer can you please bundle from master and test this patch. Please re-open with any issues. Would love to get a new RC out in the next day or so.

Works like a charm :D

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