3
votes

I've come across a rather bizarre error. I have a nested form that works as expected except when a validation fails on an existing record. When a validation fails on an existing record, the re-rendered edit view contains fields for the invalid record twice. The first set of fields is filled out according to the way the object is currently stored. The second set of fields is filled out with the information that was just submitted and found to be invalid.

I have a basic nested form where a parent (ShiftPeriod) has_many children (Shifts) and each child belongs_to the parent. ShiftPeriod accepts_nested_attributes for Shifts, with allow_destroy set to true. I'm using the nested_form gem, but I also tried using a regular form_for with the same result

The form for ShiftPeriod (I removed as much as possible to try to keep it simple until i figure this out):

<%= nested_form_for @shift_period do |f| %>
  <%= f.fields_for :shifts %>
  <%= f.link_to_add "Add shift", :shifts %>
  <%= f.submit %>
<% end %>

The partial with the fields for shifts:

<%= f.select :member_id, options_for_select(Member.crew_members.order('last_name').collect{|member| ["#{member.last_name}, #{member.first_name}", member.id]}, :selected => Member.where(:bars_num == 1).first.id) %>
<%= f.collection_select :start_time, @time_range, :dup, :hour, :selected => Time.parse(f.object.start_time.to_s) || @shift_period.start_time %>
<%= f.collection_select :end_time, @time_range, :dup, :hour, :selected => f.object.new_record? ? @time_range.last : Time.parse(f.object.end_time.to_s) %>
<%= f.select :repeat_month, options_for_select([['Never', false], ['Monthly', true]]) %>
<%= f.select :repeat, options_for_select([['Never', 0], ['Every Other Week', 1], ['Every Week', 2]]) %>
<%= f.link_to_remove "Remove" %>

The relevant part of the Shift object:

class Shift < ActiveRecord::Base
  include Coverage::SetOperations

  belongs_to :member
  belongs_to :shift_period

  delegate :date, :to => :shift_period
  delegate :daynight, :to => :shift_period

  after_save :update_shift_period_open_slots
  after_destroy :update_shift_period_open_slots

  validates_presence_of :member, :start_time, :end_time, :shift_period

The relevant part of the ShiftPeriod object:

class ShiftPeriod < ActiveRecord::Base
  has_many :shifts
  has_many :open_slots, :dependent => :destroy
  has_many :calls
  after_create :update_open_slots
  validates_presence_of :date
  validates :date, :uniqueness => {:scope => :daynight}

  accepts_nested_attributes_for :shifts, :reject_if => lambda {|a| a[:start_time].blank? || a[:end_time].blank? || a[:member_id].blank? || a[:repeat].blank? }, :allow_destroy => true

The controller: as_many children (Shifts) and each child belongs_to the parent. ShiftPeriod accepts_nested_attributes for Shifts, with allow_destroy set to true. I'm using the nested_form gem, but I also tried using a regular form_for with the same result

The controller:

def edit
  @shift_period = ShiftPeriod.find(params[:id])
  set_time_range
end

def set_time_range
  @time_range = @shift_period.daynight ? (6..18).to_a : (18..23).to_a + (0..6).to_a
  @time_range.collect!{|val| @shift_period.start_time - @shift_period.start_time.hour.hours + val.hours }
end

def update
  @shift_period = ShiftPeriod.find(params[:id])
  respond_to do |format|
    if @shift_period.update_attributes(params[:shift_period])
      format.html { redirect_to(schedule_path(:date => @shift_period.date, :notice => 'Shift period was successfully updated')) }
      format.xml  { head :ok }
    else
      set_time_range
      format.html { render :action => "edit" }
      format.xml  { render :xml => @shift_period.errors, :status => :unprocessable_entity }
    end
  end
end

The form for ShiftPeriod (I removed as much as possible to try to keep it simple until i figure this out):

<%= nested_form_for @shift_period do |f| %>
  <%= f.fields_for :shifts %>
  <%= f.link_to_add "Add shift", :shifts %>
  <%= f.submit %>
<% end %>

