nathanvda / cocoon

Dynamic nested forms using jQuery made easy; works with formtastic, simple_form or default forms
http://github.com/nathanvda/cocoon
MIT License
3.08k stars 382 forks source link

Rendering the edit screen for an item with a has_one association deletes the association #609

Open ConfusedVorlon opened 3 years ago

ConfusedVorlon commented 3 years ago

Foo has_one Bar (optional)

Creating a foo with a Bar works correctly However -merely showing the edit form causes the associated bar to be deleted

steps: 1) create Foo with nested Bar this shows http://localhost:3000/foos/2 where we can see that foo1 does indeed have a bar

2) Click edit http://localhost:3000/foos/2/edit this shows the foo, and the associated bar however: The Bar has already been deleted even though we have never directly called save, or clicked on the save button)

(opening http://localhost:3000/foos/2 agin confirms this)

the line which causes this problem is link_to_add_association 'add Bar', f, :bar

The wild thing to me is the idea that simply rendering link_to_add_association could change my model (by deleting an association)

demo project here: https://github.com/ConfusedVorlon/Cocoon-AddAssociationDelete-Example

key classes:

class Foo < ApplicationRecord
    has_one :bar, inverse_of: :foo, dependent: :destroy, required: false

    accepts_nested_attributes_for :bar, reject_if: :all_blank, allow_destroy: true
end
class Bar < ApplicationRecord
  belongs_to :foo
end
#views/foos/form.html.haml
= form_for @foo do |f|
  - if @foo.errors.any?
    #error_explanation
      %h2= "#{pluralize(@foo.errors.count, "error")} prohibited this foo from being saved:"
      %ul
        - @foo.errors.full_messages.each do |message|
          %li= message

  .field
    = f.label :name
    = f.text_field :name
  %h5 Bar
  .form-group
    = f.fields_for :bar do |bar_form|
      = render 'bar_fields', f: bar_form
    .links.max-one-association
      = link_to_add_association 'add Bar', f, :bar
  .actions
    = f.submit 'Save'
#views/foos/_bar_fields.html.haml
.nested-fields
  %br
  .form-group
    = f.label :title
    = f.text_field :title, class: "form-control", placeholder: "(optional)"
    = link_to_remove_association "remove Bar", f, class: "btn btn-small btn-danger mb-2"
ConfusedVorlon commented 3 years ago

note this also breaks updating, because by the time we actually try to save an update, the bar has already been deleted, so the association fails to find the bar with the expected id to update

nathanvda commented 3 years ago

Yes this is a known issue, because we are using the association details to create the nested items, rails presumes, for a has-one relation, only one item can exist.

This was raised in #414 and answered on stackoverflow. In short you can easily circumvent this by using the :force_non_association_create option.

= link_to_add_association 'add something', @form_obj, :comments, force_non_association_create: true

which will create a new nested element, but not using the association, so not interfering with the current state in the database.

This specific case is also documented in the wiki but is afaik rarely used (since there is less of a dynamic aspect managing a single child, as opposed to adding an unlimited number of childs I guess).

ConfusedVorlon commented 3 years ago

thanks for the super-quick response. My google-foo is clearly not up to scratch!

would you accept a pull request to add a short note to this effect in the main page? (similar to the Strong Parameters Gotcha)

nathanvda commented 3 years ago

Sure 👍

nathanvda commented 3 years ago

Ha there already was some effort to document it (or warn of this behaviour) in #247, and in that very very very old PR I claim I would attempt to fix the code. Looking at the code I can see two things:

Since I have to explicitly decide to call the build_#{association} method, I can as well just call the non-association create method. I was thinking in which case that might ever cause problems (not creating the instance on the association, but it would always cause problems --aka deleting the existing record if there is one).

I will brew a fix 👍