search

remi / her

Her is an ORM (Object Relational Mapper) that maps REST resources to Ruby objects. It is designed to build applications that are powered by a RESTful API instead of a database.
MIT License
2.05k stars 322 forks source link

NumericalityValidator support for activemodel #382

Open ggpasqualino opened 9 years ago

ggpasqualino commented 9 years ago

In the commit https://github.com/rails/rails/commit/f888fad4db6d6956b675ca0441baa5aa63edae32 it was introduced the method record_attribute_changed_in_place? to ActiveModel::Validations::NumericalityValidator which calls record.attribute_changed_in_place?. The intention was that if this method was added by ActiveRecord::AttributeMethods::Dirty then it would be called. The problem is that Her::Model doesn't include that and doesn't implement that but it responds to that because it assumes on https://github.com/remiprev/her/blob/master/lib/her/model/attributes.rb#L74 that attribute_changed_in_place is a new attribute.

Her version: 0.8.1 Active model version: 4.2.4

glappen commented 9 years ago

I think I'm running into this also when I try to use "validates_on_numericality_of":

Failure/Error: should validate_presence_of(f)
     ArgumentError:
       wrong number of arguments (2 for 1)
     # /gems/2.2.0/gems/her-0.8.1/lib/her/model/attributes.rb:173:in `attribute?'
     # /gems/2.2.0/gems/activemodel-4.2.1/lib/active_model/attribute_methods.rb:385:in `attribute_changed_in_place?'
     # /gems/2.2.0/gems/activemodel-4.2.1/lib/active_model/validations/numericality.rb:98:in `record_attribute_changed_in_place?'
     # /gems/2.2.0/gems/activemodel-4.2.1/lib/active_model/validations/numericality.rb:26:in `validate_each'
     # /gems/2.2.0/gems/activemodel-4.2.1/lib/active_model/validator.rb:151:in `block in validate'
     # /gems/2.2.0/gems/activemodel-4.2.1/lib/active_model/validator.rb:148:in `each'
     # /gems/2.2.0/gems/activemodel-4.2.1/lib/active_model/validator.rb:148:in `validate'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:455:in `public_send'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:455:in `block in make_lambda'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:192:in `call'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:192:in `block in simple'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:504:in `call'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:504:in `block in call'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:504:in `each'
     # /gems/2.2.0/gems/activesupport-4.2.1/lib/active_support/callbacks.rb:504:in `call'

If someone can give me a little direction, I'd be happy to help fix this as we need it to be working for our project ASAP. Thanks!

kritik commented 9 years ago

Here's two options to fix it: 1) monkey patch for number-validation https://gist.github.com/kritik/2c98b73a390c557b2d1a. It works for me 2) fix her to use ActiveRecord::AttributeSet

glappen commented 9 years ago

I actually fixed it in Her and I have a pull request pending approval.

kritik commented 9 years ago

Can you add a link here with you pull request?

glappen commented 9 years ago

Yeah, I should have included this before. My pull request is here: https://github.com/remiprev/her/pull/383

hubert commented 9 years ago

hi @glappen first off, thanks for the contribution. i've been on the road for a bit so haven't had much time to review PRs. apologies for the slow reply

after looking at your PR and @kritik suggestion, i fear that a solution of this nature might just paper over the root cause -- that Her::Model is not compliant with ActiveModel. would you want to take a stab at a solution using that approach?

glappen commented 9 years ago

I would be willing to take a stab at it. Can you elaborate a bit more about what you have in mind, and what would be involved in becoming ActiveModel compliant? Thanks! On Nov 6, 2015 9:44 PM, "hubert huang" notifications@github.com wrote:

hi @glappen https://github.com/glappen first off, thanks for the contribution. i've been on the road for a bit so haven't had much time to review PRs. apologies for the slow reply

after looking at your PR and @kritik https://github.com/kritik suggestion, i fear that a solution of this nature might just paper over the root cause -- that Her::Model is not compliant with ActiveModel. would you want to take a stab at a solution using that approach?

— Reply to this email directly or view it on GitHub https://github.com/remiprev/her/issues/382#issuecomment-154603514.

glappen commented 8 years ago

@hubert - Following up as I haven't heard anything (maybe because I forgot to mention you in my response). Are you suggesting @kritik 1st of 2nd option? monkey patch number validation or implement ActiveRecord::AttributeSet? Can you explain how implementing ActiveRecord::AttributeSet solves the problem here? Thanks!

hubert commented 8 years ago

hi @glappen. i understand your confusion. we def shouldn't be pulling in anything from ActiveRecord. when i looked at it earlier, i must've misread it as ActiveModel. my issue with the solution may be that i don't like the way the situation with record_attributed_in_place is handled in ActiveModel.

let me look a bit more closely and i'll get back to you.