Refactor user controller, create action, mostly.

The gist of the commit are a few improvements in the
create action, where:

* long boolean statemenst have been wrapped in smaller more readable
  methods.
* the 3rd party user info creation has been extracted (still in controller)
* a small helper method for creating a new user from params (to reduce
  visual clutter)
* specs have been added where I came across untested methods/branches

Other changes are more trivial like formatting and whitespace fixes.
Hope this helps. Regards.

app/controllers/users_controller.rb

@@ -140,83 +140,83 @@ class UsersController < ApplicationController
 
   def create
 
-    if params[:password_confirmation] != honeypot_value || params[:challenge] != challenge_value.try(:reverse)
+    if honeypot_or_challenge_fails?(params)
       # Don't give any indication that we caught you in the honeypot
-      return render(json: {success: true, active: false, message: I18n.t("login.activate_email", email: params[:email]) })
+      honey_pot_response = {
+        success: true,
+        active: false,
+        message: I18n.t("login.activate_email", email: params[:email])
+      }
+      return render(json: honey_pot_response)
     end
 
-    user = User.new
+    user = User.new_from_params(params)
-    user.name = params[:name]
-    user.email = params[:email]
-    user.password = params[:password]
-    user.username = params[:username]
 
     auth = session[:authentication]
-    if auth && auth[:email] == params[:email] && auth[:email_valid]
+    if valid_session_authentication?(auth, params[:email])
       user.active = true
     end
     user.password_required! unless auth
 
-    DiscourseHub.register_nickname( user.username, user.email ) if user.valid? && SiteSetting.call_discourse_hub?
+    if user.valid? && SiteSetting.call_discourse_hub?
+      DiscourseHub.register_nickname(user.username, user.email)
+    end
 
     if user.save
-
       msg = nil
-      active_result = user.active?
+      active_user = user.active?
-      if active_result
 
+      if active_user
         # If the user is active (remote authorized email)
         if SiteSetting.must_approve_users?
           msg = I18n.t("login.wait_approval")
-          active_result = false
+          active_user = false
         else
           log_on_user(user)
           user.enqueue_welcome_message('welcome_user')
           msg = I18n.t("login.active")
         end
-
       else
         msg = I18n.t("login.activate_email", email: user.email)
-        Jobs.enqueue(:user_email, type: :signup, user_id: user.id, email_token: user.email_tokens.first.token)
+        Jobs.enqueue(
-      end
+          :user_email, type: :signup, user_id: user.id,
-
+          email_token: user.email_tokens.first.token
-      # Create auth records
+        )
-      if auth.present?
-        if auth[:twitter_user_id] && auth[:twitter_screen_name] && TwitterUserInfo.find_by_twitter_user_id(auth[:twitter_user_id]).nil?
-          TwitterUserInfo.create(user_id: user.id, screen_name: auth[:twitter_screen_name], twitter_user_id: auth[:twitter_user_id])
-        end
-
-        if auth[:facebook].present? && FacebookUserInfo.find_by_facebook_user_id(auth[:facebook][:facebook_user_id]).nil?
-          FacebookUserInfo.create!(auth[:facebook].merge(user_id: user.id))
-        end
-
-        if auth[:github_user_id] && auth[:github_screen_name] && GithubUserInfo.find_by_github_user_id(auth[:github_user_id]).nil?
-          GithubUserInfo.create(user_id: user.id, screen_name: auth[:github_screen_name], github_user_id: auth[:github_user_id])
-        end
       end
 
+      # Create 3rd party auth records (Twitter, Facebook, GitHub)
+      create_third_party_auth_records(user, auth) if auth.present?
 
       # Clear authentication session.
       session[:authentication] = nil
 
       # JSON result
-      render json: {success: true, active: active_result, message: msg }
+      render json: { success: true, active: active_user, message: msg }
     else
