This project is archived and is in readonly mode.
Validates_acceptance_of is not respecting database field
Reported by Si | April 22nd, 2010 @ 10:27 PM | in 3.0.2
In AcceptanceValidator#setup (/lib/active_model/validations/acceptance.rb), the class instance methods are inspected, to see if a writer method exists for each validated attribute. If not, an accessor for that attribute is created:
new_attributes = attributes.reject { |name| klass.instance_methods.map(&:to_s).include?("#{name}=") }
klass.send(:attr_accessor, *new_attributes)
However, klass.instance_methods does not pick up on my database backed fields. This leads to creating an overridden method that will not actually write the underlying attribute.
Comments and changes to this ticket
-
Si April 27th, 2010 @ 10:58 AM
Sure!
I wasn't set up for tests in the activemodel area before, primarily due to a MySQL dependency. Here's a patch to use database backing for the terms_of_service field used in acceptance validation. The Eula test continues to test a non db-backed-field.
-
Ryan Bigg April 27th, 2010 @ 11:51 AM
- State changed from new to needs-more-info
-
James Sadler April 27th, 2010 @ 11:59 AM
Si,
The diff you attached is not sufficient by itself to constitute a test case. All it does is add a column to the schema definition but makes no assertions, about what the expected behaviour should be. If you could provide a fragment of code that demonstrates the breakage and/or explain the issue in more detail, that'd be great.
James.
-
Si April 27th, 2010 @ 03:20 PM
Hi James,
I'm not a fan of the style employed by AcceptanceValidationTest. The tests are named according to attribute tested (e.g. test_eula), instead of explicitly stating what is being tested. It requires poking around in the model and schema to discover exactly what terms_of_service and eula are. The expected behaviour remains exactly the same however (test terms_of_service acceptance for example), and yet breaks for me when that field is a regular db attribute.
So my diff is merely going along with the voodoo right now. With that patch in place, everything should continue to work, with better code coverage, but it doesn't. I certainly think the test methods should be renamed, bringing some clarity about the nature of the underlying attribute, and some tests should be duplicated too for better coverage, but sadly I'm not in the position to right now. I'm seeing some very funky setup / test / teardown interplay, and changing method names throws everything. Being an RSpec user, perhaps I'm missing something obvious.
Anyway, thanks for looking into this.
-
Santiago Pastorino April 28th, 2010 @ 07:07 PM
- Milestone cleared.
- State changed from needs-more-info to verified
- Assigned user set to Santiago Pastorino
We have all the needed info, i'm fixing this i have a failing test now here.
http://github.com/spastorino/rails/commit/d2e24a242c91161ec4cb9e7f6... -
Repository April 28th, 2010 @ 10:43 PM
- State changed from verified to committed
(from [ce48b3103acd2f58931aa42b93073592b291114e]) Makes validates_acceptance_of to not override database fields [#4460 state:committed]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/ce48b3103acd2f58931aa42b930735... -
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to 3.0.2
- Importance changed from to Medium
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Tags
Referenced by
- 4460 Validates_acceptance_of is not respecting database field (from [ce48b3103acd2f58931aa42b93073592b291114e]) Makes v...