Merge pull request #1581 from railsaholic/salvage_users_controller_refactoring_from_pr_1499

User ctrl refactor - breaks up large methods, moves some logic into model

app/controllers/users_controller.rb

@@ -40,7 +40,7 @@ class UsersController < ApplicationController
   end
 
   def update
-    user = User.where(username_lower: params[:username].downcase).first
+    user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
     json_result(user, serializer: UserSerializer) do |u|
       updater = UserUpdater.new(user)
@@ -127,7 +127,6 @@ class UsersController < ApplicationController
     params[:for_user_id] ? User.find(params[:for_user_id]) : current_user
   end
 
-
   def create
     return fake_success_response if suspicious? params
 
@@ -157,24 +156,28 @@ class UsersController < ApplicationController
     if @user.blank?
       flash[:error] = I18n.t('password_reset.no_token')
     else
-      if request.put? && params[:password].present?
+      raise Discourse::InvalidParameters.new(:password) unless good_reset_request_format
-        @user.password = params[:password]
+      @user.password = params[:password]
-        if @user.save
+      logon_after_password_reset if @user.save
-
-          if Guardian.new(@user).can_access_forum?
-            # Log in the user
-            log_on_user(@user)
-            flash[:success] = I18n.t('password_reset.success')
-          else
-            @requires_approval = true
-            flash[:success] = I18n.t('password_reset.success_unapproved')
-          end
-        end
-      end
     end
     render layout: 'no_js'
   end
 
+  def good_reset_request_format
+    request.put? && params[:password].present?
+  end
+
+  def logon_after_password_reset
+    if Guardian.new(@user).can_access_forum?
+      # Log in the user
+      log_on_user(@user)
+      flash[:success] = I18n.t('password_reset.success')
+    else
+      @requires_approval = true
+      flash[:success] = I18n.t('password_reset.success_unapproved')
+    end
+  end
+
   def change_email
     params.require(:email)
     user = fetch_user_from_params
@@ -229,13 +232,15 @@ class UsersController < ApplicationController
   def send_activation_email
     @user = fetch_user_from_params
     @email_token = @user.email_tokens.unconfirmed.active.first
-    if @user
+    enqueue_activation_email if @user
-      @email_token ||= @user.email_tokens.create(email: @user.email)
-      Jobs.enqueue(:user_email, type: :signup, user_id: @user.id, email_token: @email_token.token)
-    end
     render nothing: true
   end
 
+  def enqueue_activation_email
+    @email_token ||= @user.email_tokens.create(email: @user.email)
+    Jobs.enqueue(:user_email, type: :signup, user_id: @user.id, email_token: @email_token.token)
+  end
+
   def search_users
     term = params[:term].to_s.strip
     topic_id = params[:topic_id]
@@ -289,21 +294,18 @@ class UsersController < ApplicationController
     end
 
     # check the file size (note: this might also be done in the web server)
-    filesize ||= File.size(file.tempfile)
+    filesize  ||= File.size(file.tempfile)
     max_size_kb = SiteSetting.max_image_size_kb * 1024
-
     if filesize > max_size_kb
-      return render status: 413,
+      return render status: 413, text: I18n.t("upload.images.too_large", max_size_kb: max_size_kb)
-                    text: I18n.t("upload.images.too_large",
+    else
-                                  max_size_kb: max_size_kb)
+      filesize
     end
 
-    unless SiteSetting.authorized_image?(file)
+    return render status: 422, text: I18n.t("upload.images.unknown_image_type") unless SiteSetting.authorized_image?(file)
-      return render status: 422, text: I18n.t("upload.images.unknown_image_type")
-    end
 
     upload = Upload.create_for(user.id, file, filesize)
-    user.update_avatar(upload)
+    user.upload_avatar(upload)
 
     Jobs.enqueue(:generate_avatars, user_id: user.id, upload_id: upload.id)
 

app/models/user.rb

@@ -473,9 +473,9 @@ class User < ActiveRecord::Base
     created_at > 1.day.ago
   end
 
-  def update_avatar(upload)
+  def upload_avatar(avatar)
     self.uploaded_avatar_template = nil
-    self.uploaded_avatar = upload
+    self.uploaded_avatar = avatar
     self.use_uploaded_avatar = true
     self.save!
   end
@@ -574,7 +574,6 @@ class User < ActiveRecord::Base
     end
   end
 
-
   private
 
   def previous_visit_at_update_required?(timestamp)

spec/controllers/users_controller_spec.rb

@@ -249,7 +249,7 @@ describe UsersController do
     context 'valid token' do
       before do
         EmailToken.expects(:confirm).with('asdfasdf').returns(user)
-        get :password_reset, token: 'asdfasdf'
+        put :password_reset, token: 'asdfasdf', password: 'newpassword'
       end
 
       it 'returns success' do

spec/models/user_spec.rb

@@ -826,12 +826,12 @@ describe User do
     end
   end
 
-  describe "#update_avatar" do
+  describe "#upload_avatar" do
     let(:upload) { Fabricate(:upload) }
     let(:user)   { Fabricate(:user) }
 
-    it "should update use's avatar" do
+    it "should update user's avatar" do
-      expect(user.update_avatar(upload)).to be_true
+      expect(user.upload_avatar(upload)).to be_true
     end
   end