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 385 forks source link

Nested forms disappear when validation fails #114

Closed DeanDeBlock closed 11 years ago

DeanDeBlock commented 11 years ago

When submitting the form the nested data doesn't render again if the validation fails

actions look like this

  def create
    @event = Event.new(params[:event])

    respond_to do |format|
      if @event.save
        format.html { redirect_to(@event, :notice => 'Event was successfully created.') }
        format.xml  { render :xml => @event, :status => :created, :location => @event }
      else
        format.html { render :action => "new" }
        format.xml  { render :xml => @event.errors, :status => :unprocessable_entity }
      end
    end
  end

  def new
    @event = Event.new
    respond_with(@event)
  end

An event belongs to a venue

  <fieldset>
    <legend><span class="label label-inverse" style="font-size: 1.0em">2</span>&nbsp;Where</legend>
    <div id="venue">
      <div id="venue_from_list">
        <div class="form-inputs">
          <%= f.association :venue, :include_blank => 'Choose existing venue', :input_html => { :class => 'span12' }  %>
        </div>
      </div>
      <%= link_to_add_association icon('plus') + ' add a new venue for this event', f, :venue, :class => 'btn btn-small', :style => "margin-left: 180px;" %>
    </div>
  </fieldset>

we also use simpleform

Does anybody have an idea how to fix this?

nathanvda commented 11 years ago

Can you show me the new.html.erb or new.html.haml view?

I see that @event is set correctly in both cases, so I am curious how your form is built (with which object), and how you call the simple_fields_for.

DeanDeBlock commented 11 years ago
<h1><%= icon("calendar") %> Create a New Event</h2>
<%= simple_form_for [:wizards, @event], :html => {:class=>'form-horizontal'}, :remote=>true do |f| %>
  <%= f.error_notification %>
  <div class="alert alert-error">
  <%= f.error :base, :id => 'validation_errors' %>
  </div>

  <fieldset>
    <legend><span class="label label-inverse" style="font-size: 1.0em">1</span>&nbsp;Add Your Event Details</legend>
    <div class="form-inputs">
      <%= f.input :name, :placeholder => 'Enter an event name', :input_html => { :class => 'span12' } %>
      <%= f.input :edition, :placeholder => 'Optionally give an edition here (year or integer).', :input_html => { :class => 'span12' }  %>

      <div class="control-group">
      <%= f.label :logo, :class=>'string optional control-label' %>
        <div class="controls">
          <%= image_tag(@event.logo.large, :class=>'event_form_logo') if @event.logo? %>
          <%= f.file_field :logo, :class=>'string optional' %>
          <%= f.hidden_field :logo_cache %>
        </div>
      </div>

      <div id="event_group">
        <div id="event_group_from_list">
          <%= f.association :event_group, :include_blank=>'Choose existing event group',:input_html => { :class => 'span12' }  %>
        </div>
        <%= link_to_add_association icon('plus') + ' add a new event group', f, :event_group, :class => 'btn btn-small', :style => "margin-left: 180px;" %>
      </div>
    </div>
  </fieldset>

  <fieldset>
    <legend><span class="label label-inverse" style="font-size: 1.0em">2</span>&nbsp;Where</legend>
    <div id="venue">
      <div id="venue_from_list">
        <div class="form-inputs">
          <%= f.association :venue, :include_blank => 'Choose existing venue', :input_html => { :class => 'span12' }  %>
        </div>
      </div>
      <%= link_to_add_association icon('plus') + ' add a new venue for this event', f, :venue, :class => 'btn btn-small', :style => "margin-left: 180px;" %>
    </div>
  </fieldset>

  <fieldset>
    <legend><span class="label label-inverse" style="font-size: 1.0em">3</span>&nbsp;When</legend>
    <div id="event_periods">
      <%= f.simple_fields_for :event_periods do |ep| %>
        <%= render 'event_period_fields', :f => ep %>
      <% end %>
      <div class="links">
        <%= link_to_add_association icon('plus') + ' add event period', f, :event_periods, :class => 'btn btn-small', :style => "margin-left: 180px;" %>
      </div>
    </div>
  </fieldset>
  <div class="form-actions">
    <%= link_to t('.cancel', :default => t("helpers.links.cancel")),
                events_path, :class => 'btn' %>
    <%= f.submit nil, :class => 'btn btn-primary' %>
  </div>