The partial with the fields for shifts:

<%= f.select :member_id, options_for_select(Member.crew_members.order('last_name').collect{|member| ["#{member.last_name}, #{member.first_name}", member.id]}, :selected => Member.where(:bars_num == 1).first.id) %>
<%= f.collection_select :start_time, @time_range, :dup, :hour, :selected => Time.parse(f.object.start_time.to_s) || @shift_period.start_time %>
<%= f.collection_select :end_time, @time_range, :dup, :hour, :selected => f.object.new_record? ? @time_range.last : Time.parse(f.object.end_time.to_s) %>
<%= f.select :repeat_month, options_for_select([['Never', false], ['Monthly', true]]) %>
<%= f.select :repeat, options_for_select([['Never', 0], ['Every Other Week', 1], ['Every Week', 2]]) %>
<%= f.link_to_remove "Remove" %>

The relevant part of the Shift object:

class Shift < ActiveRecord::Base
  include Coverage::SetOperations

  belongs_to :member
  belongs_to :shift_period

  delegate :date, :to => :shift_period
  delegate :daynight, :to => :shift_period

  after_save :update_shift_period_open_slots
  after_destroy :update_shift_period_open_slots

  validates_presence_of :member, :start_time, :end_time, :shift_period

The relevant part of the ShiftPeriod object:

class ShiftPeriod < ActiveRecord::Base
  has_many :shifts
  has_many :open_slots, :dependent => :destroy
  has_many :calls
  after_create :update_open_slots
  validates_presence_of :date
  validates :date, :uniqueness => {:scope => :daynight}

  accepts_nested_attributes_for :shifts, :reject_if => lambda {|a| a[:start_time].blank? || a[:end_time].blank? || a[:member_id].blank? || a[:repeat].blank? }, :allow_destroy => true

The controller:

def edit
  @shift_period = ShiftPeriod.find(params[:id])
  set_time_range
end

def set_time_range
  @time_range = @shift_period.daynight ? (6..18).to_a : (18..23).to_a + (0..6).to_a
  @time_range.collect!{|val| @shift_period.start_time - @shift_period.start_time.hour.hours + val.hours }
end

def update
  @shift_period = ShiftPeriod.find(params[:id])
  respond_to do |format|
    if @shift_period.update_attributes(params[:shift_period])
      format.html { redirect_to(schedule_path(:date => @shift_period.date, :notice => 'Shift period was successfully updated')) }
      format.xml  { head :ok }
    else
      set_time_range
      format.html { render :action => "edit" }
      format.xml  { render :xml => @shift_period.errors, :status => :unprocessable_entity }
    end
  end
end
1
A little more fiddling and I discovered the duplicate forms are being displayed because there are two ruby objects in my parent class's child association (@shift_period.shifts) for each record. In other words there are multiple shift objects in the association with the same id. For example, if I display @shift_period.shifts.length on the form for a shift period with two existing shifts, it is 2 initially and four after I try to submit an invalid shift and the edit view is rendered again. No idea why.... Anyone have any thoughts?Sam Judd
I'm using a hack to get around this for now (remove the duplicated objects from the association array before rendering edit again), but why would fields_for add more than one object to the association per record just because the validation failed? Why does this only occur for validation failures on existing records and not on new records?Sam Judd
Damn it, even after 4 years the same problem is still there !. But maybe you have found the source of the problem ?Cyril Duchon-Doris

1 Answers

1
votes

Here is the hack I'm using for now to get around the problem, better ideas would be greatly appreciated.

def update
  @shift_period = ShiftPeriod.find(params[:id])
  if @shift_period.update_attributes(params[:shift_period])
    redirect_to(schedule_path(:date => @shift_period.date, :notice => 'Shift period was successfully updated')) 
  else
    set_time_range

    new_records = []
    @shift_period.shifts.each{|shift| if shift.new_record? then new_records << shift end}
    @shift_period.shifts.slice!(0,@shift_period.shifts.length/2)
    @shift_period.shifts += new_records
    render :action => "edit"
  end

end