Best practice method - creating 2 records in 1 form_for

Question

My models:

brand.rb

  has_many :products
  has_many :votes
  belongs_to :user
  accepts_nested_attributes_for :products, :allow_destroy => true

product.rb

  belongs_to :user
  belongs_to :brand

vote.rb

  belongs_to :brand
  belongs_to :user

routes.rb

  resources :brands do
    resources :products
  end

My goal: Create 2 records (product and vote) on existing Brand record in 1 form, on brand/show page.

My solution:

brand/show.html.erb

<% form_for([@brand, @brand.send(:product).klass.new]) do |f| %>
  <%= f.label :title %>
  <%= f.text_field :title %>

  <%= f.label :price %>
  <%= f.text_field :price %>

  <%= fields_for :votes, @brand.votes.new do |builder| %>
    <%= builder.label :rating %>
    <%= builder.text_field :rating %>
  <% end %>

  <%= f.submit %>
<% end %>

products_controller.rb

def create
  if Brand.exists?(:id => params[:brand_id])
    @review          = Review.new(review_params)
    @vote            = Vote.new(votes_params)
    @review.user_id  = @vote.user_id = current_user.id
    @review.brand_id = @vote.brands_id = params[:brand_id]

    if @vote.valid? && @review.valid?
      @vote.save
      @review.save
      redirect_to brands_path
    else
      flash[:errors][:vote]   = @vote.errors
      flash[:errors][:review] = @review.errors
      redirect_to brands_path
    end
  end
end    

private
def product_params
  params.require(:review).permit(:title, :price)
end    

def votes_params
  params.require(:votes).permit(:rating)
end

Is this right way of solving my task? Can I use it like that?

Answer 1

This is how I would refactor your create method:

def create
  brand = Brand.find(params[:brand_id]) # no test to know if Brand exists, if it does not it means the user gave a wrong Brand id, then a 404 error should be rendered
  @product = brand.products.create(products_params.merge({user_id: current_user.id}) # we can directly create this instance
  @vote    = brand.votes.create(votes_params) # we can directly create this instance
  # we add the errors in the flash if they exist
  flash[:errors][:vote]   = @vote.errors if @vote.errors.present?
  flash[:errors][:product] = @product.errors if @product.errors.present?

  redirect_to brands_path # since we want to redirect to brands_path if the creation succeeded or failed, we don't need to use it twice in the code
end 

Also, little improvements:

@brand.send(:product).klass.new
# can become
@brand.products.new # produces a initialized Product instance

fields_for :votes, @brand.votes.new
# can become
f.fields_for @brand.votes.new
# notice the usage of `f` builder to generate the fields_for
# it will nest the params in `params[:brand][:votes_attributes][0]`

# brand.rb
accepts_nested_attributes_for :products, :allow_destroy => true
# add the following:
accepts_nested_attributes_for :votes, :allow_destroy => true

You will obviously have to update your strong params accordingly, but this is the easy part ;-)

Answer 2

I'd change the following logic:

@review.user_id = @vote.user_id = current_user.id
@review.server_id = @vote.server_id = params[:server_id]

Just add the current_user.id and params[:server_id] to the product_params and votes_params respectively.

Also, don't see the need for using instance variables for vote/review.

Other than that the saving two models seems OK to me.