Redmine Refactor #125: Split UsersController#edit into #edit and #update

Working my way through the UsersController class, the next method that needs to be refactored is #edit. This method has the common problem of both:

  • rendering the edit form
  • also accepting the POST data from the form

To fit the REST pattern, this needs to be split into separate #edit and #update methods.

Before

1
2
3

class UsersController  'users', :action => 'edit', :id => @user
  end
end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41

class UsersController  :put, :only => :update, :render => {:nothing => true, :status => :method_not_allowed }
  def update
    @user = User.find(params[:id])
    @notification_options = @user.valid_notification_options
    @notification_option = @user.mail_notification
 
    @user.admin = params[:user][:admin] if params[:user][:admin]
    @user.login = params[:user][:login] if params[:user][:login]
    if params[:password].present? && (@user.auth_source_id.nil? || params[:user][:auth_source_id].blank?)
      @user.password, @user.password_confirmation = params[:password], params[:password_confirmation]
    end
    @user.group_ids = params[:user][:group_ids] if params[:user][:group_ids]
    @user.attributes = params[:user]
    # Was the account actived ? (do it before User#save clears the change)
    was_activated = (@user.status_change == [User::STATUS_REGISTERED, User::STATUS_ACTIVE])
    # TODO: Similar to My#account
    @user.mail_notification = params[:notification_option] || 'only_my_events'
    @user.pref.attributes = params[:pref]
    @user.pref[:no_self_notified] = (params[:no_self_notified] == '1')
 
    if @user.save
      @user.pref.save
      @user.notified_project_ids = (params[:notification_option] == 'selected' ? params[:notified_project_ids] : [])
 
      if was_activated
        Mailer.deliver_account_activated(@user)
      elsif @user.active? && params[:send_information] && !params[:password].blank? && @user.auth_source_id.nil?
        Mailer.deliver_account_information(@user, params[:password])
      end
      flash[:notice] = l(:notice_successful_update)
      redirect_to :back
    else
      @auth_sources = AuthSource.find(:all)
      @membership ||= Member.new
 
      render :action => :edit
    end
  rescue ::ActionController::RedirectBackError
    redirect_to :controller => 'users', :action => 'edit', :id => @user
  end
end

Like the other split methods refactorings I’ve done, I had to introduce a bit of duplication in both methods to make sure all of the instance variables are setup for the views. Once I’ve completed refactoring all of the controllers to REST, I’ll be refactoring inside of each controller method to remove duplication like this. Until then, I need to live with making some things worse (duplication) in order to make other things better (controller methods).

Reference commit