-
-
Notifications
You must be signed in to change notification settings - Fork 254
Make sure case for role permissions is correct #1892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis pull request fixes permission name casing inconsistencies. It updates role permission option keys to use consistent casing instead of lowercasing, adds a database migration to normalize existing permission names, and simplifies permission checking logic in the User model. Changes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
database/migrations/2025_11_11_121905_fix_case_for_role_permissions.php (1)
25-36: Optimize to only update when name changes.The migration updates all permissions even when the name hasn't changed. Consider checking if an update is needed before executing it.
Apply this diff to improve performance:
foreach (Permission::all() as $spatiePermission) { $name = $spatiePermission->name; foreach ($allPermissions as $permission) { if (Str::lower($name) === Str::lower($permission)) { $name = $permission; break; } } - $spatiePermission->update(['name' => $name]); + if ($name !== $spatiePermission->name) { + $spatiePermission->update(['name' => $name]); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Roles/RoleResource.php(1 hunks)app/Models/User.php(1 hunks)database/migrations/2025_11_11_121905_fix_case_for_role_permissions.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Models/User.php (2)
app/Models/Permission.php (1)
permissions(194-213)app/Models/Subuser.php (1)
permissions(77-80)
database/migrations/2025_11_11_121905_fix_case_for_role_permissions.php (1)
app/Models/Role.php (1)
Role(24-144)
🔇 Additional comments (2)
app/Models/User.php (1)
345-345: LGTM! Cleaner return statement.The direct return is more concise than using an intermediate variable.
app/Filament/Admin/Resources/Roles/RoleResource.php (1)
127-127: LGTM! Core fix for permission casing.Using the original model casing ensures permission keys match the canonical format. This aligns with the database migration that normalizes existing permission names.
Closes #1888