I have two models item and user_item. Item has many user_items and user_items belongs to item. I have a form where a user can create a new item. In the form a user should include a picture. The name, description, and tags get saved to a new item object. The picture should be saved as an attribute on a user_item object which gets created at the same time.
I've been reading about nested model forms and strong parameters and using accepts_nested_attributes_for. My form seems to be working but I don't understand why I don't need accepts_nested_attributes_for. Although it seems to be working, is there a problem with the way I'm doing it?
Form
<%= simple_form_for @item, url: items_path, method: :post do |item_builder| %>
<div class="well">
<%= item_builder.input :name, required: false, error: false, label: "Item name" %>
<%= item_builder.input :description, as: :text, required: false, error: false, label: "Describe item" %>
<%= item_builder.input :tag_list, required: false, label: "Tags" %>
<%= item_builder.simple_fields_for @item, @item.user_items.build do |user_item_builder| %>
<%= user_item_builder.input :picture, as: :file, required: false, label: "Picture of you with this item" %>
<% end %>
</div>
<div class="clearfix">
<%= item_builder.submit 'Submit new item', class: "btn btn-primary pull-right inherit-width" %>
</div>
<% end %>
Items_controller
def create
Item.transaction do
@item = Item.create(name: item_params[:name],
description: item_params[:description],
tag_list: item_params[:tag_list],
created_by: current_user.id,
status: Item::STATUS[:pending])
if item_params[:item] == nil
@item.errors.add(:picture, "is required")
end
if @item.errors.empty?
@user_item = @item.user_items.build(user_id: current_user.id, picture: item_params[:item][:picture])
if @user_item.save
flash[:notice] = "Thank you for your item submission."
redirect_to items_path
else
render :new
raise ActiveRecord::Rollback, "Useritem create failed"
end
else
render :new
raise ActiveRecord::Rollback, "no picture"
end
end
end
private
def item_params
params.require(:item).permit(:name, :description, :tag_list,
item: :picture)
end
EDIT:
I have made changes going along with Richard's answer below but I am now running into a couple problems. This is my current create action in the items_controller
def create
@item= Item.new item_params
@item.status = Item::STATUS[:pending]
@item.created_by = current_user.id
@item.user_items.first.user_id = current_user.id
if @item.save
redirect_to items_path, notice: "Thank you for your item request!
else
render :new
end
end
On validation fail I was getting a missing template error so I added an else and render :new.
Now on validation fail the new template is rendered but the fields_for input for picture is not shown.
I have validates_presence_of :picture in user_item.rb and I have
validates_associated :user_items in item.rb but I am still able to submit the form without a picture.
As you can see on valid submission I need the item's status attribute to be pending and a created_by attribute to be the current user id. I also needed the user_item's user_id attribute to be set to the current user id. I set this in the controller. I am wondering if setting this like I am in the create action is the correct way.
You don't need it because your pattern is wrong:
@user_item = @item.user_items.build(user_id: current_user.id, picture: item_params[:item][:picture])
With your above code, you're creating individual user_items from each @item. This does not require accepts_nested_attributes_for, but is cumbersome, against convention and very hacky.
Here's how it should be done:
Models
#app/models/item.rb
class Item < ActiveRecord::Base
has_many :user_items
has_many :users, through: :user_items
accepts_nested_attributes_for :user_items
end
#app/models/user_item.rb
class UserItem < ActiveRecord::Base
belongs_to :user
belongs_to :item
end
Controllers
#app/controllers/items_controller.rb
class ItemsController < ApplicationController
def new
@item = Item.new
@item.user_items.build
end
def create
@item = Item.new item_params
redirect_to @item, notice: "Thank you for your item submission." if @item.save
end
private
def item_params
params.require(:item).permit(:name, :description, :tag_list, user_items_attributes: [:picture])
end
end
Views
#app/views/items/new.html.erb
<%= simple_form_for @item do |item_builder| %>
<%= item_builder.input :name, required: false, error: false, label: "Item name" %>
<%= item_builder.input :description, as: :text, required: false, error: false, label: "Describe item" %>
<%= item_builder.input :tag_list, required: false, label: "Tags" %>
<%= item_builder.simple_fields_for :user_items do |user_item_builder| %>
<%= user_item_builder.input :picture, as: :file, required: false, label: "Picture of you with this item" %>
<% end %>
<%= item_builder.submit 'Submit new item', class: "btn btn-primary pull-right inherit-width" %>
<% end %>
is there a problem with the way I'm doing it
Technically, no.
However, if you aspire to be a professional, or on a similar level, you'll not get very far with the code you wrote.
Using a framework like Rails gives you access to an unprecedented array of pre-baked functionality. The best code is not the code you wrote, but the library code that's been compiled & tested over years of production use.
Whilst your code works, it's not efficient nor extensible.
--
The ultimate key of whether what you've written is "right" is whether the future you would be proud of looking at it again. If not, you're probably best refactoring.
Update
If you want to give Item a status, you'll want to look at the enum module for ActiveRecord models:
#app/models/item.rb
class Item < ActiveRecord::Base
enum status: [:active, :pending, :declined]
end
This is a very interesting method, as it provides a series of class methods (scopes), and instance methods:
@item = Item.find x
@item.active? #-> true
@item.pending? #-> false
@item.declined? #-> false
Item.active #-> collection of "active" items
Item.pending #-> collection of "pending" items
Item.declined #-> collection of "declined" items
To save an Item's status, you could use the collection_select as so:
<%= form_for @item do |f| %>
<%= f.collection_select :status, Item.statuses, :first, :first %>
<%= f.submit %>
<% end %>
Update
Your code can be improved massively:
def create
@item= Item.new item_params #-> this line should do ALL the heavy lifting.
if @item.save
redirect_to items_path, notice: "Thank you for your item request!"
else
render :new
end
end
private
def item_params
params.require(:item).permit(:name, :description, :tag_list, user_items_attributes: [:picture]).merge(created_by: current_user.id)
end
The file field won't be populated again if you have validation issues; it's an OS problem, not Rails (how does the OS know your file is in the same location as it was when you first submitted?).
You need to make your create code as succinct as possible; explicit declarations for attributes is generally a bad idea. You should put as much of it as possible into the Item model (enum etc).
Although it seems to be working, is there a problem with the way I'm
doing it?
Its great that it works - however it does have a code smell. Your controller has way to much business logic which belongs on the model layer and you would be forced to repeat that whole mess with the param checking in your update action.
So lets look at the Rails way:
class Item
enum status: [:pending, :approved, :awesome] # or whatever statuses you have
has_many :user_items
belongs_to :creator, class_name: 'User' # or author or whatever
validates_associated :user_items
accepts_nested_attributes_for :user_items, reject_if: :all_blank?
end
class UserItem
belongs_to :item
validates :picture, presence: true
end
class User
has_many :items, source: :creator, inverse_of: :creator
end
Here we setup item to accepts_nested_attributes_for :user_items and we setup a validations for UserItem, validates_associated will cause the Item validation to fail unless all the associated UserItems are valid.
ActiveRecord::Enum is a less hacky way of doing what I believe you are doing with item.status.
With our models taken care of we can proceed to with the controller:
class ItemsController
before_action :set_item, only: [:show, :edit, :update, :destroy]
def new
@item = Item.new
@item.user_items.new # seeds the form
end
def edit
@item.user_items.new unless @item.user_items.any? # seeds the form
end
def create
@item = Item.new(item_params) do |item|
item.status = :pending
item.creator = current_user
end
if @item.save
redirect_to items_path, success: 'Oh yes'
else
render :new, error: 'Oh noes'
end
end
def update
if @item.update(item_params)
redirect_to @item, success: 'Item updated'
else
render :edit, error: 'Oh noes'
end
end
private
def set_item
@item = Item.includes(:user_items).find(params[:id])
end
def item_params
params.require(:item)
.permit(:name, :description, :tag_list, user_items_attributes: [:picture])
end
end
This is MVC the Rails way - models take care of validations and associations. We use strong parameters to map form input to models in a reusable way.
Lastly lets clean up the form. We will start by extracting the form into a partial so that it can be re-used.
<% # views/items/_form.html.erb %>
<%= simple_form_for @item do |item_builder| %>
<div class="well">
<%= item_builder.input :name, require: false, label: "Item name" %>
<%= item_builder.input :description, require: false, as: :text, label: "Describe item" %>
<%= item_builder.input :tag_list, label: "Tags", require: false %>
<%= item_builder.simple_fields_for :user_items do |user_item_builder| %>
<%= user_item_builder.input :picture, as: :file, label: "Picture of you with this item" %>
<% end %>
</div>
<div class="clearfix">
<%= item_builder.submit %>
</div>
<% end %>
We can then use this form for both new and edit.
<h1>Create a new item</h1>
<%= render partial: '_form' %>
<h1>Edit <%= @item.name %></h1>
<%= render partial: '_form' %>
