Filtering index using has_many through relationships

Both Cities and Positions are associated to Jobs through a has_many :through relationship.

On the Jobs#Index page, I have a form, which when submitted allows a user to filter jobs based on a combination of city and positions.

I got this to work, but know the code can be a lot better.

This is my Jobs#Index controller:

def index 
    @cities = City.all
    @positions = Position.all

    @city = City.find(params[:city_id]) if params[:city_id] && params[:city_id] != "0"
    @position = Position.find(params[:position_id]) if (params[:position_id] && params[:position_id] != "0")

    @jobs = Job.all
    @jobs = @jobs.includes(:cities).where(cities: { id: @city }) if @city
    @jobs = @jobs.includes(:positions).where(positions: { id: @position }) if @position
    @jobs = @jobs.paginate(page: params[:page], per_page: 5)

  end

This is my Jobs#Index View:

```

• provide(:title, 'All Jobs')

.thinstripe_career
.darken
.container
.row
.span10.offset1
.jobs_header
.row
.span7
h2 All #{@city.name if @city} #{@position.name if @position} Jobs
.span3
== link_to "Sign up for Job Alerts!", "#", :class => "button"

• if current_user && current_user.admin?

== render 'shared/dashboard_header'

.container
.row
.span10.offset1
.job_search
== form_tag jobs_path, :method => 'get', :id => "jobs_filter" do
h2 I'd like jobs in
.filter_sect
== select_tag :city_id, content_tag(:option,'all cities...', :value=>"0") + options_from_collection_for_select(@cities, "id", "name", params[:city_id].to_i ), :class => "basic"
.filter_sect
== select_tag :position_id, content_tag(:option,'all roles...',:value=>"0") + options_from_collection_for_select(@positions, "id", "name", params[:position_id].to_i ), :class => "basic"
.filter_sect.filter_search
== submit_tag "Search"

In models/job.rb, you can add scopes and use them chained on the controller:

class Job (city) { includes(:cities).where(cities: { id: @city })  if city }
  scope :with_positions, ->(position) { includes(:positions).where(positions: { id: @position }) if position }

  # rest of the code ...
end

For JobsController, you can:

-
Add a before_filter to load all the data, and if you need to use that data on other actions, you just need to append the name of that action too.

-
Use memoization to load the city and the position.

In controllers/jobs_controller.rb:

class JobsController < ApplicationController
  before_filter :load_data, only: [:index]

  def index
    @jobs = Job.with_cities(city).with_positions(position).paginate(page: params[:page], per_page: 5)
  end

  # ...

  protected:

  def load_data
    @cities = City.all
    @positions = Position.all
  end

  def city
    @city ||= City.find(params[:city_id]) unless params[:city_id].empty?
  end

  def position
    @position ||= Position.find(params[:position_id]) unless params[:position_id].empty?
  end

On the view:

Use try, so instead of:

- if current_user && current_user.admin? 
      == render 'shared/dashboard_header'

you could write:

== render 'shared/dashboard_header' if current_user.try(:admin?)

Use include_blank and prompt options in select_tag.
This not only simplifies the views, but also allows to use params[city_id].empty? on the controller.

select_tag :city_id, 
  options_from_collection_for_select(@cities, :id, :name, params[:city_id].to_i), 
    include_blank: true, prompt: "all cities...", class: "basic"

Finally, you could extract ul.jobs into a partial:

= render partial: 'jobs', locals: { jobs: @jobs }

Or even the .job_search div, especially if you'll be needing this somewhere else.

StackExchange Code Review Q#38650, answer score: 3