Add to option to login with an emailadres #1196
Conversation
📝 WalkthroughWalkthroughReplaces direct username lookups with normalized login helpers in SofiaAccount, updates Devise locate_conditions to use the resolver, adjusts login/forgot-password labels to indicate "username or e‑mail", and adds unit tests for the new lookup behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Controller as SofiaAccountsController
participant Model as SofiaAccount
participant Devise
Note over User,Browser: User submits auth_key (username or email)
User->>Browser: submit auth_key
Browser->>Controller: POST /login or /forgot_password (auth_key)
Controller->>Model: find_for_login(auth_key)
alt match found
Model-->>Controller: SofiaAccount (with User)
Controller->>Devise: proceed (authenticate or trigger reset)
Devise-->>Browser: success / redirect
else no match
Controller-->>Browser: not-found / error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds the ability for users to log in using their email address in addition to their username. The changes update both the authentication logic and user-facing labels to support this new functionality.
- Implements email-based login lookup in the SofiaAccount model
- Updates Devise initializer to resolve login identifiers (email or username) to usernames
- Updates UI labels in login and password reset views to indicate users can enter either username or email
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/sofia_account.rb | Adds find_by_login and resolve_login_identifier methods to support finding accounts by username or email |
| config/initializers/devise.rb | Updates OmniAuth identity configuration to use the new identifier resolution method |
| app/controllers/sofia_accounts_controller.rb | Updates forgot password action to use the new find_by_login method |
| app/views/sofia_accounts/login.html.erb | Updates label from "Gebruikersnaam" to "Gebruikersnaam of e-mailadres" |
| app/views/sofia_accounts/forgot_password_view.html.erb | Updates label from "Gebruikersnaam" to "Gebruikersnaam of e-mailadres" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/sofia_account.rb (1)
36-41: Consider case-insensitive email matching for better UX.The email lookup at line 40 is case-sensitive, which could lead to authentication failures when users enter their email with different casing (e.g.,
User@Example.comvsuser@example.com).♻️ Proposed fix for case-insensitive email matching
def self.find_by_login(identifier) return nil if identifier.blank? trimmed = identifier.to_s.strip - find_by(username: trimmed) || joins(:user).find_by(users: { email: trimmed }) + find_by(username: trimmed) || joins(:user).find_by('LOWER(users.email) = LOWER(?)', trimmed) endNote: This assumes PostgreSQL or a database that supports the
LOWERfunction. For MySQL, the syntax is similar. Alternatively, if theusers.emailcolumn already has a case-insensitive collation, the current code is fine.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/sofia_accounts_controller.rbapp/models/sofia_account.rbapp/views/sofia_accounts/forgot_password_view.html.erbapp/views/sofia_accounts/login.html.erbconfig/initializers/devise.rb
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/sofia_accounts_controller.rb (2)
app/models/sofia_account.rb (1)
find_by_login(36-41)webpack.config.js (1)
require(5-5)
config/initializers/devise.rb (1)
app/models/sofia_account.rb (1)
resolve_login_identifier(43-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Build
🔇 Additional comments (5)
app/views/sofia_accounts/forgot_password_view.html.erb (1)
10-10: LGTM!The label update accurately reflects the new dual-identifier login capability.
app/models/sofia_account.rb (1)
43-45: LGTM!The method cleanly wraps
find_by_loginto provide the username for Devise authentication lookup.app/views/sofia_accounts/login.html.erb (1)
24-24: LGTM!The label update is consistent with the forgot password view and accurately describes the new functionality.
app/controllers/sofia_accounts_controller.rb (1)
129-129: LGTM!The updated method call correctly uses
find_by_loginto enable email-based password reset. The existing nil handling and generic error message appropriately protect against account enumeration.config/initializers/devise.rb (1)
21-24: The implementation already handles nil identifiers correctly. Whenresolve_login_identifierreturns nil (identifier not found), thelocate_conditionsattempts to match{ username: nil }, which fails to find an account. TheCallbacksController.identitymethod properly detects this case and returns a user-facing error message: "Inloggen mislukt. De ingevulde gegevens zijn incorrect." This is confirmed by the test spec which validates the behavior for non-existent accounts. No changes needed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1196 +/- ##
===========================================
+ Coverage 77.62% 77.72% +0.09%
===========================================
Files 54 54
Lines 1341 1347 +6
===========================================
+ Hits 1041 1047 +6
Misses 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/models/sofia_account.rb:
- Around line 45-47: Add unit tests for SofiaAccount.find_by_login and
SofiaAccount.resolve_login_identifier covering: successful lookup by username
and by email, case-insensitivity for both username and email, trimming
surrounding whitespace, handling nil/blank input (should return nil or
appropriate falsey), and behavior when no account matches (returns nil). Write
specs that exercise both methods directly (call find_by_login and
resolve_login_identifier) and assert expected return values, using unique
example usernames/emails and variations in case/whitespace to verify
normalization behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/sofia_account.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| def self.resolve_login_identifier(identifier) | ||
| find_by_login(identifier)&.username | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the test file for SofiaAccount
find . -type f -name "*sofia_account*" | grep -i specRepository: csvalpha/sofia
Length of output: 259
🏁 Script executed:
# Search for tests of find_by_login and resolve_login_identifier
rg -nP --type=ruby 'find_by_login|resolve_login_identifier' --glob='**/spec/**'Repository: csvalpha/sofia
Length of output: 40
🏁 Script executed:
# Also check the actual implementation to understand both methods better
cat -n app/models/sofia_account.rb | head -60Repository: csvalpha/sofia
Length of output: 2327
🏁 Script executed:
cat -n spec/models/sofia_account_spec.rbRepository: csvalpha/sofia
Length of output: 4196
Add tests for authentication methods find_by_login and resolve_login_identifier.
The SofiaAccount model lacks test coverage for these two methods. Given the repository's comprehensive test suite for SofiaAccount, these authentication methods require tests covering:
- Finding by username
- Finding by email
- Case sensitivity handling (username and email)
- Whitespace trimming
- Nil/blank input handling
- Behavior when no match is found
🤖 Prompt for AI Agents
In @app/models/sofia_account.rb around lines 45 - 47, Add unit tests for
SofiaAccount.find_by_login and SofiaAccount.resolve_login_identifier covering:
successful lookup by username and by email, case-insensitivity for both username
and email, trimming surrounding whitespace, handling nil/blank input (should
return nil or appropriate falsey), and behavior when no account matches (returns
nil). Write specs that exercise both methods directly (call find_by_login and
resolve_login_identifier) and assert expected return values, using unique
example usernames/emails and variations in case/whitespace to verify
normalization behavior.
… into feature/email-login
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/controllers/sofia_accounts_controller.rb:
- Line 129: The current SofiaAccount.find_for_login implementation loads all
records with non-nil emails and searches in Ruby; replace it with a
database-level, case-insensitive lookup in the SofiaAccount model to avoid
loading all users into memory. Update the find_for_login(login) method to
normalize the login (downcase for email comparisons) and perform a single query
that matches either a case-insensitive email (use LOWER(email) = ? or ILIKE for
Postgres) or an exact username match, returning the first matching record (e.g.,
use where(...).first or find_by with a sanitized SQL condition) so the filtering
is done by the DB instead of Ruby iteration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/sofia_accounts_controller.rbapp/models/sofia_account.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/sofia_account.rb
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/sofia_accounts_controller.rb (1)
app/models/sofia_account.rb (1)
find_for_login(36-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| def forgot_password # rubocop:disable Metrics/AbcSize, Metrics/MethodLength | ||
| generic_message = 'Als dit account bestaat, is er een email verstuurd.' | ||
| sofia_account = SofiaAccount.find_by(username: params.require(:username)) | ||
| sofia_account = SofiaAccount.find_for_login(params.require(:username)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the sofia_account.rb file
fd "sofia_account" -t fRepository: csvalpha/sofia
Length of output: 518
🏁 Script executed:
#!/bin/bash
# Locate and examine the find_for_login method
rg -n "find_for_login" -A 10 -B 2Repository: csvalpha/sofia
Length of output: 2044
🏁 Script executed:
#!/bin/bash
# Check the User model for relationships
fd "user\.rb" -t f | head -5Repository: csvalpha/sofia
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check the SofiaAccount model structure and relationships
fd "sofia_account\.rb" -t f -x cat {}Repository: csvalpha/sofia
Length of output: 2292
🏁 Script executed:
#!/bin/bash
# Check the User model for the relationship and email handling
head -100 app/models/user.rbRepository: csvalpha/sofia
Length of output: 3254
🏁 Script executed:
#!/bin/bash
# Check if there are any indexes on the email column in users table
rg -n "email" db/migrate/*.rb | grep -i "index\|add_column.*email"Repository: csvalpha/sofia
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check how find_for_login is used in the codebase
rg -n "find_for_login" -B 2 -A 2Repository: csvalpha/sofia
Length of output: 1061
🏁 Script executed:
#!/bin/bash
# Check for email indexes in all migrations
rg "add_index.*:users.*:email|add_index.*users.*email" -A 2 -B 2Repository: csvalpha/sofia
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the database schema to see current indexes
fd "schema.rb" -t f -x cat {} | grep -A 30 "t.string.*email"Repository: csvalpha/sofia
Length of output: 2416
🏁 Script executed:
#!/bin/bash
# Verify the exact relationship - check if join would work as suggested
rg "belongs_to.*user|has_one.*sofia" app/models/sofia_account.rb app/models/user.rbRepository: csvalpha/sofia
Length of output: 169
Critical performance issue in the find_for_login implementation.
The change correctly enables email-based lookup for password reset, but the find_for_login method in app/models/sofia_account.rb (lines 36-41) loads all users with non-nil emails into memory and iterates through them using Ruby's find method. This will cause severe performance degradation as your user base grows.
🐛 Recommended fix: use database-level case-insensitive query
In app/models/sofia_account.rb, replace the current implementation with a proper database query:
def self.find_for_login(identifier)
return nil if identifier.blank?
trimmed = identifier.to_s.strip
- find_by(username: trimmed) || User.includes(:sofia_account).where.not(email: nil).find { |u| u.email.casecmp?(trimmed) }&.sofia_account
+ find_by(username: trimmed) ||
+ joins(:user).where('LOWER(users.email) = LOWER(?)', trimmed).first
endThis performs the case-insensitive email match at the database level instead of loading all users into memory.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/controllers/sofia_accounts_controller.rb at line 129, The current
SofiaAccount.find_for_login implementation loads all records with non-nil emails
and searches in Ruby; replace it with a database-level, case-insensitive lookup
in the SofiaAccount model to avoid loading all users into memory. Update the
find_for_login(login) method to normalize the login (downcase for email
comparisons) and perform a single query that matches either a case-insensitive
email (use LOWER(email) = ? or ILIKE for Postgres) or an exact username match,
returning the first matching record (e.g., use where(...).first or find_by with
a sanitized SQL condition) so the filtering is done by the DB instead of Ruby
iteration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @spec/models/sofia_account_spec.rb:
- Around line 110-118: The tests assume case-insensitive username lookup but
find_for_login currently uses find_by(username: trimmed) which is DB-collation
dependent; update described_class.find_for_login to perform an explicit
case-insensitive match (e.g., use a SQL lower comparison like 'LOWER(username) =
?' with trimmed.downcase) instead of relying on find_by, while keeping the
existing email comparison using casecmp?; alternatively, if you prefer DB-side
fixes, ensure the username column has a case-insensitive collation, but the
immediate code change should be to normalize/compare username lowercased in
find_for_login so tests behave consistently across databases.
🧹 Nitpick comments (1)
spec/models/sofia_account_spec.rb (1)
120-133: Consider parameterizing repetitive whitespace tests.The whitespace handling tests (leading, trailing, surrounding) are thorough but repetitive. Consider using RSpec's parameterized tests or shared examples to reduce duplication while maintaining clarity.
Example using RSpec's parameterized approach
context 'when searching by username' do [ ['exact match', 'testuser'], ['uppercase', 'TESTUSER'], ['mixed case', 'TestUser'], ['leading whitespace', ' testuser'], ['trailing whitespace', 'testuser '], ['surrounding whitespace', ' testuser '] ].each do |description, input| it "finds account by username with #{description}" do result = described_class.find_for_login(input) expect(result).to eq(account) end end endHowever, the current explicit approach is perfectly acceptable and may be clearer for some teams.
Also applies to: 156-169, 269-277
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/models/sofia_account_spec.rb
🧰 Additional context used
🧬 Code graph analysis (1)
spec/models/sofia_account_spec.rb (1)
app/models/sofia_account.rb (2)
find_for_login(36-41)resolve_login_identifier(43-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (3)
spec/models/sofia_account_spec.rb (3)
136-176: Excellent test coverage for email-based login.The email lookup tests comprehensively cover case-insensitive matching, whitespace handling, and username precedence. The explicit use of
casecmp?in the implementation guarantees consistent case-insensitive behavior across environments.
178-212: Thorough edge case coverage.The nil/blank input handling and non-existent account scenarios are well-tested, including the important case where users with nil emails are correctly excluded from email lookups.
240-337: Well-structured tests for identifier resolution.The
resolve_login_identifiertests effectively verify that both username and email inputs correctly resolve to the account's username. The coverage of normalization (case, whitespace) and non-resolution scenarios is comprehensive.Note: The same username case-sensitivity concern flagged for
find_for_loginapplies here, as this method delegates to it.
| it 'finds account by username regardless of case' do | ||
| result = described_class.find_for_login('TESTUSER') | ||
| expect(result).to eq(account) | ||
| end | ||
|
|
||
| it 'finds account by username with mixed case' do | ||
| result = described_class.find_for_login('TestUser') | ||
| expect(result).to eq(account) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check database schema for sofia_accounts table and username column
fd schema.rb -x cat {} | rg -A 20 'create_table.*sofia_accounts'Repository: csvalpha/sofia
Length of output: 880
🏁 Script executed:
# Search for migrations related to sofia_accounts or username
fd -e rb . db/migrate/ | head -20Repository: csvalpha/sofia
Length of output: 1351
🏁 Script executed:
# Check for any collation or case-insensitive settings
rg -i 'collat|citext' db/Repository: csvalpha/sofia
Length of output: 40
🏁 Script executed:
# Locate the find_for_login implementation
rg -A 15 'def.*find_for_login' --type rubyRepository: csvalpha/sofia
Length of output: 722
Verify database collation supports case-insensitive username matching or add explicit case normalization.
These tests assume case-insensitive username lookups (lines 110-118, 232-236: 'TESTUSER' finding 'testuser'), but the implementation uses find_by(username: trimmed), which is case-sensitive by default unless the database specifies a case-insensitive collation. The schema shows no explicit collation settings on the username column, creating environment-dependent behavior where tests may pass in development (SQLite) but fail in production (PostgreSQL/MySQL without case-insensitive collation).
Email lookups explicitly use casecmp? for guaranteed case-insensitivity, but usernames rely on database configuration. For consistency and reliability, either:
- Configure the database column with case-insensitive collation, or
- Update the implementation to use explicit case normalization:
find_by('LOWER(username) = ?', trimmed.downcase) || User.includes(:sofia_account).where.not(email: nil).find { |u| u.email.casecmp?(trimmed) }&.sofia_account🤖 Prompt for AI Agents
In @spec/models/sofia_account_spec.rb around lines 110 - 118, The tests assume
case-insensitive username lookup but find_for_login currently uses
find_by(username: trimmed) which is DB-collation dependent; update
described_class.find_for_login to perform an explicit case-insensitive match
(e.g., use a SQL lower comparison like 'LOWER(username) = ?' with
trimmed.downcase) instead of relying on find_by, while keeping the existing
email comparison using casecmp?; alternatively, if you prefer DB-side fixes,
ensure the username column has a case-insensitive collation, but the immediate
code change should be to normalize/compare username lowercased in find_for_login
so tests behave consistently across databases.
This PR adds the option to login with a emailadress so when people forget there password and username the can still login in
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.