Adjust validation rules for guest users registration - #62
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 |
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
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
|
@AIC-BV did you test this? |
No not yet I'll try to find some time this week |
|
@mjauvin tested and works perfectly, can now register a user that already existed as guest |
v2.3.1 was tagged and released straight off the merges of #61 and #62 plus the composer.json constraint bump, but updates/version.yaml was never updated to match. The plugin therefore still reported itself as 2.3.0, and those three commits counted as shipped-but-unreleased work as far as Winter's update system was concerned. The v2.3.1 tag cannot be moved, so 2.3.1 is recorded here retroactively and this changelog fix itself ships as 2.3.2. No functional changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces #29
Summary by CodeRabbit