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.

diff --git a/app/models/user.rb b/app/models/user.rb
index a82a7d28a0e6928783e99424c6d177ca43ac69bd..9459db7fe483eef098ba39c3bc7f737bedf7f8f8 100644 (file)
--- 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 2ec36c060b4ea6483bb440097f15bd0aa8934a6d..80a06c43ab43d84dbcce105e76a90623666a4575 100644 (file)
--- 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' }
-    end
+      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 'redirects to login' do
-      expect(response).to redirect_to(new_user_session_path)
+      it 'queues up bootstrapping of home timeline' do
+        expect(BootstrapTimelineWorker).to have_received(:perform_async).with(user.account_id)
+      end
     end
 
-    it 'queues up bootstrapping of home timeline' do
-      expect(BootstrapTimelineWorker).to have_received(:perform_async).with(user.account_id)
+    context 'when user is updating email' do
+      let!(:user) { Fabricate(:user, confirmation_token: 'foobar', unconfirmed_email: 'new-email@example.com') }
+
+      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 5ed7ed88b2f3ff11f0faec69a7654db32ced8ad2..8171c939a921bbc62e7dd70bc13c8a7e3d15abe6 100644 (file)
--- 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)