Rails 4 deleting nested attributes, works on create but not on edit/update

Jeremy E picture Jeremy E · Jul 5, 2014 · Viewed 17.4k times · Source

So I'm working from this Railscast.

And I'm aware that there have been some changes in Rails 4 for Strong parameters.

First relevant question.

Second relevant question

I've quadruple checked my implementation, but can't see where I'm going wrong. As it is at the moment, ticking the "destroy" box when submitting the patient initially (i.e. the create method) works as intended, and will delete any medication that has a checked box and permitting any that does not (from the three form inputs it provides).

However when I subsequently edit that patient, any medications that don't get checked to be deleted are duplicated (so I end up with more attached medications than I started with), and any that are checked for deletion don't seem to change.

So if there two medicines attached "Med1" and "Med2", and I edit the patient, if both are marked for deletion I will still end up with "Med1" and "Med2". If only "Med1" is marked for deletion I will end up with "Med1" and "Med2" and an extra "Med2". If neither are marked for deletion I will end up with two each of "Med1" and "Med2".

#patient.rb
class Patient < ActiveRecord::Base
has_many :procedures
has_many :medications, dependent: :destroy
has_many :previous_operations, dependent: :destroy

accepts_nested_attributes_for :medications, :allow_destroy => true, :reject_if => lambda { |a| a[:name].blank? },
end

#views/patients/_form.html.erb
<%= form_for(@patient) do |f| %>
  <% if @patient.errors.any? %>
    <div id="error_explanation">
      <h2><%= pluralize(@patient.errors.count, "error") %> prohibited this patient from being saved:</h2>

      <ul>
      <% @patient.errors.full_messages.each do |msg| %>
        <li><%= msg %></li>
      <% end %>
      </ul>
    </div>
  <% end %>

  <%= f.fields_for :medications do |builder| %>
    <%= render "medication_fields", :f => builder %>
  <% end %>

  <div class="field">
    <%= f.label :first_name %><br>
    <%= f.text_field :first_name %>
  </div>
  <div class="field">
    <%= f.label :last_name %><br>
    <%= f.text_field :last_name %>
  </div>
  <div class="actions">
    <%= f.submit %>
  </div>
<% end %>

#views/patients/medications_fields.html
<div class="field">
  <%= f.label :name %><br>
  <%= f.text_field :name %>
</div>
<div class="field">
  <%= f.label :_destroy, "Remove Medication" %>
  <%= f.check_box :_destroy %>
</div>

#controllers/patients_controller.rb
class PatientsController < ApplicationController
  before_action :set_patient, only: [:show, :edit, :update, :destroy]

  # GET /patients
  # GET /patients.json
  def index
    @patients = Patient.all
  end

  # GET /patients/1
  # GET /patients/1.json
  def show
  end

  # GET /patients/new
  def new
    @patient = Patient.new
    3.times { @patient.medications.build }
  end

  # GET /patients/1/edit
  def edit
  end

  # POST /patients
  # POST /patients.json
  def create
    @patient = Patient.new(patient_params)

    respond_to do |format|
      if @patient.save
        format.html { redirect_to @patient, notice: 'Patient was successfully created.' }
        format.json { render action: 'show', status: :created, location: @patient }
      else
        format.html { render action: 'new' }
        format.json { render json: @patient.errors, status: :unprocessable_entity }
      end
    end
  end

  # PATCH/PUT /patients/1
  # PATCH/PUT /patients/1.json
  def update
    respond_to do |format|
      if @patient.update(patient_params)
        format.html { redirect_to @patient, notice: 'Patient was successfully updated.' }
        format.json { head :no_content }
      else
        format.html { render action: 'edit' }
        format.json { render json: @patient.errors, status: :unprocessable_entity }
      end
    end
  end

  # DELETE /patients/1
  # DELETE /patients/1.json
  def destroy
    @patient.destroy
    respond_to do |format|
      format.html { redirect_to patients_url }
      format.json { head :no_content }
    end
    flash[:notice] = "Patient was successfully deleted."
  end

  private

    # Never trust parameters from the scary internet, only allow the white list through.
    def patient_params
      params.require(:patient).permit(:first_name, :last_name, medications_attributes: [:name, :_destroy])
    end

end

I've been super careful and checked over a million times the :_destroy flag being allowed through strong parameters, but still no dice.

Any help appreciated, must be something obvious that I just can't see.

EDIT

Changing this...

    # Never trust parameters from the scary internet, only allow the white list through.
    def patient_params
      params.require(:patient).permit(:first_name, :last_name, medications_attributes: [:name, :_destroy])
    end

To this...

    # Never trust parameters from the scary internet, only allow the white list through.
    def patient_params
      params.require(:patient).permit!
    end

appears to work correctly, so I'm still sure it's something to do with strong parameters, but the latter is less secure I'm sure and not best practice.

Answer

Mandeep picture Mandeep · Jul 5, 2014

when you are doing

# Never trust parameters from the scary internet, only allow the white list through.
def patient_params
  params.require(:patient).permit!
end

it permits all of your attributes and it's not recommended. It's more of a hack rather than a solution.

If you look at your question

Rails 4 deleting nested attributes, works on create but not on edit/update

and if you look at your params permitted

# Never trust parameters from the scary internet, only allow the white list through.
def patient_params
  params.require(:patient).permit(:first_name, :last_name, medications_attributes: [:name, :_destroy])
end

This will work on creating patients but not on updating or editing them because when you create a new record it doesn't require you to permit id but when you want to update or edit a record you need its id to be permitted as well.

Fix:

Just pass the id attribute to permitted attributes and it will work for you

# Never trust parameters from the scary internet, only allow the white list through.
def patient_params
  params.require(:patient).permit(:id, :first_name, :last_name, medications_attributes: [:id,:name, :_destroy])
end