diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 945d50a5a51..006fb58b7f2 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/lib/email_updater.rb b/lib/email_updater.rb index 5b7b0aec950..68724a908d2 100644 --- a/lib/email_updater.rb +++ b/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 diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb index 31dbe7af9db..d10ec0b8bd7 100644 --- a/spec/components/email_updater_spec.rb +++ b/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) } diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a4a948aced8..ce974f3e1dd 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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