Don't set the notification_email when only unconfirmed_email is changed22846-notifications-broken-during-email-address-change-before-email-confirmed

Signed-off-by: Rémy Coutable <remy@rymai.me>

2 files changed, 82 insertions, 33 deletions

diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 684fa030baf..6a2f4a39f09 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -393,24 +393,6 @@ describe User do
   end
 
   describe 'after commit hook' do
-    describe '.update_invalid_gpg_signatures' do
-      let(:user) do
-        create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
-          user.skip_reconfirmation!
-        end
-      end
-
-      it 'does nothing when the name is updated' do
-        expect(user).not_to receive(:update_invalid_gpg_signatures)
-        user.update_attributes!(name: 'Bette')
-      end
-
-      it 'synchronizes the gpg keys when the email is updated' do
-        expect(user).to receive(:update_invalid_gpg_signatures).at_most(:twice)
-        user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
-      end
-    end
-
     describe '#update_emails_with_primary_email' do
       before do
         @user = create(:user, email: 'primary@example.com').tap do |user|
@@ -450,6 +432,76 @@ describe User do
         expect(@user.emails.first.confirmed_at).not_to eq nil
       end
     end
+
+    describe '#update_notification_email' do
+      # Regression: https://gitlab.com/gitlab-org/gitlab-ce/issues/22846
+      context 'when changing :email' do
+        let(:user) { create(:user) }
+        let(:new_email) { 'new-email@example.com' }
+
+        it 'sets :unconfirmed_email' do
+          expect do
+            user.tap { |u| u.update!(email: new_email) }.reload
+          end.to change(user, :unconfirmed_email).to(new_email)
+        end
+
+        it 'does not change :notification_email' do
+          expect do
+            user.tap { |u| u.update!(email: new_email) }.reload
+          end.not_to change(user, :notification_email)
+        end
+
+        it 'updates :notification_email to the new email once confirmed' do
+          user.update!(email: new_email)
+
+          expect do
+            user.tap(&:confirm).reload
+          end.to change(user, :notification_email).to eq(new_email)
+        end
+
+        context 'and :notification_email is set to a secondary email' do
+          let!(:email_attrs) { attributes_for(:email, :confirmed, user: user) }
+          let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user) }
+
+          before do
+            user.emails.create(email_attrs)
+            user.tap { |u| u.update!(notification_email: email_attrs[:email]) }.reload
+          end
+
+          it 'does not change :notification_email to :email' do
+            expect do
+              user.tap { |u| u.update!(email: new_email) }.reload
+            end.not_to change(user, :notification_email)
+          end
+
+          it 'does not change :notification_email to :email once confirmed' do
+            user.update!(email: new_email)
+
+            expect do
+              user.tap(&:confirm).reload
+            end.not_to change(user, :notification_email)
+          end
+        end
+      end
+    end
+
+    describe '#update_invalid_gpg_signatures' do
+      let(:user) do
+        create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
+          user.skip_reconfirmation!
+        end
+      end
+
+      it 'does nothing when the name is updated' do
+        expect(user).not_to receive(:update_invalid_gpg_signatures)
+        user.update_attributes!(name: 'Bette')
+      end
+
+      it 'synchronizes the gpg keys when the email is updated' do
+        expect(user).to receive(:update_invalid_gpg_signatures).at_most(:twice)
+        user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
+      end
+    end
   end
 
   describe '#update_tracked_fields!', :clean_gitlab_redis_shared_state do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index e8196980a8c..05637eb0729 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -488,10 +488,6 @@ describe API::Users do
   describe "PUT /users/:id" do
     let!(:admin_user) { create(:admin) }
 
-    before do
-      admin
-    end
-
     it "updates user with new bio" do
       put api("/users/#{user.id}", admin), { bio: 'new test bio' }
 
@@ -525,27 +521,28 @@ describe API::Users do
       expect(json_response['avatar_url']).to include(user.avatar_path)
     end
 
-    it 'updates user with his own email' do
-      put api("/users/#{user.id}", admin), email: user.email
-
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['email']).to eq(user.email)
-      expect(user.reload.email).to eq(user.email)
-    end
-
     it 'updates user with a new email' do
+      old_email = user.email
+      old_notification_email = user.notification_email
       put api("/users/#{user.id}", admin), email: 'new@email.com'
 
+      user.reload
+
       expect(response).to have_gitlab_http_status(200)
-      expect(user.reload.notification_email).to eq('new@email.com')
+      expect(user).to be_confirmed
+      expect(user.email).to eq(old_email)
+      expect(user.notification_email).to eq(old_notification_email)
+      expect(user.unconfirmed_email).to eq('new@email.com')
     end
 
     it 'skips reconfirmation when requested' do
-      put api("/users/#{user.id}", admin), { skip_reconfirmation: true }
+      put api("/users/#{user.id}", admin), email: 'new@email.com', skip_reconfirmation: true
 
       user.reload
 
-      expect(user.confirmed_at).to be_present
+      expect(response).to have_gitlab_http_status(200)
+      expect(user).to be_confirmed
+      expect(user.email).to eq('new@email.com')
     end
 
     it 'updates user with his own username' do