Am I doing this Association Correctly?

Ruby on Rails
1

Am I doing this assosiation correctly, or is there a slicker way of doing this?

class Company < ActiveRecord::Base
  has_many :job_sheets
end

class JobSheet < ActiveRecord::Base
  belongs_to :company
  belongs_to :user
  has_many :products, dependent: :destroy
  accepts_nested_attributes_for :products, reject_if: :reject_products, allow_destroy: true

  def reject_products(attributed)
    attributed['product_code'].blank?
  end
end

class Product < ActiveRecord::Base
  belongs_to :job_sheet
end

class User < ActiveRecord::Base
  has_secure_password
  has_many :job_sheets
end

I want a company to have many job_sheets and a job_sheet to belong to a company.
I want a user to have many job sheets and a job sheet to belong to a user.

I want the user to only see the job sheets they have created.

This is my controller for job_sheets, is this also correct or is there a better way ?

  before_filter :authorize

  def index
    @company = find_company
    @job_sheets = @company.job_sheets.where(user: current_user)
  end

  def new
    @company = find_company
    @job_sheet = @company.job_sheets.new
    @job_sheet.products.new
    @job_sheet.user = current_user
  end

  def create
    @company = find_company
    @job_sheet = @company.job_sheets.new(job_sheet_params)
    @job_sheet.user = current_user
    @job_sheet.save
    redirect_to company_job_sheets_path(@company)
  end

  def show
    @company = find_company
    @job_sheet = find_job_sheet(@company)
  end

  def edit
    @company = find_company
    @job_sheet = find_job_sheet(@company)
  end

  def update
    @company = find_company
    @job_sheet = find_job_sheet(@company)
    @job_sheet.update_attributes(job_sheet_params)
    redirect_to company_job_sheets_path(@company)
  end

  def destroy
    @company = find_company
    @job_sheet = find_job_sheet(@company)
    @job_sheet.destroy
    respond_to do |format|
      format.html { redirect_to company_job_sheets_path(@company) }
      format.js
    end
  end



  private

  def find_company
    Company.find(params[:company_id])
  end

  def find_job_sheet(company)
    company.job_sheets.find(params[:id])
  end
end

Sorry it’s so long any help will be much appreciated.

2

I can’t find another way

3

Looks pretty reasonable to me.

One slight improvement: define a scope on JobSheet for getting just those for a given user, like this:

class JobSheet < ActiveRecord::Base
  def self.for_user(user)
    where(user: user)
  end
end

Then call that in your controller (like in your index action).

4

Another tweak:

def new
  @company = find_company
  @job_sheet = @company.job_sheets.new(user: current_user) # Set user here
  @job_sheet.products.new # Are you sure you need this line?
end

Also, your create action doesn’t handle what happens if save fails.

Also, I think show is cleaner this way:

def show
  @company = find_company
  @job_sheet = JobSheet.find(params[:id]) # Don't think you need to scope this based on Company
end
5

@benorenstein Thank you for your feedback much appreciated!

Would I need to do JobSheet.find(params[:id]) in my edit action too?

Am I right in saying that I only need to go through @company when creating a new record and creating/updating one?

6

I would just get the JobSheet directly in the edit action too, yes.

You don’t need to go through @company, it’s just one option. This would also work:

@job_sheet = JobSheet.new(company: @company)

Doing @company.job_sheets.new is just another way of accomplishing the same thing–they both just set company_id in JobSheet. Either works, really.