Move updating a user's email to its own controller

app/controllers/users_controller.rb

@@ -7,7 +7,7 @@ class UsersController < ApplicationController
   skip_before_filter :authorize_mini_profiler, only: [:avatar]
   skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :account_created, :activate_account, :perform_account_activation, :authorize_email, :user_preferences_redirect, :avatar, :my_redirect, :toggle_anon, :admin_login]
 
-  before_filter :ensure_logged_in, only: [:username, :update, :change_email, :user_preferences_redirect, :upload_user_image, :pick_avatar, :destroy_user_image, :destroy, :check_emails]
+  before_filter :ensure_logged_in, only: [:username, :update, :user_preferences_redirect, :upload_user_image, :pick_avatar, :destroy_user_image, :destroy, :check_emails]
   before_filter :respond_to_suspicious_request, only: [:create]
 
   # we need to allow account creation with bad CSRF tokens, if people are caching, the CSRF token on the
@@ -471,35 +471,6 @@ class UsersController < ApplicationController
     end
   end
 
-  def change_email
-    params.require(:email)
-    user = fetch_user_from_params
-    guardian.ensure_can_edit_email!(user)
-    lower_email = Email.downcase(params[:email]).strip
-
-    RateLimiter.new(user, "change-email-hr-#{request.remote_ip}", 6, 1.hour).performed!
-    RateLimiter.new(user, "change-email-min-#{request.remote_ip}", 3, 1.minute).performed!
-
-    EmailValidator.new(attributes: :email).validate_each(user, :email, lower_email)
-    return render_json_error(user.errors.full_messages) if user.errors[:email].present?
-
-    # Raise an error if the email is already in use
-    return render_json_error(I18n.t('change_email.error')) if User.find_by_email(lower_email)
-
-    email_token = user.email_tokens.create(email: lower_email)
-    Jobs.enqueue(
-      :user_email,
-      to_address: lower_email,
-      type: :authorize_email,
-      user_id: user.id,
-      email_token: email_token.token
-    )
-
-    render nothing: true
-  rescue RateLimiter::LimitExceeded
-    render_json_error(I18n.t("rate_limiter.slow_down"))
-  end
-
   def authorize_email
     expires_now()
     if @user = EmailToken.confirm(params[:token])

app/controllers/users_email_controller.rb

@@ -0,0 +1,41 @@
+require_dependency 'rate_limiter'
+require_dependency 'email_validator'
+
+class UsersEmailController < ApplicationController
+
+  before_filter :ensure_logged_in
+
+  def index
+  end
+
+  def update
+    params.require(:email)
+    user = fetch_user_from_params
+    guardian.ensure_can_edit_email!(user)
+    lower_email = Email.downcase(params[:email]).strip
+
+    RateLimiter.new(user, "change-email-hr-#{request.remote_ip}", 6, 1.hour).performed!
+    RateLimiter.new(user, "change-email-min-#{request.remote_ip}", 3, 1.minute).performed!
+
+    EmailValidator.new(attributes: :email).validate_each(user, :email, lower_email)
+    return render_json_error(user.errors.full_messages) if user.errors[:email].present?
+
+    # Raise an error if the email is already in use
+    return render_json_error(I18n.t('change_email.error')) if User.find_by_email(lower_email)
+
+    email_token = user.email_tokens.create(email: lower_email)
+    Jobs.enqueue(
+      :user_email,
+      to_address: lower_email,
+      type: :authorize_email,
+      user_id: user.id,
+      email_token: email_token.token
+    )
+
+    render nothing: true
+  rescue RateLimiter::LimitExceeded
+    render_json_error(I18n.t("rate_limiter.slow_down"))
+  end
+
+end
+

config/routes.rb

@@ -292,8 +292,8 @@ Discourse::Application.routes.draw do
   put "users/:username" => "users#update", constraints: {username: USERNAME_ROUTE_FORMAT}
   put "users/:username/emails" => "users#check_emails", constraints: {username: USERNAME_ROUTE_FORMAT}
   get "users/:username/preferences" => "users#preferences", constraints: {username: USERNAME_ROUTE_FORMAT}, as: :email_preferences
-  get "users/:username/preferences/email" => "users#preferences", constraints: {username: USERNAME_ROUTE_FORMAT}
+  get "users/:username/preferences/email" => "users_email#index", constraints: {username: USERNAME_ROUTE_FORMAT}
-  put "users/:username/preferences/email" => "users#change_email", constraints: {username: USERNAME_ROUTE_FORMAT}
+  put "users/:username/preferences/email" => "users_email#update", constraints: {username: USERNAME_ROUTE_FORMAT}
   get "users/:username/preferences/about-me" => "users#preferences", constraints: {username: USERNAME_ROUTE_FORMAT}
   get "users/:username/preferences/badge_title" => "users#preferences", constraints: {username: USERNAME_ROUTE_FORMAT}
   put "users/:username/preferences/badge_title" => "users#badge_title", constraints: {username: USERNAME_ROUTE_FORMAT}

spec/controllers/users_controller_spec.rb

@@ -219,75 +219,6 @@ describe UsersController do
     end
   end
 
