Add to option to login with an emailadres - #1196
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces direct username lookups with SofiaAccount.find_for_login/resolve_login_identifier, updates Devise locate_conditions to use the resolver, updates login/forgot-password labels to permit username or email, adds tests, and adjusts seeds for explicit emails and historical activity times. 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 login identifier (username or email)
User->>Browser: enter auth_key
Browser->>Controller: POST /login or /forgot_password (auth_key)
Controller->>Model: find_for_login(auth_key)
alt match found
Model-->>Controller: SofiaAccount (resolved username)
Controller->>Devise: locate/authorize using resolved username
Devise-->>Browser: success or password-reset email sent
else no match
Controller-->>Browser: not-found / error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. 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.68% 77.78% +0.09%
===========================================
Files 54 54
Lines 1340 1346 +6
===========================================
+ Hits 1041 1047 +6
Misses 299 299 ☔ 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
… 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
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
spec/controllers/callbacks_controller/sofia_account_spec.rb (1)
33-37: Inconsistent test assertion pattern.This test now uses
have_http_status(:ok)instead ofresponse.content_type, but all other similar tests in this file (lines 51, 69, 88, 107, 125, 143) still useexpect(response.content_type).to eq 'application/json; charset=utf-8'.If the content type check was removed intentionally for this specific case, consider adding a brief comment explaining why. Otherwise, align this assertion with the other tests for consistency.
app/models/sofia_account.rb (2)
36-41: Case-sensitivity asymmetry between username and email lookups.Username lookup is case-sensitive while email lookup is case-insensitive. This could confuse users who expect consistent behavior—logging in with
USER@EXAMPLE.COMworks, butTESTUSERfails when the username istestuser.If this is intentional, consider documenting it. Otherwise, consider making username lookup case-insensitive as well:
- find_by(username: trimmed) || User.where('LOWER(email) = LOWER(?)', trimmed).first&.sofia_account + where('LOWER(username) = LOWER(?)', trimmed).first || User.where('LOWER(email) = LOWER(?)', trimmed).first&.sofia_accountNote: If you make username lookup case-insensitive, ensure you have a functional index on
LOWER(username)for performance.
40-40: Minor: Preferfind_byoverwhere().firstfor single-record lookup.Using
find_byis more idiomatic Rails and slightly more readable.- find_by(username: trimmed) || User.where('LOWER(email) = LOWER(?)', trimmed).first&.sofia_account + find_by(username: trimmed) || User.find_by('LOWER(email) = LOWER(?)', trimmed)&.sofia_accountspec/models/sofia_account_spec.rb (1)
168-215: Tests for.resolve_login_identifierare nested inside the.find_for_logindescribe block.The contexts starting at line 168 ("when identifier resolves to an account") and line 207 ("when identifier does not resolve to an account") are testing
resolve_login_identifierbut are nested under thedescribe '.find_for_login'block. This makes the test output confusing and misrepresents what's being tested.Move these to a separate describe block:
♻️ Proposed structure
end +end + +describe '.resolve_login_identifier' do + let!(:account) { create(:sofia_account, username: 'testuser', password: 'password1234') } + let!(:account_with_email) { create(:sofia_account, password: 'password1234') } context 'when identifier resolves to an account' do # ... existing tests ... end context 'when identifier does not resolve to an account' do # ... existing tests ... end -end end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/models/sofia_account.rbconfig/initializers/devise.rbspec/controllers/callbacks_controller/sofia_account_spec.rbspec/models/sofia_account_spec.rb
🧰 Additional context used
🧬 Code graph analysis (2)
config/initializers/devise.rb (1)
app/models/sofia_account.rb (1)
resolve_login_identifier(43-45)
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). (2)
- GitHub Check: Test
- GitHub Check: Lint
🔇 Additional comments (3)
config/initializers/devise.rb (1)
21-24: LGTM! The fallback logic correctly handles nil resolution.The lambda properly falls back to the original
auth_keywhenresolve_login_identifierreturns nil (for non-existent accounts), allowing normal authentication failure flow.One consideration: if
resolve_login_identifierraises an unexpected exception (e.g., database connection issues), the entire login will fail with an error rather than gracefully degrading. This may be acceptable since such conditions indicate broader system issues.spec/models/sofia_account_spec.rb (2)
141-145: Good test case for precedence verification.This test correctly validates that username matching takes priority over email matching when both could match, which aligns with the
||precedence in the implementation.
100-119: Thorough username lookup coverage.The tests comprehensively cover exact match, case sensitivity, and whitespace trimming for username lookups.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/models/sofia_account_spec.rb (1)
141-145: Test doesn't fully verify username preference over email.This test updates
account_with_emailto have both username and email as'user@example.com', so it matches itself either way. To truly verify preference, you'd need two accounts where one's username matches the query and another's email matches.💡 Suggested improvement
it 'prefers username match over email match' do - account_with_email.update!(username: 'user@example.com') - result = described_class.find_for_login('user@example.com') - expect(result).to eq(account_with_email) + # account has email 'user@example.com' from the before block + # Create another account whose username matches the email + username_match_account = create(:sofia_account, username: 'user@example.com', password: 'password1234') + result = described_class.find_for_login('user@example.com') + expect(result).to eq(username_match_account) end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/sofia_account.rbspec/controllers/callbacks_controller/sofia_account_spec.rbspec/models/sofia_account_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/controllers/callbacks_controller/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). (2)
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (2)
app/models/sofia_account.rb (1)
36-45: LGTM!The implementation is well-designed:
- Input validation with
blank?handles nil, empty, and whitespace-only inputs- Parameterized queries prevent SQL injection
- Case-insensitive matching with
LOWER()is database-portable- Username lookup takes precedence over email lookup as intended
- Safe navigation (
&.) handles the case where a User exists but has no SofiaAccountspec/models/sofia_account_spec.rb (1)
100-218: Comprehensive test coverage for the new login resolution methods.The tests cover the key scenarios well:
- Username and email lookup with case-insensitivity and whitespace handling
- Nil/blank/whitespace-only input edge cases
- Non-existent account handling
- The
resolve_login_identifiertests appropriately verify delegation tofind_for_login
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@db/seeds.rb`:
- Around line 27-28: The email-generation line constructs email =
"#{name.downcase.tr(' ', '.').gsub(' de ', '.').gsub(' van der ', '.').gsub('
van ', '.')}@example.com" but the tr(' ', '.') call runs first, so the
subsequent gsub calls never match; either remove the dead gsub calls if you only
want to convert spaces to dots, or move the particle removals before the tr
(e.g., run gsub(/ van der | de | van /i, ' ') on name.downcase first, then tr('
', '.') ) so particles are stripped then spaces turned into dots; update the
email construction in db/seeds.rb (variable email) accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
db/seeds.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
🔇 Additional comments (4)
db/seeds.rb (4)
30-30: LGTM!Adding Benjamin Knopje with an explicit email and a birthday indicating a minor (16 years ago) is appropriate for testing age-related functionality.
48-60: LGTM!The refactored approach of creating activities with valid future times first, then storing the intended past times for later application, is a clean pattern for seeding historical data that would otherwise fail time-based validations.
94-98: LGTM!Using
update_columnsto bypass validations and callbacks is appropriate here for locking historical activities with past timestamps. Placing this at the end ensures all dependent data (orders, credit mutations) is created while the activities are still in a valid state.
114-127: LGTM!Adding explicit emails to the Sofia account users aligns with the PR objective of enabling email-based login and ensures these seeded accounts can be used to test the new login functionality.
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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.