Skip to content

Conversation

@akshayutture-augment
Copy link

… 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.
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@akshayutture-augment
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Nov 14, 2025

Greptile Overview

Confidence Score: 2/5

  • This PR has a critical architectural issue with database writes during validation
  • Score reflects the critical logic issue in BlockedEmail.should_block? which modifies database state during validation. While the feature implementation is well-structured with good test coverage, the side-effect in validation violates Rails conventions and could cause transaction rollback issues or race conditions in concurrent scenarios.
  • Pay close attention to app/models/blocked_email.rb - the should_block? method needs refactoring to separate statistics tracking from the blocking check

Important Files Changed

File Analysis

Filename Score Overview
app/models/blocked_email.rb 2/5 New model for blocking emails with tracking stats; has critical database write during validation issue
lib/validators/email_validator.rb 4/5 Email validation logic extracted from User model; integrates BlockedEmail checks, missing newline
app/models/user.rb 5/5 Refactored to use new EmailValidator instead of inline validation methods
app/controllers/users_controller.rb 5/5 Enhanced error response to include error hash and values for client-side handling

Sequence Diagram

sequenceDiagram
    participant Client as Browser/Client
    participant Controller as UsersController
    participant User as User Model
    participant Validator as EmailValidator
    participant Blocked as BlockedEmail
    participant DB as Database

    Client->>Controller: POST /users/create (email, username, password)
    Controller->>User: User.new_from_params(params)
    Controller->>User: user.save
    User->>User: validate :email, presence: true, uniqueness: true
    User->>Validator: validates :email, email: true
    Validator->>Validator: Check whitelist/blacklist settings
    alt Email passes whitelist/blacklist
        Validator->>Blocked: BlockedEmail.should_block?(email)
        Blocked->>DB: WHERE email = ?
        alt Email found in blocked_emails
            Blocked->>DB: UPDATE match_count, last_match_at
            Blocked->>DB: save()
            Blocked-->>Validator: true/false (based on action_type)
        else Email not found
            Blocked-->>Validator: false
        end
        alt Email is blocked
            Validator->>User: errors.add(:email, "blocked")
        end
    else Email fails whitelist/blacklist
        Validator->>User: errors.add(:email, "not_allowed")
    end
    User-->>Controller: validation result
    alt Validation failed
        Controller->>Client: JSON {success: false, errors: {...}, values: {...}}
        Client->>Client: rejectedEmails.pushObject(email)
        Client->>Client: Re-validate email field
        Client->>Client: Show error on form
    else Validation passed
        Controller->>Client: JSON {success: true, message: ...}
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +11 to +19
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Modifying database state during validation is problematic. This method writes to the database (record.save) even during read-only validation checks, which can cause issues with transaction rollbacks and violates Rails conventions where validations should be side-effect free.

Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/blocked_email.rb
Line: 11:19

Comment:
**logic:** Modifying database state during validation is problematic. This method writes to the database (`record.save`) even during read-only validation checks, which can cause issues with transaction rollbacks and violates Rails conventions where validations should be side-effect free.

How can I resolve this? If you propose a fix, please make it concise.

value =~ regexp
end

end No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Missing newline at end of file

Suggested change
end
end
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/validators/email_validator.rb
Line: 24:24

Comment:
**syntax:** Missing newline at end of file

```suggestion
end
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants