Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Validates_acceptance_of is not respecting database field

#4460

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.

Reported by Si · April 22nd, 2010 @ 10:27 PM

State: committed
Milestone: 3.0.2
Assigned to: Santiago Pastorino Santiago Pastorino
Importance: Medium

Activity

  1. Santiago Pastorino
    Santiago Pastorino

    Can you provide at least a test case?.
    Thanks.

    April 26th, 2010 @ 04:44 AM

  2. Si
    Si

    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.

    April 27th, 2010 @ 10:58 AM

  3. Ryan Bigg
    Ryan Bigg
    • State changed from new to needs-more-info

    April 27th, 2010 @ 11:51 AM

  4. James Sadler
    James Sadler

    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.

    April 27th, 2010 @ 11:59 AM

  5. Si
    Si

    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.

    April 27th, 2010 @ 03:20 PM

  6. Santiago Pastorino
    Santiago Pastorino
    • 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...

    April 28th, 2010 @ 07:07 PM

  7. Santiago Pastorino
  8. Repository
    Repository
    • State changed from verified to committed

    April 28th, 2010 @ 10:43 PM

  9. Si
    Si

    I can verify that this patch resolves my issue. Thanks Santiago!

    April 29th, 2010 @ 01:36 PM

  10. Santiago Pastorino
  11. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2
    • Importance changed from to Medium

    October 15th, 2010 @ 11:01 PM