class PeopleController < ApplicationController
before_filter :clean_attributes, :only => [:create, :update]
# show the form to create a new user
def new
if flash[:user_with_errors]
@user = flash[:user_with_errors]
else
@user = User.new
end
show_user_form('add user', 'create')
end
# show the form to edit an existing user
def edit
if flash[:user_with_errors]
@user = flash[:user_with_errors]
else
@user = get_user(params[:id])
end
if @user
show_user_form('edit user', 'update')
else
flash[:error] = 'user not found'
redirect_to(root_path)
end
end
# this is the target of the 'new' action
def create
if request.post?
user = User.new
save_user(user)
redirect_to(:action => 'new')
end
end
# this is the target of the 'edit' action
def update
if request.post?
if user = get_user(params[:id])
save_user(user)
redirect_to(:action => 'edit', :id => params[:id])
else
flash[:error] = 'user not found'
redirect_to(root_path)
end
end
end
private
# before filter to remove unwanted whitespace
def clean_attributes
begin
[:forename, :surname, :email].each do |attribute|
params[:user][attribute].squish! if params[:user][attribute]
end
rescue
end
end
# show the user form (for new or edit actions)
def show_user_form(title, action)
@form_title = title
@action = action
render :action => 'user_form'
end
# save submitted user details
def save_user(user)
user.attributes = params[:user]
# set administrator only attributes
if current_user_is_admin?
user.login = params[:user][:login]
end
if user.save
flash[:success] = 'user saved successfully'
else
# blank out password for redisplay
user.password = nil
user.password_confirmation = nil
flash[:error] = 'save failed'
flash[:user_with_errors] = user
end
end
# get the user object
def get_user(id)
begin
user = User.find(id)
rescue ActiveRecord::RecordNotFound
logger.error("Attempt to access user id that doesn't exist (id: #{id})")
end
user
end
end
Refactorings
No refactoring yet !
Adam
November 5, 2008, November 05, 2008 15:56, permalink
Sometimes it is just a good idea to be explicit instead of DRY. I feel this is one of those times.
class UsersController < ApplicationController
def new
@user = User.new
end
def edit
@user = User.find(params[:id])
end
def create
@user = User.new(params[:user])
@user.login = params[:user][:login] if admin?
if @user.save
flash[:success] = 'user saved successfully'
redirect_to users_url
else
flash[:error] = 'save failed'
render :action => 'new'
end
end
def update
@user = User.find(params[:id])
@user.login = params[:user][:login] if admin?
if @user.update_attributes(params[:user])
flash[:success] = 'user saved successfully'
redirect_to users_url
else
flash[:error] = 'save failed'
render :action => 'edit'
end
end
end
class User < ActiveRecord::Base
before_save :squish_attributes
protected
def squish_attributes
attributes.each do |key,value|
value.squish! if value.respond_to?(:squish!)
end
end
end
Dave
November 6, 2008, November 06, 2008 07:56, permalink
Hi Adam,
thanks very much for your refactoring. I like the idea of moving the 'clean_attributes' method to the model, however I've changed the filter from 'before_save' to 'before_validation'. This is because one of the attributes in my model is invalid if it contains whitespace characters, and the before_save filter doesn't fire if validation has failed.
I also like your method of 'squishing' every attribute, however there are some attributes where leading or trailing whitespace is allowed, like the password for example, so I've kept the array of attributes (instead of attributes.each, although I didn't know you could do that so that's useful to know!). Is there any advantage in using one of the following over the other?
value.squish! if value.respond_to?(:squish!)
value.squish! unless value.nil?
I still want to share the user form template between the two actions, as they only differ in title and form action. The show_user_form method does this, although it still doesn't seem very elegant (not sure why). Perhaps the user form is used for editing a user by default, but can be used for a new user too, with something like this perhaps?
<% form_for(:user, :url => { :action => @action || 'update', :id => params[:id] } ) do |form| %> etc.
Cheers
Dave
class UsersController < ApplicationController ## Controller [ruby_on_rails]
def new
@user = User.new
show_user_form('new user', 'create')
end
def edit
@user = User.find(params[:id])
show_user_form('edit user', 'update')
end
def create
@user = User.new(params[:user])
@user.login = params[:user][:login] if admin?
if @user.save
flash[:success] = 'user saved successfully'
redirect_to users_url
else
flash[:error] = 'save failed'
show_user_form('new user', 'create')
end
end
def update
@user = User.find(params[:id])
@user.login = params[:user][:login] if admin?
if @user.update_attributes(params[:user])
flash[:success] = 'user saved successfully'
redirect_to users_url
else
flash[:error] = 'save failed'
render :action => 'edit'
show_user_form('edit user', 'update')
end
end
private
# show the user form (for new or edit actions)
def show_user_form(title, action)
@form_title = title
@action = action
render :template => 'user_form'
end
end
class User < ActiveRecord::Base ## Model [ruby_on_rails]
before_validation :squish_attributes
private
def squish_attributes
[:forename, :surname, :email].each do |attribute|
attribute.squish! if attribute.respond_to?(:squish!)
end
end
end
Adam
November 12, 2008, November 12, 2008 16:51, permalink
I used respond_to? because squish! is not a built in type and thus I wouldn't know if each object would have the method available. If you've made it available to every kind of object that could be passed in there, then you don't need to worry about the check.
If the form is shared, do this in your form. form_for knows which action it should point to based on the user instance.
<% form_for @user do |f| %> <% end %>
Hello,
I have a 'people' controller to control user accounts. I want to share as much code as possible between the 'new' and 'edit' actions. I've tried to DRY it up as much as I can but in doing that I fear I might have made it more complicated. In the controller code below, the two methods 'new' and 'edit' both display a template 'user_form.html.erb', where title and form action are changed accordingly. The 'new' method submits to 'create', and the 'edit' method submits to 'update'. Errors are redisplayed via saving the user object with errors in the hash.
I would be very grateful for and refactoring suggestions anyone might have!
Many thanks,
Dave