FIX: Delete unconfirmed emails first if available (#13046)

FIX: Delete unconfirmed emails first if available (#13046)

Users can end up with the same email both as secondary and unconfirmed. When they tried to delete the unconfirmed ones, the secondary one was deleted.

diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js
index f0a0541..6d09211 100644
--- a/app/assets/javascripts/discourse/app/models/user.js
+++ b/app/assets/javascripts/discourse/app/models/user.js
@@ -416,8 +416,11 @@ const User = RestModel.extend({
       type: "DELETE",
       data: { email },
     }).then(() => {
-      this.secondary_emails.removeObject(email);
-      this.unconfirmed_emails.removeObject(email);
+      if (this.unconfirmed_emails.includes(email)) {
+        this.unconfirmed_emails.removeObject(email);
+      } else {
+        this.secondary_emails.removeObject(email);
+      }
     });
   },
 
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index bb796c3..5d7a37e 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -285,11 +285,10 @@ class UsersController < ApplicationController
     guardian.ensure_can_edit!(user)
 
     ActiveRecord::Base.transaction do
-      if email = user.user_emails.find_by(email: params[:email], primary: false)
-        email.destroy
-        DiscourseEvent.trigger(:user_updated, user)
-      elsif change_requests = user.email_change_requests.where(new_email: params[:email]).presence
+      if change_requests = user.email_change_requests.where(new_email: params[:email]).presence
         change_requests.destroy_all
+      elsif user.user_emails.where(email: params[:email], primary: false).destroy_all.present?
+        DiscourseEvent.trigger(:user_updated, user)
       else
         return render json: failed_json, status: 428
       end
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 2ef955e..4bbf1fd 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -2898,16 +2898,29 @@ describe UsersController do
       expect(event[:params].first).to eq(user)
     end
 
-    it "can destroy duplicate emails" do
+    it "can destroy unconfirmed emails" do
+      request_1 = EmailChangeRequest.create!(
+        user: user,
+        new_email: user_email.email,
+        change_state: EmailChangeRequest.states[:authorizing_new]
+      )
+
       EmailChangeRequest.create!(
         user: user,
-        new_email: user.email,
+        new_email: other_email.email,
         change_state: EmailChangeRequest.states[:authorizing_new]
       )
 
-      delete "/u/#{user.username}/preferences/email.json", params: { email: user_email.email }
+      EmailChangeRequest.create!(
+        user: user,
+        new_email: other_email.email,
+        change_state: EmailChangeRequest.states[:authorizing_new]
+      )
+
+      delete "/u/#{user.username}/preferences/email.json", params: { email: other_email.email }
 
-      expect(user.email_change_requests).to be_empty
+      expect(user.user_emails.pluck(:email)).to contain_exactly(user_email.email, other_email.email)
+      expect(user.email_change_requests).to contain_exactly(request_1)
     end
   end
 

GitHub sha: 034a0493

This commit appears in #13046 which was approved by eviltrout. It was merged by SamSaffron.