-      render json: {success: false, message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n"))}
+      render json: {
+        success: false,
+        message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n"))
+      }
     end
   rescue ActiveRecord::StatementInvalid
-    render json: {success: false, message: I18n.t("login.something_already_taken")}
+    render json: { success: false, message: I18n.t("login.something_already_taken") }
   rescue DiscourseHub::NicknameUnavailable
-    render json: {success: false, message: I18n.t("login.errors", errors:I18n.t("login.not_available", suggestion: User.suggest_username(params[:username])) )}
+    render json: { success: false,
+      message: I18n.t(
+        "login.errors",
+        errors:I18n.t(
+          "login.not_available", suggestion: User.suggest_username(params[:username])
+        )
+      )
+    }
   rescue RestClient::Forbidden
-    render json: {errors: [I18n.t("discourse_hub.access_token_problem")]}
+    render json: { errors: [I18n.t("discourse_hub.access_token_problem")] }
   end
 
   def get_honeypot_value
     render json: {value: honeypot_value, challenge: challenge_value}
   end
 
-
   # all avatars are funneled through here
   def avatar
 
@@ -224,13 +224,10 @@ class UsersController < ApplicationController
     # raise ActiveRecord::RecordNotFound
 
     user = User.select(:email).where(username_lower: params[:username].downcase).first
-    if user
+    if user.present?
-      # for now we only support gravatar in square (redirect cached for a day), later we can use x-sendfile and/or a cdn to serve local
+      # for now we only support gravatar in square (redirect cached for a day),
-      size = params[:size].to_i
+      # later we can use x-sendfile and/or a cdn to serve local
-      size = 64 if size == 0
+      size = determine_avatar_size(params[:size])
-      size = 10 if size < 10
-      size = 128 if size > 128
-
       url = user.avatar_template.gsub("{size}", size.to_s)
       expires_in 1.day
       redirect_to url
@@ -270,14 +267,18 @@ class UsersController < ApplicationController
     guardian.ensure_can_edit!(user)
 
     # Raise an error if the email is already in use
-    raise Discourse::InvalidParameters.new(:email) if User.where("lower(email) = ?", params[:email].downcase).exists?
+    if User.where("lower(email) = ?", params[:email].downcase).exists?
+      raise Discourse::InvalidParameters.new(:email)
+    end
 
     email_token = user.email_tokens.create(email: params[:email])
-    Jobs.enqueue(:user_email,
+    Jobs.enqueue(
-                 to_address: params[:email],
+      :user_email,
-                 type: :authorize_email,
+      to_address: params[:email],
-                 user_id: user.id,
+      type: :authorize_email,
-                 email_token: email_token.token)
+      user_id: user.id,
+      email_token: email_token.token
+    )
 
     render nothing: true
   end
@@ -327,8 +328,8 @@ class UsersController < ApplicationController
 
     results = UserSearch.search term, topic_id
 
-    render json: { users: results.as_json( only:    [ :username, :name ],
+    render json: { users: results.as_json(only: [ :username, :name ],
-                                           methods: :avatar_template ) }
+                                          methods: :avatar_template) }
   end
 
   private
@@ -351,4 +352,58 @@ class UsersController < ApplicationController
       guardian.ensure_can_see!(user)
       user
     end
+
+    def honeypot_or_challenge_fails?(params)
+      params[:password_confirmation] != honeypot_value ||
+      params[:challenge] != challenge_value.try(:reverse)
+    end
+
+    def valid_session_authentication?(auth, email)
+      auth && auth[:email] == email && auth[:email_valid]
+    end
+
+    def create_third_party_auth_records(user, auth)
+      if twitter_auth?(auth)
+        TwitterUserInfo.create(
+          user_id: user.id,
+          screen_name: auth[:twitter_screen_name],
+          twitter_user_id: auth[:twitter_user_id]
+        )
+      end
+
+      if facebook_auth?(auth)
+        FacebookUserInfo.create!(auth[:facebook].merge(user_id: user.id))
+      end
+
+      if github_auth?(auth)
+        GithubUserInfo.create(
+          user_id: user.id,
+          screen_name: auth[:github_screen_name],
+          github_user_id: auth[:github_user_id]
+        )
+      end
+    end
+
+    def twitter_auth?(auth)
+      auth[:twitter_user_id] && auth[:twitter_screen_name] &&
+      TwitterUserInfo.find_by_twitter_user_id(auth[:twitter_user_id]).nil?
+    end
+
+    def facebook_auth?(auth)
+      auth[:facebook].present? &&
+      FacebookUserInfo.find_by_facebook_user_id(auth[:facebook][:facebook_user_id]).nil?
+    end
+
+    def github_auth?(auth)
+      auth[:github_user_id] && auth[:github_screen_name] &&
+      GithubUserInfo.find_by_github_user_id(auth[:github_user_id]).nil?
+    end
+
+    def determine_avatar_size(size)
+      size = size.to_i
+      size = 64 if size == 0
+      size = 10 if size < 10
+      size = 128 if size > 128
+      size
+    end
 end

app/models/user.rb

@@ -102,6 +102,15 @@ class User < ActiveRecord::Base
     find_available_username_based_on(name)
   end
 
+  def self.new_from_params(params)
+    user = User.new
+    user.name = params[:name]
+    user.email = params[:email]
+    user.password = params[:password]
+    user.username = params[:username]
+    user
+  end
+
   def self.create_for_email(email, opts={})
     username = suggest_username(email)
 

spec/controllers/users_controller_spec.rb

@@ -277,12 +277,14 @@ describe UsersController do
     context 'when creating a non active user (unconfirmed email)' do
       it 'should enqueue a signup email' do
         Jobs.expects(:enqueue).with(:user_email, has_entries(type: :signup))
-        xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email
+        xhr :post, :create, name: @user.name, username: @user.username,
+                            password: "strongpassword", email: @user.email
       end
 
       it "doesn't send a welcome email" do
         User.any_instance.expects(:enqueue_welcome_message).with('welcome_user').never
-        xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email
+        xhr :post, :create, name: @user.name, username: @user.username,
+                            password: "strongpassword", email: @user.email
       end
     end
 
@@ -294,7 +296,8 @@ describe UsersController do
 
       it 'should enqueue a signup email' do
         User.any_instance.expects(:enqueue_welcome_message).with('welcome_user')
-        xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email
+        xhr :post, :create, name: @user.name, username: @user.username,
+                            password: "strongpassword", email: @user.email
       end
 
       it "should be logged in" do
@@ -309,6 +312,7 @@ describe UsersController do
         ::JSON.parse(response.body)['active'].should == true
       end
 
+
       context 'when approving of users is required' do
         before do
           SiteSetting.expects(:must_approve_users).returns(true)
@@ -322,14 +326,51 @@ describe UsersController do
         it "doesn't return active in the JSON" do
           ::JSON.parse(response.body)['active'].should == false
         end
-
       end
 
+      context 'authentication records for' do
+
+        before do
+          SiteSetting.expects(:must_approve_users).returns(true)
+        end
+
+        it 'should create twitter user info if none exists' do
+          twitter_auth = { twitter_user_id: 42, twitter_screen_name: "bruce" }
+          session[:authentication] = twitter_auth
+          TwitterUserInfo.expects(:find_by_twitter_user_id).returns(nil)
+          TwitterUserInfo.expects(:create)
+
+          xhr :post, :create, name: @user.name, username: @user.username,
+            password: "strongpassword", email: @user.email
+        end
+
+        it 'should create facebook user info if none exists' do
+          fb_auth = { facebook: { facebook_user_id: 42} }
+          session[:authentication] = fb_auth
+          FacebookUserInfo.expects(:find_by_facebook_user_id).returns(nil)
+          FacebookUserInfo.expects(:create!)
+
+          xhr :post, :create, name: @user.name, username: @user.username,
+                              password: "strongpassword", email: @user.email
+        end
+
+        it 'should create github user info if none exists' do
+          gh_auth = { github_user_id: 2, github_screen_name: "bruce" }
+          session[:authentication] = gh_auth
+          GithubUserInfo.expects(:find_by_github_user_id).returns(nil)
+          GithubUserInfo.expects(:create)
+
+          xhr :post, :create, name: @user.name, username: @user.username,
+                              password: "strongpassword", email: @user.email
+        end
+
+      end
     end
 
     context 'after success' do
       before do
-        xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email
+        xhr :post, :create, name: @user.name, username: @user.username,
+                            password: "strongpassword", email: @user.email
       end
 
       it 'should succeed' do
@@ -403,13 +444,22 @@ describe UsersController do
       it_should_behave_like 'failed signup'
     end
 
-    context 'when InvalidStatement is raised' do
+    context 'when an Exception is raised' do
-      before do
+
-        User.any_instance.stubs(:save).raises(ActiveRecord::StatementInvalid)
+      [ ActiveRecord::StatementInvalid,
+        DiscourseHub::NicknameUnavailable,
+        RestClient::Forbidden ].each do |exception|
+        before { User.any_instance.stubs(:save).raises(exception) }
+
+        let(:create_params) {
+          { name: @user.name, username: @user.username,
+            password: "strongpassword", email: @user.email}
+        }
+
+        it_should_behave_like 'failed signup'
       end
-      let(:create_params) { {name: @user.name, username: @user.username, password: "strongpassword", email: @user.email} }
-      it_should_behave_like 'failed signup'
     end
+
   end
 
   context '.username' do
@@ -704,7 +754,9 @@ describe UsersController do
 
     context 'not logged in' do
       it 'raises an error when not logged in' do
-        lambda { xhr :put, :update, username: 'somename' }.should raise_error(Discourse::NotLoggedIn)
+        expect do
+          xhr :put, :update, username: 'somename'
+        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
 
@@ -735,7 +787,6 @@ describe UsersController do
         end
       end
     end
-
   end
 
   describe "search_users" do