Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 !@setting.created_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>
share|improve this question
2  
just two comments: 1) never, ever, hardcode ids in the code, 2) Move code to the model, these methods have too much code. –  tokland Dec 20 '12 at 17:35
add comment

1 Answer

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.