class AccountsController < ApplicationController
layout 'application'
before_filter :login_required, :except => :show
# Change password action
def update
return unless request.put?
# we authenticate with email
if Dj.authenticate(current_dj.email, params[:old_password])
if ((params[:password] == params[:password_confirmation]) && !params[:password_confirmation].blank?)
current_dj.password_confirmation = params[:password_confirmation]
current_dj.password = params[:password]
if current_dj.save
flash[:notice] = "Password successfully updated."
redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
else
flash[:error] = "An error occured, your password was not changed."
render :action => 'edit'
end
else
flash[:error] = "New password does not match the password confirmation."
@old_password = params[:old_password]
render :action => 'edit'
end
else
flash[:error] = "Your old password is incorrect."
render :action => 'edit'
end
end
end
Refactorings
No refactoring yet !
David Calavera
July 17, 2008, July 17, 2008 06:41, permalink
2 ratings.
Login to rate!
droping the spaghetti code
class AccountsController < ApplicationController
layout 'application'
before_filter :login_required, :except => :show
# Change password action
def update
return unless request.put?
# we authenticate with email
render_error("Your old password is incorrect.") unless Dj.authenticate(current_dj.email, params[:old_password])
if (params[:password] != params[:password_confirmation] || params[:password_confirmation].blank?)
@old_password = params[:old_password]
render_error("New password does not match the password confirmation.")
end
current_dj.password_confirmation = params[:password_confirmation]
current_dj.password = params[:password]
render_error("An error occured, your password was not changed.") unless current_dj.save
flash[:notice] = "Password successfully updated."
redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
end
def render_error(msg)
flash[:error] = msg
render :action => 'edit'
end
end
rupert
July 17, 2008, July 17, 2008 10:10, permalink
No rating.
Login to rate!
Personally I'd use model validations to do the checking of what the user has subimitted and move the rest of the logic (checking the old password is correct) into a reset_password method on the Dj model to give something like below (untested).
<%= error_messages_for :dj %>
<% form_for :dj, :url => accounts_path(@dj), :html => {:method => :put} do |form| %>
<%= form.password_field :old_password %>
<%= form.password_field :password %>
<%= form.password_field :password_confirmation %>
<%= submit_tag 'save new password' %>
<% end %>
class AccountsController < ApplicationController
layout 'application'
before_filter :login_required, :except => :show
def update
return unless request.put?
current_dj.reset_password(current_dj.email, params[:dj])
flash[:notice] = "Password successfully updated."
redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
rescue ActiveRecord::RecordInvalid
render :action => 'edit'
rescue Dj::AuthenticationFailed
flash[:error] = "Your old password is incorrect."
render :action => 'edit'
end
end
class Dj < ActiveRecordBase
class AuthenticationFailed < RuntimeError ; end
validates_length_of :password, :minimum => 1
validates_confirmation_of :password
def reset_password(email, params)
raise AuthenticationFailed unless Dj.authenticate(email, params[:old_password])
update_attributes!(:password => parms[:password], :password_confirmation => params[:password_confirmation])
end
end
rupert
July 17, 2008, July 17, 2008 10:30, permalink
2 ratings.
Login to rate!
a slight tidyup - in that the email doesn't need to be passed into the reset_password method as the dj already knows his email address. I've also removed the return unless request.put? as this should be enforced by the routing
class AccountsController < ApplicationController
layout 'application'
before_filter :login_required, :except => :show
def update
return unless request.put?
current_dj.reset_password(params[:dj])
flash[:notice] = "Password successfully updated."
redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
rescue ActiveRecord::RecordInvalid
render :action => 'edit'
rescue Dj::AuthenticationFailed
flash[:error] = "Your old password is incorrect."
render :action => 'edit'
end
end
class Dj < ActiveRecordBase
class AuthenticationFailed < RuntimeError ; end
validates_length_of :password, :minimum => 1
validates_confirmation_of :password
def reset_password(params)
raise AuthenticationFailed unless Dj.authenticate(email, params[:old_password])
update_attributes!(:password => parms[:password], :password_confirmation => params[:password_confirmation])
end
end
jamesgolick
July 17, 2008, July 17, 2008 13:15, permalink
3 ratings.
Login to rate!
Managing flow with exceptions sucks. Exceptions are heavy, both from a readability point of view, and from a performance point of view. They should be reserved for unexpected situations, and ones that you want to allow to bubble up the stack and catch later.
What if we used validations to handle the problem? It sure simplifies things.
class Dj < ActiveRecord::Base
attr_accessor :old_password
validates_length_of :password, :minimum => 1
validates_confirmation_of :password
validate_on_update :validate_old_password
def reset_password(attrs)
# I could see plucking out a few specific attributes to update, here, but no matter, if validations are complete
update_attributes attrs
end
protected
def validate_old_password
errors.add(:old_password, 'does not match') if password_changed? && !authenticated?(old_password)
end
end
class AccountsController < ApplicationController
layout 'application'
before_filter :login_required, :except => :show
# Change password action
def update
if current_dj.reset_password(params[:dj])
flash[:notice] = "Password successfully updated."
redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
else
# I don't know if we need this error message anymore, since the validations will take care of it
flash[:error] = "An error occured, your password was not changed."
render :action => 'edit'
end
end
end
Mostly straight from a tutorial, but it's uuuugly: