Refactor SessionController#create, reduce complexity.

Don't compromise readablity

app/controllers/session_controller.rb

@@ -10,49 +10,30 @@ class SessionController < ApplicationController
     params.require(:login)
     params.require(:password)
 
-    login    = params[:login].strip
-    password = params[:password]
-    login    = login[1..-1] if login[0] == "@"
+    login = params[:login].strip
+    login = login[1..-1] if login[0] == "@"
 
-    @user = User.find_by_username_or_email(login)
+    if user = User.find_by_username_or_email(login)
 
-    if @user.present?
-
-      # If the site requires user approval and the user is not approved yet
-      if SiteSetting.must_approve_users? && !@user.approved? && !@user.admin?
-        render json: {error: I18n.t("login.not_approved")}
+      # If their password is correct
+      unless user.confirm_password?(params[:password])
+        invalid_credentials
         return
       end
 
-      # If their password is correct
-      if @user.confirm_password?(password)
-
-        if @user.suspended?
-          if reason = @user.suspend_reason
-            render json: { error: I18n.t("login.suspended_with_reason", {date: I18n.l(@user.suspended_till, format: :date_only), reason: reason}) }
-          else
-            render json: { error: I18n.t("login.suspended", {date: I18n.l(@user.suspended_till, format: :date_only)}) }
-          end
-          return
-        end
-
-        if @user.email_confirmed?
-          log_on_user(@user)
-          render_serialized(@user, UserSerializer)
-          return
-        else
-          render json: {
-            error: I18n.t("login.not_activated"),
-            reason: 'not_activated',
-            sent_to_email: @user.email_logs.where(email_type: 'signup').order('created_at DESC').first.try(:to_address) || @user.email,
-            current_email: @user.email
-          }
-          return
-        end
+      # If the site requires user approval and the user is not approved yet
+      if login_not_approved_for?(user)
+        login_not_approved
+        return
       end
     end
 
-    render json: {error: I18n.t("login.incorrect_username_email_or_password")}
+    if user.suspended?
+      failed_to_login(user)
+      return
+    end
+
+    user.email_confirmed? ? login(user) : not_activated(user)
   end
 
   def forgot_password
@@ -73,4 +54,39 @@ class SessionController < ApplicationController
     render nothing: true
   end
 
+  private
+
+  def login_not_approved_for?(user)
+    SiteSetting.must_approve_users? && !user.approved? && !user.admin?
+  end
+
+  def invalid_credentials
+    render json: {error: I18n.t("login.incorrect_username_email_or_password")}
+  end
+
+  def login_not_approved
+    render json: {error: I18n.t("login.not_approved")}
+  end
+
+  def not_activated(user)
+    render json: {
+      error: I18n.t("login.not_activated"),
+      reason: 'not_activated',
+      sent_to_email: user.find_email || user.email,
+      current_email: user.email
+    }
+  end
+
+  def failed_to_login(user)
+    message = user.suspend_reason ? "login.suspended_with_reason" : "login.suspended"
+
+    render json: { error: I18n.t(message, { date: I18n.l(user.suspended_till, format: :date_only),
+                                            reason: user.suspend_reason}) }
+  end
+
+  def login(user)
+    log_on_user(user)
+    render_serialized(user, UserSerializer)
+  end
+
 end

app/models/email_log.rb

@@ -19,6 +19,11 @@ class EmailLog < ActiveRecord::Base
     EmailLog.where(reply_key: reply_key).first
   end
 
+  def self.last_sent_email_address
+    where(email_type: 'signup').order('created_at DESC')
+                               .first.try(:to_address)
+  end
+
 end
 
 # == Schema Information

app/models/user.rb

@@ -47,6 +47,8 @@ class User < ActiveRecord::Base
 
   belongs_to :uploaded_avatar, class_name: 'Upload', dependent: :destroy
 
+  delegate :last_sent_email_address, :to => :email_logs
+
   validates_presence_of :username
   validate :username_validator
   validates :email, presence: true, uniqueness: true
@@ -276,7 +278,6 @@ class User < ActiveRecord::Base
     end
   end
 
-
   def update_last_seen!(now=Time.zone.now)
     now_date = now.to_date
     # Only update last seen once every minute
@@ -493,6 +494,10 @@ class User < ActiveRecord::Base
     ApiKey.where(user_id: self.id).delete_all
   end
 
+  def find_email
+    last_sent_email_address || email
+  end
+
   protected
 
   def cook

spec/fabricators/email_log_fabricator.rb

@@ -1,4 +1,4 @@
-Fabricator(:email_log,) do
+Fabricator(:email_log) do
   user
   to_address { sequence(:address) { |i| "blah#{i}@example.com" } }
   email_type :invite

spec/models/email_log_spec.rb

@@ -21,4 +21,26 @@ describe EmailLog do
 
   end
 
+  describe ".last_sent_email_address" do
+    let(:user) { Fabricate(:user) }
+
+    context "when user's email exist in the logs" do
+      before do
+        user.email_logs.create(email_type: 'signup', to_address: user.email)
+        user.email_logs.create(email_type: 'blah', to_address: user.email)
+        user.reload
+      end
+
+      it "the user's last email from the log" do
+        expect(user.email_logs.last_sent_email_address).to eq(user.email)
+      end
+    end
+
+    context "when user's email does not exist email logs" do
+      it "returns nil" do
+        expect(user.email_logs.last_sent_email_address).to be_nil
+      end
+    end
+  end
+
 end

spec/models/user_spec.rb

@@ -885,4 +885,25 @@ describe User do
 
   end
 
+  describe "#find_email" do
+
+    let(:user) { Fabricate(:user, email: "bob@example.com") }
+
+    context "when email is exists in the email logs" do
+      before { user.stubs(:last_sent_email_address).returns("bob@lastemail.com") }
+
+      it "returns email from the logs" do
+        expect(user.find_email).to eq("bob@lastemail.com")
+      end
+    end
+
+    context "when email does not exist in the email logs" do
+      before { user.stubs(:last_sent_email_address).returns(nil) }
+
+      it "fetches the user's email" do
+        expect(user.find_email).to eq(user.email)
+      end
+    end
+  end
+
 end