Add comprehensive email validation for blocked users#8
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 a comprehensive email blocking system to prevent account creation with specific email addresses. The implementation includes a new BlockedEmail model for tracking and managing blocked emails, with support for different action types (block or do nothing) and usage statistics.
Changes:
- Created
BlockedEmailmodel with database migration for tracking blocked emails and their match statistics - Refactored email validation from
Usermodel into a dedicatedEmailValidatorthat integrates with the blocked email system - Enhanced account creation error responses to include detailed validation information and track rejected emails on the client side
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/blocked_email.rb | New model implementing blocked email logic with action types and match tracking |
| db/migrate/20130724201552_create_blocked_emails.rb | Database migration creating the blocked_emails table with email, action_type, and tracking fields |
| lib/validators/email_validator.rb | New custom validator consolidating email domain whitelist/blacklist and blocked email checks |
| app/models/user.rb | Removed inline email validation logic, replaced with declarative validator usage |
| app/controllers/users_controller.rb | Enhanced error response to include structured errors and user values |
| app/assets/javascripts/discourse/controllers/create_account_controller.js | Added client-side tracking of rejected emails to provide immediate feedback |
| config/locales/server.en.yml | Added new error message for blocked emails |
| spec/models/blocked_email_spec.rb | Comprehensive test coverage for BlockedEmail model behavior |
| spec/fabricators/blocked_email_fabricator.rb | Test factory for creating BlockedEmail instances |
| spec/components/validators/email_validator_spec.rb | Test coverage for EmailValidator integration with blocked emails |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| 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.
The deprecated RSpec syntax .should is used. Consider updating to the modern expect syntax: 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.
The deprecated RSpec syntax .should is used. Consider updating to the modern 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.
The deprecated RSpec syntax .should is used. Consider updating to the modern expect syntax: 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.
The deprecated RSpec syntax .should is used. Consider updating to the modern expect syntax: expect(blocked_email.last_match_at).to be_within_one_second_of(Time.zone.now)
| 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 |
There was a problem hiding this comment.
The deprecated RSpec syntax .should_not is used. Consider updating to the modern expect syntax: expect(record.errors[:email]).not_to be_present
| 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 |
| 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 |
There was a problem hiding this comment.
The deprecated RSpec syntax .should is used. Consider updating to the modern expect syntax: expect(record.errors[:email]).to be_present
| 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 |
| 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.
Ruby style prefers && over and for boolean operators in conditional expressions. Using and can lead to unexpected precedence issues.
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) | |
| if record.errors[attribute].blank? && BlockedEmail.should_block?(value) |
| def self.should_block?(email) | ||
| record = BlockedEmail.where(email: email).first | ||
| if record | ||
| record.match_count += 1 | ||
| record.last_match_at = Time.zone.now | ||
| record.save | ||
| end | ||
| record && record.action_type == actions[:block] | ||
| end | ||
|
|
There was a problem hiding this comment.
The should_block? method performs a write operation (incrementing match_count and updating last_match_at) on every check. This creates a database write for every email validation, even during read-only operations like form validation. Consider separating the blocking check from the statistics update, or using a background job to update statistics asynchronously.
| def self.should_block?(email) | |
| record = BlockedEmail.where(email: email).first | |
| if record | |
| record.match_count += 1 | |
| record.last_match_at = Time.zone.now | |
| record.save | |
| end | |
| record && record.action_type == actions[:block] | |
| end | |
| def self.find_by_email(email) | |
| BlockedEmail.where(email: email).first | |
| end | |
| def self.should_block?(email) | |
| record = find_by_email(email) | |
| record && record.action_type == actions[:block] | |
| end | |
| def self.record_match!(email) | |
| record = find_by_email(email) | |
| return unless record | |
| record.match_count += 1 | |
| record.last_match_at = Time.zone.now | |
| record.save | |
| end |
Test 3
Summary by CodeRabbit
Release Notes
New Features
Database
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#3