FIX: Do not add same email multiple times (#12322)

The user and an admin could create multiple email change requests for
the same user. If any of the requests was validated and it became
primary, the other request could not be deleted anymore.

app/controllers/users_controller.rb

@@ -283,17 +283,14 @@ class UsersController < ApplicationController
     user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
 
-    user_email = user.user_emails.find_by(email: params[:email])
-    if user_email&.primary
-      return render json: failed_json, status: 428
-    end
-
     ActiveRecord::Base.transaction do
-      if user_email
-        user_email.destroy
+      if email = user.user_emails.find_by(email: params[:email], primary: false)
+        email.destroy
         DiscourseEvent.trigger(:user_updated, user)
-      elsif
-        user.email_change_requests.where(new_email: params[:email]).destroy_all
+      elsif change_requests = user.email_change_requests.where(new_email: params[:email]).presence
+        change_requests.destroy_all
+      else
+        return render json: failed_json, status: 428
       end
 
       if current_user.staff? && current_user != user

lib/email_updater.rb

@@ -41,10 +41,10 @@ class EmailUpdater
       UserHistory.create!(action: UserHistory.actions[:add_email], acting_user_id: @user.id)
     end
 
-    change_req = EmailChangeRequest.find_or_initialize_by(
-      user_id: @user.id, new_email: email, requested_by: @guardian.user
-    )
+    change_req = EmailChangeRequest.find_or_initialize_by(user_id: @user.id, new_email: email)
+
     if change_req.new_record?
+      change_req.requested_by = @guardian.user
       change_req.old_email = old_email
       change_req.new_email = email
     end

spec/components/email_updater_spec.rb

@@ -17,6 +17,15 @@ describe EmailUpdater do
     expect(updater.errors.messages[:base].first).to be I18n.t("change_email.error_staged")
   end
 
+  it "does not create multiple email change requests" do
+    user = Fabricate(:user)
+
+    EmailUpdater.new(guardian: Fabricate(:admin).guardian, user: user).change_to(new_email)
+    EmailUpdater.new(guardian: Fabricate(:admin).guardian, user: user).change_to(new_email)
+
+    expect(user.email_change_requests.count).to eq(1)
+  end
+
   context "when an admin is changing the email of another user" do
     let(:admin) { Fabricate(:admin) }
     let(:updater) { EmailUpdater.new(guardian: admin.guardian, user: user) }

spec/requests/users_controller_spec.rb

@@ -2852,6 +2852,18 @@ describe UsersController do
       expect(event[:event_name]).to eq(:user_updated)
       expect(event[:params].first).to eq(user)
     end
+
+    it "can destroy duplicate emails" do
+      EmailChangeRequest.create!(
+        user: user,
+        new_email: user.email,
+        change_state: EmailChangeRequest.states[:authorizing_new]
+      )
+
+      delete "/u/#{user.username}/preferences/email.json", params: { email: user_email.email }
+
+      expect(user.email_change_requests).to be_empty
+    end
   end
 
   describe '#is_local_username' do