Adjust validation rules for guest users registration#62
Merged
Conversation
WalkthroughAdds dynamic validation branching in account registration to handle LOGIN_EMAIL vs LOGIN_USERNAME, and introduces User::findByUsername to lookup users by username; validation rules are adjusted based on whether a guest user with the same identifier already exists. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/Account.php`:
- Around line 311-313: The username rule added in the guest-conversion branch
(inside the UserModel::where(...) check) is too restrictive because it forces
'between:6,255' and will reject existing short usernames; update the assignment
to $rules['username'] so it does not enforce the 6–255 constraint (either remove
the between rule or lower it to match the system's actual username policy, e.g.
use 'required|username' or 'required|min:2|username') to allow conversion of
existing guests with shorter usernames.
- Around line 299-315: The switch over $this->loginAttribute() lacks a default
branch causing both 'email' and 'username' rules to remain if the config is
invalid; add a default case that treats the setting as LOGIN_EMAIL (or otherwise
a safe fallback) — e.g., unset($rules['username']) and optionally log a warning
— so registration validation remains permissive; update the switch in the
Account component that checks UserSettings::LOGIN_EMAIL and
UserSettings::LOGIN_USERNAME to include this default branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1f43ab3-31bf-406b-b2e8-63c73ed3da87
📒 Files selected for processing (1)
components/Account.php
mjauvin
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (2)
components/Account.php (2)
299-314:⚠️ Potential issue | 🟡 MinorAdd a safe fallback
defaultcase in the login-attribute switch.If
login_attributehas an unexpected value, both identifiers remain required, making registration stricter than intended. Add a default branch (email-safe fallback).Suggested patch
switch ($this->loginAttribute()) { case UserSettings::LOGIN_EMAIL: unset($rules['username']); $user = UserModel::findByEmail(post('email')); if ($user && $user->is_guest) { $rules['email'] = 'required|between:6,255|email'; } break; case UserSettings::LOGIN_USERNAME: unset($rules['email']); $user = UserModel::findByUsername(post('username')); if ($user && $user->is_guest) { $rules['username'] = 'required|between:6,255'; } break; + default: + unset($rules['username']); + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Account.php` around lines 299 - 314, The switch on $this->loginAttribute() (cases UserSettings::LOGIN_EMAIL and UserSettings::LOGIN_USERNAME) lacks a default, so unexpected login_attribute values leave both identifiers required; add a default branch that behaves like the email-safe fallback: unset($rules['username']), validate email presence/format like in the LOGIN_EMAIL branch (use UserModel::findByEmail(post('email')) and the same guest check to set $rules['email'] = 'required|between:6,255|email') so registration remains permissive/safe when the attribute is unknown.
311-311:⚠️ Potential issue | 🟠 MajorUsername guest override is still too restrictive (
between:6,255).This can reject valid existing guest usernames shorter than 6 chars during conversion. Align it with the model’s baseline username length policy.
Suggested patch
- $rules['username'] = 'required|between:6,255'; + $rules['username'] = 'required|between:2,255';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Account.php` at line 311, The username validation in components/Account.php currently hardcodes 'required|between:6,255' which is too restrictive for guest conversion; replace this hardcoded rule with the model's baseline username validation (use the User model's existing username validation constant/attribute—e.g., User::USERNAME_VALIDATION or User::$rules['username']—so $rules['username'] uses the same policy as the model) and remove the hardcoded between:6,255 to ensure shorter existing guest usernames are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/Account.php`:
- Around line 299-314: The switch on $this->loginAttribute() (cases
UserSettings::LOGIN_EMAIL and UserSettings::LOGIN_USERNAME) lacks a default, so
unexpected login_attribute values leave both identifiers required; add a default
branch that behaves like the email-safe fallback: unset($rules['username']),
validate email presence/format like in the LOGIN_EMAIL branch (use
UserModel::findByEmail(post('email')) and the same guest check to set
$rules['email'] = 'required|between:6,255|email') so registration remains
permissive/safe when the attribute is unknown.
- Line 311: The username validation in components/Account.php currently
hardcodes 'required|between:6,255' which is too restrictive for guest
conversion; replace this hardcoded rule with the model's baseline username
validation (use the User model's existing username validation
constant/attribute—e.g., User::USERNAME_VALIDATION or
User::$rules['username']—so $rules['username'] uses the same policy as the
model) and remove the hardcoded between:6,255 to ensure shorter existing guest
usernames are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5690977-1d23-4227-8de1-b3ee632144af
📒 Files selected for processing (2)
components/Account.phpmodels/User.php
Member
Author
|
@AIC-BV did you test this? |
Contributor
No not yet I'll try to find some time this week |
AIC-BV
reviewed
Apr 3, 2026
Contributor
|
@mjauvin tested and works perfectly, can now register a user that already existed as guest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #29
Summary by CodeRabbit