CODEWITHSUNDEEP
ruby-on-railsruby-on-rails-3ruby-on-rails-4
appleII717
appleII717

Reputation: 368

adding shallow nested associations controller before action

I'm use a short form of shallow routing:

resources :officers, except: :new

resources :members do
  resources :officers, only: :new
end

This is a member has_many officers : officers belong_to member association. Members can fill many officer roles over the years, the reason for has_many.

I've had problems dealing with this for years and decided to ask a question on the preferred Rails way of handling the creation of an associated has_many record.

On the Members show page I have a link:

= link_to 'New Officer', new_member_officer_path(@member)

There is no new_officer_path link in the application, but I wanted to trap someone putting in officers/new in the url. In the Officers _form partial, I set a hidden field member_id to @member.id if its a new record (id.nil?)

My first failed attempt:

def new
  set_member
  @officer = Officer.new(member_id: @member.id)     
end

def set_member
  if params[:member_id]        
    @member = Active.find(params[:member_id])
  else
    redirect_to members_path, alert: "Officers must be created from the Members Show view"   
  end
end

This failed, I guess because you can't redirect from a called method from an action.

I then tried:

before_action :set_member, only: :new

def new
  @officer = Officer.new(member_id: @member.id)     
end

def set_member
  if params[:member_id]        
    @member = Active.find(params[:member_id])
  else
    redirect_to members_path, alert: "Officers must be created from the Members Show view"   
  end
end

This worked in a valid members/xx/officers/new url call but Failed in an on officers/new url. Since there is not a new route, it went to the show action and failed try to find :id new

Removing the only: :new condition on resources officers allow that approach to work, but I have an unneeded route.

Moving stuff around in my first attempt:

def new
  set_member
  if @member
    @officer = Officer.new(member_id: @member.id)
  else
    redirect_to members_path, alert: "Officers must be created from the Members Show view"   
  end
end

def set_member
  if params[:member_id]        
    @member = Active.find(params[:member_id])
  end
end

Works, but for some reason does not feel right. I almost like leaving the new route in and the before_action approach.

Again, just looking for the standard approach. Also, is setting the foreign id in a hidden field the standard approach?

Upvotes: 2

Views: 482

Answers (1)

Farley Knight
Farley Knight

Reputation: 1793

Personally I would write it like this:

def new
  @member = Active.find(params[:member_id]) if params[:member_id].present?
  if @member.present?
    @officer = @member.officers.build
  else
    redirect_to members_path, alert: "Officers must be created from the Members Show view"   
  end
end

Using present? avoids any potential issues with the way Ruby handles nil as false-y. Not a huge concern, but I think it's best practice to return booleans instead of memorizing Ruby's interpretation of objects to boolean.

Using build automatically sets the member_id. And I didn't feel that set_member needed to be it's own method. However, if the logic in that method gets more complicated, I might revert back to using set_member.

Upvotes: 2

Related Questions