Refactor(permission,test)#398
Conversation
dosubot
Bot
added
the
size:L
WalkthroughThe pull request includes modifications to permission codes in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
web/src/utils/recursionGetKey.ts (1)
1-21: Add JSDoc documentation for better code clarityWhile the file header provides project information, the function itself lacks documentation explaining its purpose, parameters, and return value.
Add function documentation:
+/** + * Recursively extracts values for a specified key from a nested array of objects + * @template T - Type of objects in the array + * @param {T[]} arr - Array of objects to process + * @param {keyof T} key - Key to extract from each object + * @returns {unknown[]} Array of extracted values + */ export function recursionGetKey<T extends Record<string, unknown>>(arr: T[], key: keyof T): unknown[] {databases/seeders/menu_update_20241031.php (1)
43-51: Add documentation for permission mapping structure.Consider adding PHPDoc comments explaining:
- The structure and format of permission codes
- The reasoning behind the permission name changes
- Any implications for existing code using these permissions
+ /** + * Returns the mapping of old permission codes to new ones. + * Format: 'old:permission:code' => 'new:permission:code' + * + * @return array<string, string> Array of permission mappings + */ public function data(): arrayapp/Http/Admin/Controller/Permission/RoleController.php (1)
Line range hint
159-165: Consider enhancing error handling with more specific messages.While the current error handling catches non-existent roles, consider these improvements:
- Add a specific error message to help identify the problematic role ID
- Validate permission IDs before applying them to prevent potential issues with invalid permissions
Here's a suggested improvement:
if (! $this->service->existsById($id)) { - throw new BusinessException(code: ResultCode::NOT_FOUND); + throw new BusinessException( + sprintf('Role with ID %d not found', $id), + ResultCode::NOT_FOUND + ); } $permissionIds = Arr::get($request->validated(), 'permissions', []); +if (! empty($permissionIds)) { + // Validate that all permission IDs exist before applying + $validPermissions = Menu::whereIn('id', $permissionIds)->pluck('id')->toArray(); + $invalidPermissions = array_diff($permissionIds, $validPermissions); + if (! empty($invalidPermissions)) { + throw new BusinessException( + sprintf('Invalid permission IDs: %s', implode(', ', $invalidPermissions)), + ResultCode::INVALID_PARAMS + ); + } +} $this->service->batchGrantPermissionsForRole($id, $permissionIds);web/src/modules/base/views/permission/user/index.vue (1)
133-133: Good addition ofawaitfor proper async handling!The addition of
awaitensures that the table refresh operation completes before proceeding, which is a good practice to prevent potential race conditions.Consider adding error handling for the refresh operation:
- await proTableRef.value.refresh() + try { + await proTableRef.value.refresh() + } catch (error) { + msg.error(t('crud.refreshError')) + }app/Http/Admin/Controller/Permission/UserController.php (2)
175-175: LGTM! Consider updating API documentation.The permission code change aligns with the new standardized format. Since this is a breaking change in permission codes, ensure that the API documentation and client applications are updated accordingly.
Consider adding a comment in the API documentation to highlight this permission code change for API consumers.
Line range hint
157-175: Consider providing a migration guide for the permission code changes.The permission code refactoring introduces a more structured format (
permission:entity:action), which improves maintainability. However, this is a breaking change that affects multiple components.Consider:
- Adding a migration guide in the documentation
- Implementing a temporary backward compatibility layer
- Providing a script to help users identify and update affected configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
app/Http/Admin/Controller/Permission/RoleController.php(2 hunks)app/Http/Admin/Controller/Permission/UserController.php(2 hunks)databases/seeders/menu_update_20241031.php(1 hunks)tests/Feature/Admin/Permission/RoleControllerTest.php(1 hunks)tests/Feature/Admin/Permission/UserControllerTest.php(1 hunks)web/src/modules/base/views/permission/role/data/getTableColumns.tsx(1 hunks)web/src/modules/base/views/permission/user/index.vue(1 hunks)web/src/store/modules/useRouteStore.ts(1 hunks)web/src/store/modules/useUserStore.ts(2 hunks)web/src/utils/recursionGetKey.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/src/store/modules/useRouteStore.ts
🔇 Additional comments (11)
web/src/utils/recursionGetKey.ts (1)
10-21: Verify the impact on route processing
Since this function is used for processing route names in useUserStore.ts, we should verify that it handles all possible route configurations correctly.
Let's check the route configurations:
✅ Verification successful
Let me gather more information about the menu structure and route configurations.
Based on the analysis of the codebase, I can now provide a final response:
The recursionGetKey function correctly handles route configurations
The function is used in useUserStore.ts to extract route names from the menu structure for permission checking. The analysis shows that:
- The menu structure follows a consistent pattern with optional
childrenarrays and requirednameproperties - The recursive traversal matches the hierarchical menu structure used throughout the application
- The function is only used for permission code extraction, not for actual route processing
- The route configurations are properly handled separately by the router system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check route configurations to ensure they match the expected structure
# Expected structure: Objects with optional 'children' array and the key being extracted
# Search for route definitions
rg -l 'routes?\s*=|routes?:\s*\[' --type ts --type vue
# Search for usage of recursionGetKey
rg 'recursionGetKey' -A 5 -B 5
Length of output: 2769
Script:
#!/bin/bash
# Check the menu structure and route configurations
# Look for menu type definitions and interfaces
ast-grep --pattern 'interface $_Menu {
$$$
}'
# Search for menu-related code and route configurations
rg -t ts "(menu.*=|route.*=|interface.*Menu)" -A 5 -B 2
# Look specifically for the getMenu implementation
ast-grep --pattern 'function getMenu() {
$$$
}'
Length of output: 58977
databases/seeders/menu_update_20241031.php (2)
1-16: LGTM! Well-structured file header with proper imports.
The file includes appropriate strict typing, comprehensive documentation headers, and necessary imports.
19-27: Verify the usage of BASE_DATA constant.
The BASE_DATA constant appears to be unused in the implementation. If it's intended for future use or documentation purposes, consider adding a comment explaining its purpose. Otherwise, consider removing it to avoid confusion.
web/src/modules/base/views/permission/role/data/getTableColumns.tsx (1)
57-57: Permission check update looks good but needs verification.
The change from a single permission to an array of permissions aligns with the backend changes. However, let's verify the permission consistency across the stack.
✅ Verification successful
Permission changes are consistent across the stack
The verification shows that:
- Backend controller methods are properly decorated with the new permissions
permission:role:getMenuandpermission:role:setMenu - A database seeder maps old permissions to these new ones
- Tests validate both permissions are working correctly
- Similar pattern is used in other parts of the codebase (e.g., user management)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify permission consistency across frontend and backend
# Expected: Permission strings should match in both frontend and backend
# Check frontend permission usage
echo "Frontend permission usage:"
rg -A 2 "permission:role:(getMenu|setMenu|permission)" --type ts --type tsx
# Check backend permission definitions
echo "Backend permission definitions:"
rg -A 2 "permission:role:(getMenu|setMenu|permission)" --type php
Length of output: 2566
Script:
#!/bin/bash
# Let's try again with correct file type for tsx/ts files and expand the search
echo "Frontend permission usage:"
rg -A 2 "permission:role:(getMenu|setMenu|permission)"
# Also check for any other related permission checks in the codebase
echo -e "\nRelated permission checks:"
ast-grep --pattern 'showBtn($$$)'
Length of output: 3961
app/Http/Admin/Controller/Permission/RoleController.php (2)
157-157: Permission code change maintains consistency with the new naming scheme.
The updated permission code permission:role:setMenu aligns with the standardized format and pairs logically with the corresponding getMenu permission.
Let's verify the permission code consistency and related changes:
#!/bin/bash
# Search for any remaining old permission code
rg "permission:set:role"
# Search for the new permission code format
rg "permission:role:setMenu"
# Look for related test updates to ensure test coverage is maintained
rg -t php "role:setMenu" "tests/"138-138: Permission code change aligns with the new naming convention.
The updated permission code permission:role:getMenu better reflects the method's purpose and follows the new standardized format permission:{entity}:{action}.
Let's verify the consistency of this permission code change across the codebase:
✅ Verification successful
Permission code change is consistent with the codebase-wide refactoring
The verification confirms that:
- The old permission code
permission:get:roleis being systematically replaced withpermission:role:getMenuas part of a broader permission naming standardization - The change is properly reflected in:
- Database seeders (menu_update_20241031.php contains the mapping)
- Tests (RoleControllerTest.php has updated assertions)
- Frontend code (getTableColumns.tsx uses the new permission)
- The new format follows the consistent pattern
permission:{entity}:{action}across the codebase, as evidenced by similar changes in UserController (permission:user:getRole,permission:user:setRole)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old permission code format
rg "permission:get:role"
# Search for the new permission code format to verify consistent usage
rg "permission:role:getMenu"
# Search for similar permission patterns to verify naming convention
rg "permission:[a-z]+:(get|set)[A-Z][a-zA-Z]+"
Length of output: 1425
Script:
#!/bin/bash
# Search for the new permission code format to verify consistent usage
rg "permission:role:getMenu"
# Search for similar permission patterns in PHP files
rg -t php "permission:[a-z]+:(get|set)[A-Z][a-zA-Z]+"
# Check menu seeders for permission mappings
rg -A 5 "permission:get:role|permission:role:getMenu" databases/seeders/
Length of output: 5057
app/Http/Admin/Controller/Permission/UserController.php (1)
157-157: LGTM! Verify test coverage for the updated permission code.
The permission code change follows the new standardized format permission:entity:action, which improves consistency across the codebase.
✅ Verification successful
Test coverage exists for the updated permission code
The test file tests/Feature/Admin/Permission/UserControllerTest.php contains comprehensive test coverage for the updated permission code permission:user:getRole. The test verifies:
- Permission check before access
- Permission grant functionality
- Actual role retrieval with the new permission code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for the updated permission code
# Test: Search for test cases covering the new permission code
rg -l 'permission:user:getRole' --type php '*Test.php'
Length of output: 104
Script:
#!/bin/bash
# Let's try a more comprehensive search for test files
fd "Test\.php$" | while read -r file; do
echo "=== $file ==="
rg -A 5 "permission:user:getRole|'user:get:roles'" "$file" || true
done
# Also search for the controller test specifically
fd "UserControllerTest\.php$" | while read -r file; do
echo "=== $file ==="
cat "$file"
done
Length of output: 20066
web/src/store/modules/useUserStore.ts (2)
16-16: LGTM: Clean import statement
The import statement follows proper TypeScript conventions and uses appropriate module aliasing.
135-137: 🛠️ Refactor suggestion
Verify menu structure and add type safety
The new permission collection logic has changed from route-based to menu-based, which could have security implications. Consider the following improvements:
- Validate menu structure before recursion
- Add type checking for the menu items
- Handle potential undefined 'name' properties
Let's verify the menu structure and usage:
Consider adding type safety and validation:
- const codes: string[] = recursionGetKey(getMenu(), 'name')
+ const menu = getMenu()
+ if (!menu || !Array.isArray(menu)) {
+ throw new Error('Invalid menu structure')
+ }
+ const codes: string[] = recursionGetKey(menu, 'name')
+ // Filter out undefined/null values
+ .filter(Boolean)tests/Feature/Admin/Permission/RoleControllerTest.php (1)
231-237: Verify permission code changes across the codebase.
The permission codes have been updated from permission:set:role and permission:get:role to permission:role:setMenu and permission:role:getMenu. Let's ensure these changes are consistent across the codebase.
✅ Verification successful
Permission code changes are consistent across the codebase
The permission code changes from permission:get:role and permission:set:role to permission:role:getMenu and permission:role:setMenu are properly migrated across the codebase:
- The migration is handled by
MenuUpdate20241031seeder which explicitly maps the old codes to new ones:
'permission:get:role' => 'permission:role:getMenu',
'permission:set:role' => 'permission:role:setMenu',- The new permission codes are correctly used in:
- RoleController.php annotations (
@Permission) - Frontend code in getTableColumns.tsx
- Test file (RoleControllerTest.php)
The changes are consistent and complete across all relevant parts of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for old and new permission codes to ensure complete migration
echo "Checking for old permission codes..."
rg -l "permission:(set|get):role"
echo -e "\nChecking for new permission codes..."
rg -l "permission:role:(setMenu|getMenu)"
echo -e "\nChecking RoleController for permission annotations..."
rg -A 5 "@Permission" "app/Http/Admin/Controller/Permission/RoleController.php"
Length of output: 759
Script:
#!/bin/bash
# Let's examine the content of the files that contain permission codes
echo "Content of menu_update_20241031.php:"
cat databases/seeders/menu_update_20241031.php
echo -e "\nContent of menu_seeder_20240926.php:"
cat databases/seeders/menu_seeder_20240926.php
echo -e "\nContent of RoleController.php:"
cat app/Http/Admin/Controller/Permission/RoleController.php
echo -e "\nContent of getTableColumns.tsx:"
cat web/src/modules/base/views/permission/role/data/getTableColumns.tsx
Length of output: 32420
tests/Feature/Admin/Permission/UserControllerTest.php (1)
267-273: Permission naming convention updates look good!
The permission string updates follow the new standardized format:
- Added 'permission:' prefix
- Using camelCase for actions
- Following entity-action pattern
Let's verify the permission string updates are consistent across the codebase:
✅ Verification successful
Permission updates are correctly implemented with proper migration path
The verification shows a well-managed permission update:
- The new format
permission:user:getRoleandpermission:user:setRoleis consistently implemented in:- UserControllerTest.php (test cases)
- UserController.php (controller annotations)
- Frontend components (table columns)
- Old format occurrences (
user:get:roles,user:set:roles) only appear in:- menu_seeder_20240926.php (historical seeder)
- menu_update_20241031.php (migration mapping old to new permissions)
The presence of the mapping in menu_update_20241031.php indicates a proper migration path for the permission changes, ensuring backward compatibility during the transition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style permission strings and verify new permission format adoption
# Check for old permission format
echo "Checking for old permission format..."
rg -l "user:(get|set):roles?"
# Verify new permission format usage
echo "Verifying new permission format..."
rg -l "permission:user:(get|set)Role"
# Check permission strings in route/middleware definitions
echo "Checking permission definitions in routes..."
rg -l "middleware.*permission:" -g "routes/*"
Length of output: 838
Script:
#!/bin/bash
# Let's check the specific content of files that still contain old permission format
echo "Content of files with old permission format:"
rg "user:(get|set):roles?" databases/seeders/menu_seeder_20240926.php databases/seeders/menu_update_20241031.php -B 2 -A 2
# Let's also check route definitions more broadly
echo -e "\nChecking route definitions:"
fd "routes" --type f --exec rg "permission:" {} -B 1 -A 1
# Let's verify the complete implementation in UserController
echo -e "\nChecking UserController implementation:"
rg "permission:user:(get|set)Role" app/Http/Admin/Controller/Permission/UserController.php -B 2 -A 2
Length of output: 2366
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores