Add comprehensive email validation for blocked users

[everettbu]

Test 3

[Copilot]

Pull Request Overview

This PR implements a comprehensive email validation system for blocked users, adding a new BlockedEmail model and associated functionality to prevent user registration with blacklisted email addresses.

• Introduces a new BlockedEmail model with database migration and validation logic
• Refactors email validation from inline User model code to a reusable EmailValidator class
• Adds client-side validation to prevent re-submission of rejected emails

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
app/models/blocked_email.rb	New model for managing blocked email addresses with action types and tracking
lib/validators/email_validator.rb	New validator class extracting email validation logic from User model
app/models/user.rb	Refactored to use new EmailValidator and simplified validation declarations
db/migrate/20130724201552_create_blocked_emails.rb	Database migration creating blocked_emails table with indexes
app/controllers/users_controller.rb	Enhanced error response to include structured error data
app/assets/javascripts/discourse/controllers/create_account_controller.js	Client-side validation to track and prevent resubmission of rejected emails
spec/models/blocked_email_spec.rb	Test suite for BlockedEmail model functionality
spec/components/validators/email_validator_spec.rb	Test suite for EmailValidator class
spec/fabricators/blocked_email_fabricator.rb	Test factory for BlockedEmail objects
config/locales/server.en.yml	Added localization for blocked email error message

[Copilot]

Use the newer RSpec expectation syntax expect(subject).to be_falsey instead of the deprecated should syntax.

Suggested change

	subject.should be_false
	end
	shared_examples "when a BlockedEmail record matches" do
	it "updates statistics" do
	Timecop.freeze(Time.zone.now) do
	expect { subject }.to change { blocked_email.reload.match_count }.by(1)
	blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now)
	end
	end
	end
	context "action_type is :block" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) }
	it { should be_true }
	include_examples "when a BlockedEmail record matches"
	end
	context "action_type is :do_nothing" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:do_nothing]) }
	it { should be_false }
	expect(subject).to be_falsey
	end
	shared_examples "when a BlockedEmail record matches" do
	it "updates statistics" do
	Timecop.freeze(Time.zone.now) do
	expect { subject }.to change { blocked_email.reload.match_count }.by(1)
	expect(blocked_email.last_match_at).to be_within_one_second_of(Time.zone.now)
	end
	end
	end
	context "action_type is :block" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) }
	it { expect(subject).to be_truthy }
	include_examples "when a BlockedEmail record matches"
	end
	context "action_type is :do_nothing" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:do_nothing]) }
	it { expect(subject).to be_falsey }

[Copilot]

Use the newer RSpec expectation syntax it { is_expected.to be_truthy } instead of the deprecated should syntax.

Suggested change

	subject.should be_false
	end
	shared_examples "when a BlockedEmail record matches" do
	it "updates statistics" do
	Timecop.freeze(Time.zone.now) do
	expect { subject }.to change { blocked_email.reload.match_count }.by(1)
	blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now)
	end
	end
	end
	context "action_type is :block" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) }
	it { should be_true }
	include_examples "when a BlockedEmail record matches"
	end
	context "action_type is :do_nothing" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:do_nothing]) }
	it { should be_false }
	is_expected.to be_falsey
	end
	shared_examples "when a BlockedEmail record matches" do
	it "updates statistics" do
	Timecop.freeze(Time.zone.now) do
	expect { subject }.to change { blocked_email.reload.match_count }.by(1)
	expect(blocked_email.last_match_at).to be_within_one_second_of(Time.zone.now)
	end
	end
	end
	context "action_type is :block" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) }
	it { is_expected.to be_truthy }
	include_examples "when a BlockedEmail record matches"
	end
	context "action_type is :do_nothing" do
	let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:do_nothing]) }
	it { is_expected.to be_falsey }

[Copilot]

Use the newer RSpec expectation syntax it { is_expected.to be_falsey } instead of the deprecated should syntax.

Suggested change

	it { should be_false }
	it { is_expected.to be_falsey }

[Copilot]

Use the newer RSpec expectation syntax expect(record.errors[:email]).not_to be_present instead of the deprecated should syntax.

Suggested change

	record.errors[:email].should_not be_present
	end
	it "adds an error when email matches a blocked email" do
	BlockedEmail.stubs(:should_block?).with(record.email).returns(true)
	validate
	record.errors[:email].should be_present
	expect(record.errors[:email]).not_to be_present
	end
	it "adds an error when email matches a blocked email" do
	BlockedEmail.stubs(:should_block?).with(record.email).returns(true)
	validate
	expect(record.errors[:email]).to be_present

[Copilot]

Use the newer RSpec expectation syntax expect(record.errors[:email]).to be_present instead of the deprecated should syntax.

Suggested change

	record.errors[:email].should_not be_present
	end
	it "adds an error when email matches a blocked email" do
	BlockedEmail.stubs(:should_block?).with(record.email).returns(true)
	validate
	record.errors[:email].should be_present
	expect(record.errors[:email]).not_to be_present
	end
	it "adds an error when email matches a blocked email" do
	BlockedEmail.stubs(:should_block?).with(record.email).returns(true)
	validate
	expect(record.errors[:email]).to be_present

[Copilot]

The increment and save operations are not atomic, which could lead to race conditions in concurrent environments. Consider using increment!(:match_count) or update_counters for atomic counter updates.

Suggested change

	record.match_count += 1
	record.last_match_at = Time.zone.now
	record.save
	record.increment!(:match_count)
	record.update(last_match_at: Time.zone.now)

[Copilot]

Setting last_match_at separately from the counter increment creates a race condition. Consider using update_columns or combining both updates in a single atomic operation.

Suggested change

	record.match_count += 1
	record.last_match_at = Time.zone.now
	record.save
	record.update_columns(
	match_count: record.match_count + 1,
	last_match_at: Time.zone.now
	)

[github-actions]

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.