Add comprehensive email validation for blocked users#9
Add comprehensive email validation for blocked users#9akshayutture-augment wants to merge 1 commit into
Conversation
… many times each email address is blocked, and last time it was blocked. Move email validation out of User model and into EmailValidator. Signup form remembers which email addresses have failed and shows validation error on email field.
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive email validation for blocked users by introducing a BlockedEmail model and integrating it into the user registration flow. The changes enable administrators to maintain a list of blocked email addresses and prevent users from registering with those addresses.
- Introduces
BlockedEmailmodel with action types (block/do_nothing) and tracking statistics - Refactors email validation from User model into a reusable
EmailValidatorclass - Enhances frontend to handle and display blocked email errors during account creation
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/blocked_email.rb | New model for storing and checking blocked email addresses with match tracking |
| lib/validators/email_validator.rb | New validator class that consolidates email validation logic including blocked email checks |
| app/models/user.rb | Refactors email validation to use the new EmailValidator class |
| app/controllers/users_controller.rb | Enhances error response to include detailed error information and user values |
| app/assets/javascripts/discourse/controllers/create_account_controller.js | Adds client-side tracking of rejected emails to prevent resubmission |
| db/migrate/20130724201552_create_blocked_emails.rb | Database migration for blocked_emails table |
| config/locales/server.en.yml | Adds localized error message for blocked emails |
| spec/models/blocked_email_spec.rb | Comprehensive test coverage for BlockedEmail model |
| spec/fabricators/blocked_email_fabricator.rb | Test factory for BlockedEmail |
| spec/components/validators/email_validator_spec.rb | Tests for EmailValidator integration with BlockedEmail |
|
|
||
| describe "new record" do | ||
| it "sets a default action_type" do | ||
| BlockedEmail.create(email: email).action_type.should == BlockedEmail.actions[:block] |
There was a problem hiding this comment.
Replace deprecated should syntax with modern RSpec expect syntax: expect(BlockedEmail.create(email: email).action_type).to eq(BlockedEmail.actions[:block])
| BlockedEmail.create(email: email).action_type.should == BlockedEmail.actions[:block] | |
| expect(BlockedEmail.create(email: email).action_type).to eq(BlockedEmail.actions[:block]) |
| it "last_match_at is null" do | ||
| # If we manually load the table with some emails, we can see whether those emails | ||
| # have ever been blocked by looking at last_match_at. | ||
| BlockedEmail.create(email: email).last_match_at.should be_nil |
There was a problem hiding this comment.
Replace deprecated should syntax with modern RSpec expect syntax: expect(BlockedEmail.create(email: email).last_match_at).to be_nil
| BlockedEmail.create(email: email).last_match_at.should be_nil | |
| expect(BlockedEmail.create(email: email).last_match_at).to be_nil |
| subject { BlockedEmail.should_block?(email) } | ||
|
|
||
| it "returns false if a record with the email doesn't exist" do | ||
| subject.should be_false |
There was a problem hiding this comment.
Replace deprecated should syntax with modern RSpec expect syntax: expect(subject).to be false
| subject.should be_false | |
| expect(subject).to be false |
| 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) |
There was a problem hiding this comment.
Replace deprecated should syntax with modern RSpec expect syntax: expect(blocked_email.last_match_at).to be_within_one_second_of(Time.zone.now)
| blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now) | |
| expect(blocked_email.last_match_at).to be_within_one_second_of(Time.zone.now) |
|
|
||
| context "action_type is :block" do | ||
| let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) } | ||
| it { should be_true } |
There was a problem hiding this comment.
Replace deprecated should syntax with modern RSpec expect syntax: it { is_expected.to be true }
| it { should be_true } | |
| it { is_expected.to be true } |
| it "doesn't add an error when email doesn't match a blocked email" do | ||
| BlockedEmail.stubs(:should_block?).with(record.email).returns(false) | ||
| validate | ||
| record.errors[:email].should_not be_present |
There was a problem hiding this comment.
Replace deprecated should_not syntax with modern RSpec expect syntax: expect(record.errors[:email]).not_to be_present
| record.errors[:email].should_not be_present | |
| expect(record.errors[:email]).not_to be_present |
| 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 |
There was a problem hiding this comment.
Replace deprecated should syntax with modern RSpec expect syntax: expect(record.errors[:email]).to be_present
| record.errors[:email].should be_present | |
| expect(record.errors[:email]).to be_present |
| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) | ||
| end | ||
| end | ||
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) |
There was a problem hiding this comment.
Use && instead of and for boolean logic to avoid precedence issues. Change to: if record.errors[attribute].blank? && BlockedEmail.should_block?(value)
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) | |
| if record.errors[attribute].blank? && BlockedEmail.should_block?(value) |
| record.match_count += 1 | ||
| record.last_match_at = Time.zone.now | ||
| record.save |
There was a problem hiding this comment.
The read-modify-write operation on match_count is not thread-safe. Consider using record.increment!(:match_count) or wrapping in a transaction with pessimistic locking to prevent race conditions when multiple requests check the same blocked email simultaneously.
| record.match_count += 1 | |
| record.last_match_at = Time.zone.now | |
| record.save | |
| record.increment!(:match_count) | |
| record.update_column(:last_match_at, Time.zone.now) |
| record.match_count += 1 | ||
| record.last_match_at = Time.zone.now | ||
| record.save |
There was a problem hiding this comment.
Use update_columns(match_count: record.match_count + 1, last_match_at: Time.zone.now) or increment!(:match_count) with touch(:last_match_at) to avoid loading callbacks and validations for this statistics update.
| 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) |
Test 3
Replicated from ai-code-review-evaluation/discourse-copilot#3