From 5ec25ff3e1c53a4feab1e9b9a3f1660cca538c23 Mon Sep 17 00:00:00 2001
From: Patrick Figel <patrick@figel.email>
Date: Fri, 5 Jan 2018 00:15:35 +0100
Subject: [PATCH] Fix email confirmation link not updating email (#6187)

A change introduced in #6125 prevents
`Devise::Models::Confirmable#confirm` from being called for existing
users, which in turn leads to `email` not being set to
`unconfirmed_email`, breaking email updates. This also adds a test
that would've caught this issue.
---
 app/models/user.rb                            |  8 ++--
 .../auth/confirmations_controller_spec.rb     | 40 ++++++++++++++-----
 spec/models/user_spec.rb                      |  8 ++++
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index a82a7d28a0e..9459db7fe48 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -126,18 +126,18 @@ class User < ApplicationRecord
   end
 
   def confirm
-    return if confirmed?
+    new_user = !confirmed?
 
     super
-    update_statistics!
+    update_statistics! if new_user
   end
 
   def confirm!
-    return if confirmed?
+    new_user = !confirmed?
 
     skip_confirmation!
     save!
-    update_statistics!
+    update_statistics! if new_user
   end
 
   def promote!
diff --git a/spec/controllers/auth/confirmations_controller_spec.rb b/spec/controllers/auth/confirmations_controller_spec.rb
index 2ec36c060b4..80a06c43ab4 100644
--- a/spec/controllers/auth/confirmations_controller_spec.rb
+++ b/spec/controllers/auth/confirmations_controller_spec.rb
@@ -12,20 +12,40 @@ describe Auth::ConfirmationsController, type: :controller do
   end
 
   describe 'GET #show' do
-    let!(:user) { Fabricate(:user, confirmation_token: 'foobar', confirmed_at: nil) }
+    context 'when user is unconfirmed' do
+      let!(:user) { Fabricate(:user, confirmation_token: 'foobar', confirmed_at: nil) }
 
-    before do
-      allow(BootstrapTimelineWorker).to receive(:perform_async)
-      @request.env['devise.mapping'] = Devise.mappings[:user]
-      get :show, params: { confirmation_token: 'foobar' }
+      before do
+        allow(BootstrapTimelineWorker).to receive(:perform_async)
+        @request.env['devise.mapping'] = Devise.mappings[:user]
+        get :show, params: { confirmation_token: 'foobar' }
+      end
+
+      it 'redirects to login' do
+        expect(response).to redirect_to(new_user_session_path)
+      end
+
+      it 'queues up bootstrapping of home timeline' do
+        expect(BootstrapTimelineWorker).to have_received(:perform_async).with(user.account_id)
+      end
     end
 
-    it 'redirects to login' do
-      expect(response).to redirect_to(new_user_session_path)
-    end
+    context 'when user is updating email' do
+      let!(:user) { Fabricate(:user, confirmation_token: 'foobar', unconfirmed_email: 'new-email@example.com') }
 
-    it 'queues up bootstrapping of home timeline' do
-      expect(BootstrapTimelineWorker).to have_received(:perform_async).with(user.account_id)
+      before do
+        allow(BootstrapTimelineWorker).to receive(:perform_async)
+        @request.env['devise.mapping'] = Devise.mappings[:user]
+        get :show, params: { confirmation_token: 'foobar' }
+      end
+
+      it 'redirects to login' do
+        expect(response).to redirect_to(new_user_session_path)
+      end
+
+      it 'does not queue up bootstrapping of home timeline' do
+        expect(BootstrapTimelineWorker).to_not have_received(:perform_async)
+      end
     end
   end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 5ed7ed88b2f..8171c939a92 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -148,6 +148,14 @@ RSpec.describe User, type: :model do
     end
   end
 
+  describe '#confirm' do
+    it 'sets email to unconfirmed_email' do
+      user = Fabricate.build(:user, confirmed_at: Time.now.utc, unconfirmed_email: 'new-email@example.com')
+      user.confirm
+      expect(user.email).to eq 'new-email@example.com'
+    end
+  end
+
   describe '#disable_two_factor!' do
     it 'saves false for otp_required_for_login' do
       user = Fabricate.build(:user, otp_required_for_login: true)