Has many through relationship needs code review

Question

Having an issue optimising has_many through relationship

To quickly introduce I have Setting model and Notification model (that is mainly used as a storage of all notification types).

They have through relationship :email_notification, where notification can be specified as primary or secondary.

I think I have hardcoded the new, create, edit and update actions:

  def new
    @setting = Setting.new(:truck_fleet_id => current_user.truck_fleet.id) if !current_user.admin?
    @setting = Setting.new(:truck_fleet_id => 3) if current_user.admin
    @notifications = Notification.all

    respond_to do |format|
      format.html # new.html.erb
      format.json { render json: @setting }
    end
  end

  # GET /settings/1/edit
  def edit
    @setting = Setting.find(params[:id])
    @notifications = Notification.all
    @notifications.each do |n|
      @setting.email_notifications.build(:notification_id => n.id)
    end if @setting.email_notifications.blank?
  end

  # POST /settings
  # POST /settings.json
  def create
    @setting = Setting.new(params[:setting])
    @notifications = Notification.all
    respond_to do |format|
      if @setting.save
        @notifications.each do |n|
          if params[:fields][n.id.to_s].nil?
            @setting.email_notifications.create!(:notification_id => n.id.to_s, :primary => false, :secondary => false)
          else
            @setting.email_notifications.create!(:notification_id => n.id.to_s, :primary => params[:fields][n.id.to_s][:primary] ||= false, :secondary => params[:fields][n.id.to_s][:secondary] ||= false)
          end
        end
        format.html { redirect_to @setting, notice: 'Setting was successfully created.' }
        format.json { render json: @setting, status: :created, location: @setting }
      else
        format.html { render action: "new" }
        format.json { render json: @setting.errors, status: :unprocessable_entity }
      end
    end

  end

  # PUT /settings/1
  # PUT /settings/1.json
  def update
    @setting = Setting.find(params[:id])
    @notifications = Notification.all
    respond_to do |format|
      if @setting.update_attributes(params[:setting])
        @notifications.each do |n|
          em = @setting.email_notifications.where(:notification_id => n.id).first
          if params[:fields][n.id.to_s].nil?
            em.update_attributes(:notification_id => n.id.to_s, :primary => false, :secondary => false)
          else
            em.update_attributes(:notification_id => n.id.to_s, :primary => params[:fields][n.id.to_s][:primary] ||= false, :secondary => params[:fields][n.id.to_s][:secondary] ||= false)
          end
        end
        format.html { redirect_to @setting, notice: 'Setting was successfully updated.' }
        format.json { head :no_content }
      else
        format.html { render action: "edit" }
        format.json { render json: @setting.errors, status: :unprocessable_entity }
      end
    end
  end

and in the app/views/settings/_form.html.erb

<%= form_for @setting, :html => { :class => 'form-horizontal' } do |f| %>
    <% @setting.email_notifications.each do |e, i| %>
        <%= f.fields_for e do |builder| %>
            <%= render "update_email_notifications", :f => builder, :emails => e, :index => i if !e.notification.nil? %>
        <% end %>
    <% end if [email protected]_at.nil? %>
    <% @notifications.each do |n| %>
        <%= f.fields_for n.email_notifications do |builder| %>
            <%= render "create_email_notifications", :f => builder, :notification => n, :index => n.id %>
        <% end %>
    <% end if @setting.created_at.nil? %>
    <% end %>
<% end %>

in app/views/settings/_update_email_notifications.html.erb

<div class="control-group">
    <b><%= f.label :main, "#{emails.notification.name}", :class => 'control-label' if !emails.notification.nil? %></b>
    <div class="controls">
        <%= puts emails.primary.inspect %>
        <%= check_box_tag "fields[#{emails.notification.id}][primary]", '1', emails.primary %> Primary Contacts </br>
        <%= check_box_tag "fields[#{emails.notification.id}][secondary]", '1', emails.secondary %> Secondary Contacts
        <% days = [['1 day', 1],['2 days', 2],['3 days', 3],['4 days', 4],['5 days', 5],['6 days', 6],['7 days', 7]]%>
        <%= collection_select emails, "fields[#{emails.notification.id}][interval]", days, :last, :first if Notification.find(emails.notification_id).required_intervals %>
    </div>
</div>

Answer 1

This is just a starting point for you. Move the logic found in the new controller into the settings model. Refactor the constant id into a model constant. Move Notification.all into a before_filter since it is called in every controller action. In the edit, I do not like having the if statement after the block. I think it is less readable.

Model:

class Setting < ActiveRecord::Base
  ADMIN_TRUCK_FLEET_ID = 3

  def self.new_with_truck_id(user)
    fleet_id = user.admin? ? ADMIN_TRUCK_FLEET_ID : user.truck_fleet.id
    self.new(truck_fleet_id: fleet_id)
  end
end

Controller (New and Edit):

class SettingsController
  before_filter :get_notifications

  def new
    @setting = Setting.new_with_truck_id(current_user)

    respond_to do |format|
      format.html
      format.json { render json: @setting }
    end
  end

  def edit
    @setting = Setting.find(params[:id])
    if @setting.email_notifications.blank?
      @notifications.each do |n|
        @setting.email_notifications.build(:notification_id => n.id)
      end
    end
  end

  private

  def get_notifications
    @notifications = Notification.all
  end
end

Further to do: Move the conditionals in create and update out of the respond_to block. n.id.to_s can be refactored to n.to_param. Think about if more logic can be pushed to the model. Does email notification occur every time a model is saved? Maybe push it to a model callback (after_save) or an observer.