-  describe '.change_email' do
-    let(:new_email) { 'bubblegum@adventuretime.ooo' }
-
-    it "requires you to be logged in" do
-      expect { xhr :put, :change_email, username: 'asdf', email: new_email }.to raise_error(Discourse::NotLoggedIn)
-    end
-
-    context 'when logged in' do
-      let!(:user) { log_in }
-
-      it 'raises an error without an email parameter' do
-        expect { xhr :put, :change_email, username: user.username }.to raise_error(ActionController::ParameterMissing)
-      end
-
-      it "raises an error if you can't edit the user's email" do
-        Guardian.any_instance.expects(:can_edit_email?).with(user).returns(false)
-        xhr :put, :change_email, username: user.username, email: new_email
-        expect(response).to be_forbidden
-      end
-
-      context 'when the new email address is taken' do
-        let!(:other_user) { Fabricate(:coding_horror) }
-        it 'raises an error' do
-          xhr :put, :change_email, username: user.username, email: other_user.email
-          expect(response).to_not be_success
-        end
-
-        it 'raises an error if there is whitespace too' do
-          xhr :put, :change_email, username: user.username, email: other_user.email + ' '
-          expect(response).to_not be_success
-        end
-      end
-
-      context 'when new email is different case of existing email' do
-        let!(:other_user) { Fabricate(:user, email: 'case.insensitive@gmail.com')}
-
-        it 'raises an error' do
-          xhr :put, :change_email, username: user.username, email: other_user.email.upcase
-          expect(response).to_not be_success
-        end
-      end
-
-      it 'raises an error when new email domain is present in email_domains_blacklist site setting' do
-        SiteSetting.email_domains_blacklist = "mailinator.com"
-        xhr :put, :change_email, username: user.username, email: "not_good@mailinator.com"
-        expect(response).to_not be_success
-      end
-
-      it 'raises an error when new email domain is not present in email_domains_whitelist site setting' do
-        SiteSetting.email_domains_whitelist = "discourse.org"
-        xhr :put, :change_email, username: user.username, email: new_email
-        expect(response).to_not be_success
-      end
-
-      context 'success' do
-
-        it 'has an email token' do
-          expect { xhr :put, :change_email, username: user.username, email: new_email }.to change(EmailToken, :count)
-        end
-
-        it 'enqueues an email authorization' do
-          Jobs.expects(:enqueue).with(:user_email, has_entries(type: :authorize_email, user_id: user.id, to_address: new_email))
-          xhr :put, :change_email, username: user.username, email: new_email
-        end
-      end
-    end
-
-  end
-
   describe '.password_reset' do
     let(:user) { Fabricate(:user) }
 

spec/controllers/users_email_controller_spec.rb

@@ -0,0 +1,74 @@
+require 'rails_helper'
+
+describe UsersEmailController do
+
+  describe '.update' do
+    let(:new_email) { 'bubblegum@adventuretime.ooo' }
+
+    it "requires you to be logged in" do
+      expect { xhr :put, :update, username: 'asdf', email: new_email }.to raise_error(Discourse::NotLoggedIn)
+    end
+
+    context 'when logged in' do
+      let!(:user) { log_in }
+
+      it 'raises an error without an email parameter' do
+        expect { xhr :put, :update, username: user.username }.to raise_error(ActionController::ParameterMissing)
+      end
+
+      it "raises an error if you can't edit the user's email" do
+        Guardian.any_instance.expects(:can_edit_email?).with(user).returns(false)
+        xhr :put, :update, username: user.username, email: new_email
+        expect(response).to be_forbidden
+      end
+
+      context 'when the new email address is taken' do
+        let!(:other_user) { Fabricate(:coding_horror) }
+        it 'raises an error' do
+          xhr :put, :update, username: user.username, email: other_user.email
+          expect(response).to_not be_success
+        end
+
+        it 'raises an error if there is whitespace too' do
+          xhr :put, :update, username: user.username, email: other_user.email + ' '
+          expect(response).to_not be_success
+        end
+      end
+
+      context 'when new email is different case of existing email' do
+        let!(:other_user) { Fabricate(:user, email: 'case.insensitive@gmail.com')}
+
+        it 'raises an error' do
+          xhr :put, :update, username: user.username, email: other_user.email.upcase
+          expect(response).to_not be_success
+        end
+      end
+
+      it 'raises an error when new email domain is present in email_domains_blacklist site setting' do
+        SiteSetting.email_domains_blacklist = "mailinator.com"
+        xhr :put, :update, username: user.username, email: "not_good@mailinator.com"
+        expect(response).to_not be_success
+      end
+
+      it 'raises an error when new email domain is not present in email_domains_whitelist site setting' do
+        SiteSetting.email_domains_whitelist = "discourse.org"
+        xhr :put, :update, username: user.username, email: new_email
+        expect(response).to_not be_success
+      end
+
+      context 'success' do
+
+        it 'has an email token' do
+          expect { xhr :put, :update, username: user.username, email: new_email }.to change(EmailToken, :count)
+        end
+
+        it 'enqueues an email authorization' do
+          Jobs.expects(:enqueue).with(:user_email, has_entries(type: :authorize_email, user_id: user.id, to_address: new_email))
+          xhr :put, :update, username: user.username, email: new_email
+        end
+      end
+    end
+
+  end
+
+end