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

Should #save also save nested/associated model objects? #73

Closed

Conversation

TylerRick
Copy link

https://github.com/apotonick/reform/blob/master/lib/reform/form/active_record.rb#L39

    def save(*)
      super.tap do
        model.save unless block_given? # DISCUSS: should we implement nested saving here?
      end
    end

When I tried out https://github.com/gogogarrett/reform_example, I noticed this bug there: when you edit an album and change one of the song titles, those changes are lost — they are not saved!

Since it is so easy to assume that associated model objects are going to be saved, perhaps this method should automatically save them... That would be my vote anyway.

I don't know how hard that would be though. I started adding a test for this in test/active_record_test.rb but then discovered that the test setup only has one ActiveRecord model available — Artist, which doesn't have any has_many association yet...

…y (:created_at => ["can't be

blank"]), just call from_hash directly, to make it clearer that we're not interested in any side
effects from calling validate.
@apotonick
Copy link
Member

Feel free to extend the AR tests!

BTW there's an issue with a discussion on this already. I couldn't find it, thou.

…veRecord automatically saves

the model object now.
…ave associated objects.

(Though if the main object has autosave: true on any associations, those would automatically get
saved when the main object is saved.)
@TylerRick
Copy link
Author

Thanks, I'll give it a try...

The closest I've found so far (discussions about nested models) are #33 and #43.

…ctiveRecord#save, in addition

to calling model.save on the "main model".  This should recursively make sure model.save gets called
on all nested model objects (as long as those nested forms also include Reform::Form::ActiveRecord).
@TylerRick
Copy link
Author

Okay, I've attached a pull request (with hub pull-request -i 73). Let me know what you think.

2 questions/concerns so far:

  1. Is there a better way to enumerate all nested collection "subforms" besides fields.each_pair?
  2. Since this calls save on the nested form, it is up to that form to then persist its own model object. This should happen automatically if you include Reform::Form::ActiveRecord in the collection block. (See Reform::Form::ActiveRecord is not automatically included? #72)

@TylerRick
Copy link
Author

Just saw this line in the code. Maybe that's what I was looking for...

mapper.new(@fields).nested_forms do |attr, form|

@apotonick apotonick closed this in 36f2323 Apr 30, 2014
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

Successfully merging this pull request may close these issues.

2 participants