<% end %>
nathanvda commented 11 years ago

The partial for the event_periods is correct, but the other two associations are not. There is no call to simple_fields_for, so it does not iterate over the association. So it will never show any nested elements, even if they are saved correctly. You will have to use the simple_fields_for for those two as well.

I am not sure if you are mixing up your associations. When using f.association it does all the work for you. If you are using cocoon, you should render a partial per nested element, and I do not see that at all, except for the event_periods.

HTH.

dgmstuart commented 10 years ago

For those who come after me:

I think the reason that there's no call to simple_fields_for is because the relationships are belongs_to rather than has_many: rather than editing a list of items (which is what simple_fields_for would give you), the user wants to pick one venue from a select list, or create a new one using a form (ditto for event_group).

It does seem like cocoon is designed to support this kind of use case (it's in the main docs) but looking at the demo, it doesn't seem to have the desired behaviour: if you add some validations onto the Owner model, then on failed validation, the fields disappear.

The answer seems to be that cocoon doesn't (can't?) do the rendering for you - you need to do it yourself: i.e. conditionally show either the select or the form fields depending on whether you've got an (invalid) venue or not.

I've been having exactly the same problem (coincidentally also with event/venue relationships) and I've got a certain distance towards a working solution by taking that approach:

<% if @event.venue %>
  <%= fields_for :venue, @event.venue do |vf| %>
    <%= render 'venue_fields' %>
  <% end %>
<% else %>
  <%= f.association :venue, :include_blank => 'Choose existing venue', :input_html => { :class => 'span12' }  %>
  <%= link_to_add_association icon('plus') + ' add a new venue for this event', f, :venue, :class => 'btn btn-small', :style => "margin-left: 180px;" %>
<% end %>

This doesn't actually fully work, but it illustrates the point. To do it properly the f.association and link_to_add_association should be rendered but hidden if @event.venue exists, so that they can be shown on after-remove (i.e. clicking the link_to_remove_association)

Also, although it works for a form which is only used for creation, it probably wouldn't make sense for edit: the user will want to either select another venue from the list, or create a totally new one - not edit the previously selected or created venue.

Caveat: I'm not personally using simpleform and I don't normally write erb, so please excuse any errors

@nathanvda: I'm kind of concluding that Cocoon doesn't actually support this belongs_to use case described in the callbacks section of the readme, because it doesn't display failed validations.

I would be delighted to be proven wrong through - is there a better way of doing this?

nathanvda commented 10 years ago

Hi Duncan, I think your conclusions are mostly correct. It can be done, but it is not nearly as clean. It is sufficient for simple problems, but when handling validations it can quickly get dirty, and in that case I would rather do a two-step process. E.g. first save the (as per your example) newly created venue, and then select that from a list. But at the moment, cocoon is no help there. Mmmmm this is an interesting use-case, might be interesting to extend cocoon to support this. Not sure if I can find the time, but I will try to have a look.

dgmstuart commented 10 years ago

HI Nathan - many thanks for your response!

Yes, it seems like a tricky problem, but I think the use-case is fairly common. It might require quite a different approach, since the list, the form and the show/hide links are all closely related.

I think for the moment I'll just find some way of rolling my own solution for this specific case.

marcusg commented 9 years ago

hey guys, I'm facing the same problem atm - is there any other way or just the quick and dirty approach from @dgmstuart?

nathanvda commented 9 years ago

Nothing has changed or improved in the meantime :)

Chryus commented 9 years ago

I think I may have found a solution (at least when using standard forms). In the else statement of the create action, rebuild the nested belongs_to object, like in the new action. This solved the missing field problem for me after redirect on validation failure.

respond_to do |format|
  if @event.save
    format.html { redirect_to(@event, :notice => 'Event was successfully created.') }
    format.xml  { render :xml => @event, :status => :created, :location => @event }
  else
    @event.build_venue
    format.html { render :action => "new" }
    format.xml  { render :xml => @event.errors, :status => :unprocessable_entity }
  end
end
dgmstuart commented 9 years ago

@Chryus That will build a new blank venue associated with the event, right? I think the desired behaviour is to instead display the venue data which the user had previously entered.

(Apologies if I've got the wrong end of the stick: it's been a long time since I looked at this)