Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested form issue: undefined method from_hash for nil #69

Closed
ivanyv opened this issue Feb 19, 2014 · 22 comments
Closed

Nested form issue: undefined method from_hash for nil #69

ivanyv opened this issue Feb 19, 2014 · 22 comments

Comments

@ivanyv
Copy link

ivanyv commented Feb 19, 2014

I haven't slept much, so probably I'm missing something obvious but I've been going around this for a couple hours to no avail.

class InvoiceForm < Reform::Form
  include Reform::Form::ActiveRecord

  property :date

  collection :line_items do
    property :description
    property :quantity
    property :price
  end
end

params = {"date"=>"2014-02-18", "line_items_attributes"=>
{"1392781994276"=>{"_destroy"=>"false", "description"=>"Manual entry", "quantity"=>"1", "price"=>"20"}}}

invoice = InvoiceForm.new(Invoice.new)
invoice.validate(params) # This throws the error on lib/representable/deserializer.rb:50

Right now I'm about to use a composition instead, to work around this issue, but I'm wondering what's the real problem.

@ivanyv
Copy link
Author

ivanyv commented Feb 19, 2014

Still haven't found the issue... Here's a simple test case: https://github.com/ivanyv/reform-test

Go to /albums/new and try to create it to see the exception.

What I've gathered so far is that object on ObjectDeserializer.new is a Hash with the song params instead of a Form object like it expects (https://github.com/apotonick/representable/blob/master/lib/representable/deserializer.rb#L28).

@apotonick
Copy link
Member

This is the old problem of a not properly set up Invoice: #67

Reform doesn't know how to setup your models (, yet), you have to do it yourself and provide a LineItem attached to the invoice.

@tamouse
Copy link

tamouse commented Feb 21, 2014

I am having this problem as well, but I thought I had set up my models correctly. At https://gist.github.com/tamouse/9126168#file-subscriptionscontroller-rb-L27-L36:

  def build_subscription(user)
    # For Reform::Form, need to build out items
    user.build_extra_user_information
    user.extra_user_information.build_credit_card
    user.extra_user_information.build_billing_address
    user.extra_user_information.build_shipping_address
    user.build_subscription
    user.build_reward
    user
  end

I am setting up the model that gets passed in to the new call. In particular, when I run self.save, I'm getting the NoMethodError: undefined method 'from_hash' for #<CreditCard:0x007fcf3349a638> . Is there something else I should be doing? CreditCard is under ExtraUserInfo which is under User which is the model the form is working on.

@apotonick
Copy link
Member

@tamouse your problem seems to be something else, as it tries to call from_hash on the CreditCard instance, which is totally wrong. It might be the 2-level nesting, I am working on this stuff atm.

@tamouse
Copy link

tamouse commented Feb 21, 2014

Thanks much, @apotonick . Let me know if I can offer any more information.

@ivanyv
Copy link
Author

ivanyv commented Feb 21, 2014

Ah, thanks Nick, I did end up duplicating Rails' accepts_nested_at functionality.

@apotonick
Copy link
Member

🤦 NO, just set up your model correctly for now, @ivanyv !!!

@ivanyv
Copy link
Author

ivanyv commented Feb 21, 2014

LOL, well, right now it's working as I intended and I'm on a deadline so will have to wait until next refactoring :P

Say, what exactly do you mean by setting it up correctly?

invoice = InvoiceForm.new(Invoice.new)
invoice.line_items = [ LineItemForm.new(LineItem.new) ]
invoice.validate(params)

Patience man, patience :D

@apotonick
Copy link
Member

Yeah, exactly! ❤️ Does that work for you?

@ivanyv
Copy link
Author

ivanyv commented Feb 21, 2014

Argh, it doesn't work, same exception :(

@apotonick
Copy link
Member

@ivanyv Does the exception happen at form creation or after calling #validate?

@ivanyv
Copy link
Author

ivanyv commented Feb 24, 2014

It happens on validate.

@apotonick
Copy link
Member

I guess this is because you setup your model correctly when rendering the form but not when validating it, @ivanyv (two different actions).

@apotonick
Copy link
Member

@tamouse You're using the form wrong! You have to call form.validate(params[..]) and then .save.

@ivanyv
Copy link
Author

ivanyv commented Feb 25, 2014

Man this feels so much harder than you'd think. If you could find a bit of time to fork my sample repo (https://github.com/ivanyv/reform-test) and show us mortals how it's done? :D

I tried these two variations before validation (on create):

@album = AlbumForm.new Album.new(songs: []) # I knew it wouldn't work, but just for kicks
@album = AlbumForm.new Album.new(songs: [Song.new] * params[:album][:songs_attributes].keys.size)
@album.validate(params[:album])

The second one at least doesn't throw an exception (and feels godawful weird), but only the first song is saved.

So excuse my french, but how the fuck do I setup the model correctly? (curse directed at my frustration, not you :D

@ivanyv
Copy link
Author

ivanyv commented Feb 25, 2014

And doh! The second variation doesn't save all songs because all elements in the array are one and the same 🤦

@album = AlbumForm.new Album.new
params[:album][:songs_attributes].each do
  @album.songs << Song.new
end
# And on update:
@album = AlbumForm.new Album.find(params[:id])
@album.songs = @album.model.songs

Still, I don't think that's the way it should be done, is it?

@apotonick
Copy link
Member

That's aaaalmost correct 😬 I've been thinking about this today and the problem is reform is not supposed to know how "the fuck" your models work. You might want to create a brand-new song for each entry in params, others want to find a song, then create, other might want to whatever.... it's not that easy to solve, otherwise I would have done that months ago.

@apotonick
Copy link
Member

Ideally, the form wouldn't know anything about the model and vice-versa:

album = Album.new
3.times do 
  album.songs << Song.new
end

AlbumForm.new(album)

I might come up with a simplification pretty soon.

@ivanyv
Copy link
Author

ivanyv commented Feb 27, 2014

I think I finally get it. I was just trying too hard to fit it in my mind with how ActiveRecord works. But, I've been thinking, Reform does know about my models, at least in the limited way I want it to handle them:

class AlbumForm < Reform::Form
  property :name
  collection :songs do
    property :artist
    property :title
  end
end

So when I do @album.validate(params[:album]), what's stopping a Reform ActiveRecord module from automatically initializing AR objects as necessary? It might "pollute" Reform somewhat, but as long as it's in the separate module I don't see a problem (and it would help with developing modules for other ORMs).

Barring that, I'd build my own wrapper around it (which is actually sort of what I did by replicating some of Rails' nested attributes functionality).

@apotonick
Copy link
Member

It's exactly how you say: I will build a separate component that knows about the ORM-internals like id and does instantiate the right objects.

I had a look at it and I decided it's simplest to just implement it in Reform now and later move it to representable.

Anyway, I wanna stand my ground here for keeping Reform's core clean!!! The reason why nested_attributes has got problems is because it knows too much about everything and there's no abstraction between different layers.

Reform clearly requires you to pass in a prepared model and then does all the nesting without even knowing it's dealing with ActiveRecord. That makes it more flexible, forward-compatible and easier to extend.

@ivanyv
Copy link
Author

ivanyv commented Mar 8, 2014

Completely agree with your approach :)

I've been having difficulty parsing Representable's code; right now I just don't have enough spare brain cycles it seems. I'm building a lot of experience using Reform itself though, so I hope I can be of help later on.

@apotonick
Copy link
Member

Implemented :populate_if_empty in 1.0: https://github.com/apotonick/reform#populating-forms-for-validation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants