feat(menu): 更新菜单权限并添加数据中心相关权限#388
Conversation
- 修改附件管理相关权限代码，增加 dataCenter前缀 - 添加数据中心菜单及其子菜单项 - 更新相关测试用例以匹配新的权限代码
dosubot
Bot
added
the
size:L
WalkthroughThe changes in this pull request involve updates to permission codes in the Changes
Possibly related PRs
Poem
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/Http/Admin/Controller/AttachmentController.php (3 hunks)
- databases/seeders/menu_update_20241029.php (1 hunks)
- tests/Feature/Admin/DataCenter/AttachmentControllerTest.php (2 hunks)
🔇 Additional comments (8)
tests/Feature/Admin/DataCenter/AttachmentControllerTest.php (3)
31-31: LGTM! Permission code update aligns with requirements.
The updated permission code dataCenter:attachment:list correctly reflects the new hierarchical permission structure, and the test comprehensively verifies all access scenarios.
71-71: LGTM! Permission code update maintains test integrity.
The updated permission code dataCenter:attachment:delete correctly reflects the new structure while maintaining comprehensive test coverage, including proper setup, authorization checks, and deletion verification.
Line range hint 1-91: Consider adding test coverage for upload operation.
The test file covers list and delete operations, but appears to be missing test coverage for the upload operation that should use the dataCenter:attachment:upload permission code. Consider adding a testUpload method to ensure complete test coverage of all attachment operations.
Would you like me to help generate a test method for the upload operation?
app/Http/Admin/Controller/AttachmentController.php (3)
51-51: LGTM! Permission code update aligns with data center categorization.
The permission code change properly reflects the method's categorization under the data center module, maintaining consistency with the Swagger documentation.
72-72: LGTM! Permission code update maintains consistency.
The permission code change follows the same pattern as other methods, maintaining a consistent permission hierarchy within the data center module.
89-89: LGTM! Permission code update is consistent. Verify related components.
The permission code change maintains consistency with the data center module structure.
Let's verify that all related components (tests, seeders, frontend) have been updated to use the new permission codes:
✅ Verification successful
Permission code changes are properly synchronized across all components
All related files have been updated to use the new dataCenter:attachment:* permission codes:
- Controller: All three permissions (list, upload, delete) use the new format
- Database seeder: Menu permissions are correctly defined with the new format
- Test file: Test cases use the updated permission codes
The changes are consistent and no remnants of the old permission format were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old permission codes that might need updating
rg -l 'attachment:(list|upload|delete)' --type php
# Search for the new permission codes to ensure they're properly referenced
rg -l 'dataCenter:attachment:(list|upload|delete)' --type php
Length of output: 424
Script:
#!/bin/bash
# Let's check the actual content of these files to verify the permission codes
echo "=== AttachmentController.php ==="
rg 'attachment:(list|upload|delete)|dataCenter:attachment:(list|upload|delete)' app/Http/Admin/Controller/AttachmentController.php
echo -e "\n=== menu_update_20241029.php ==="
rg 'attachment:(list|upload|delete)|dataCenter:attachment:(list|upload|delete)' databases/seeders/menu_update_20241029.php
echo -e "\n=== AttachmentControllerTest.php ==="
rg 'attachment:(list|upload|delete)|dataCenter:attachment:(list|upload|delete)' tests/Feature/Admin/DataCenter/AttachmentControllerTest.php
Length of output: 1124
databases/seeders/menu_update_20241029.php (2)
1-28: LGTM! Well-structured class declaration with proper documentation.
The file follows PHP best practices with strict typing, proper documentation, and clear constant definition.
82-104: Verify permission code alignment with controllers.
The permission codes (dataCenter:attachment:list, dataCenter:attachment:upload, dataCenter:attachment:delete) should match the permission checks in the AttachmentController. Let's verify this alignment.
✅ Verification successful
Permission codes perfectly align with controller annotations ✓
The permission codes in the menu seeder exactly match the Permission attribute annotations in AttachmentController.php:
dataCenter:attachment:listdataCenter:attachment:uploaddataCenter:attachment:delete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for permission checks in AttachmentController
rg -A 2 "Permission\(['\"]dataCenter:attachment:" app/
Length of output: 58
Script:
#!/bin/bash
# Let's try a broader search for the controller and permission checks
# First, find the attachment controller
fd -e php "AttachmentController"
# Also search for any permission checks related to attachments
rg -i "permission.*attach" app/
Length of output: 496
| public function data(): array | ||
| { | ||
| return [ | ||
| [ | ||
| 'name' => 'dataCenter', | ||
| 'path' => '/dataCenter', | ||
| 'meta' => new Meta([ | ||
| 'title' => '数据中心', | ||
| 'i18n' => 'baseMenu.dataCenter.index', | ||
| 'icon' => 'ri:database-line', | ||
| 'type' => 'M', | ||
| 'hidden' => 1, | ||
| 'componentPath' => 'modules/', | ||
| 'componentSuffix' => '.vue', | ||
| 'breadcrumbEnable' => 1, | ||
| 'copyright' => 1, | ||
| 'cache' => 1, | ||
| 'affix' => 0, | ||
| ]), | ||
| 'children' => [ | ||
| [ | ||
| 'name' => 'dataCenter:attachment', | ||
| 'path' => '/dataCenter/attachment', | ||
| 'component' => 'base/views/dataCenter/attachment/index', | ||
| 'meta' => new Meta([ | ||
| 'title' => '附件管理', | ||
| 'type' => 'M', | ||
| 'hidden' => 1, | ||
| 'icon' => 'ri:attachment-line', | ||
| 'i18n' => 'baseMenu.dataCenter.attachment', | ||
| 'componentPath' => 'modules/', | ||
| 'componentSuffix' => '.vue', | ||
| 'breadcrumbEnable' => 1, | ||
| 'copyright' => 1, | ||
| 'cache' => 1, | ||
| 'affix' => 0, | ||
| ]), | ||
| 'children' => [ | ||
| [ | ||
| 'name' => 'dataCenter:attachment:list', | ||
| 'meta' => new Meta([ | ||
| 'title' => '附件列表', | ||
| 'i18n' => 'baseMenu.dataCenter.attachmentList', | ||
| 'type' => 'B', | ||
| ]), | ||
| ], | ||
| [ | ||
| 'name' => 'dataCenter:attachment:upload', | ||
| 'meta' => new Meta([ | ||
| 'title' => '上传附件', | ||
| 'i18n' => 'baseMenu.dataCenter.attachmentUpload', | ||
| 'type' => 'B', | ||
| ]), | ||
| ], | ||
| [ | ||
| 'name' => 'dataCenter:attachment:delete', | ||
| 'meta' => new Meta([ | ||
| 'title' => '删除附件', | ||
| 'i18n' => 'baseMenu.dataCenter.attachmentDelete', | ||
| 'type' => 'B', | ||
| ]), | ||
| ], | ||
| ], | ||
| ], | ||
| ], | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using constants for menu types and common values.
The menu structure is well-organized, but could benefit from these improvements:
- Define constants for menu types ('M', 'B')
- Consider creating an enum for menu types
- Consider validating i18n keys format
Example implementation:
class MenuType
{
public const MENU = 'M';
public const BUTTON = 'B';
}
// Usage in data():
'type' => MenuType::MENU| public function create(array $data, int $parent_id = 0): void | ||
| { | ||
| foreach ($data as $v) { | ||
| $_v = $v; | ||
| if (isset($v['children'])) { | ||
| unset($_v['children']); | ||
| } | ||
| $_v['parent_id'] = $parent_id; | ||
| $menu = Menu::create(array_merge(self::BASE_DATA, $_v)); | ||
| if (isset($v['children']) && count($v['children'])) { | ||
| $this->create($v['children'], $menu->id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation and optimizing performance.
The recursive creation logic works but could be improved:
- Add validation for required fields before creation
- Consider adding a max depth limit to prevent stack overflow
- Consider using batch inserts for better performance
Here's a suggested implementation:
+ private const MAX_DEPTH = 5; // Prevent excessive nesting
- public function create(array $data, int $parent_id = 0): void
+ public function create(array $data, int $parent_id = 0, int $depth = 0): void
{
+ if ($depth >= self::MAX_DEPTH) {
+ throw new \RuntimeException('Maximum menu depth exceeded');
+ }
+
+ $batch = [];
foreach ($data as $v) {
$_v = $v;
+ // Validate required fields
+ if (empty($_v['name'])) {
+ throw new \InvalidArgumentException('Menu name is required');
+ }
+
if (isset($v['children'])) {
unset($_v['children']);
}
$_v['parent_id'] = $parent_id;
- $menu = Menu::create(array_merge(self::BASE_DATA, $_v));
+ $batch[] = array_merge(self::BASE_DATA, $_v);
}
+
+ // Batch insert menus
+ $menus = Menu::insert($batch);
+
+ // Process children
+ foreach ($data as $k => $v) {
if (isset($v['children']) && count($v['children'])) {
- $this->create($v['children'], $menu->id);
+ $this->create($v['children'], $menus[$k]->id, $depth + 1);
}
}
}Committable suggestion was skipped due to low confidence.
| public function run(): void | ||
| { | ||
| if (env('DB_DRIVER') === 'odbc-sql-server') { | ||
| Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] ON;'); | ||
| } | ||
| $this->create($this->data()); | ||
| if (env('DB_DRIVER') === 'odbc-sql-server') { | ||
| Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] OFF;'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding transaction and error handling.
While the SQL Server identity insert handling is good, consider these improvements for better reliability:
- Wrap the seeding operation in a transaction
- Add try-catch block for error handling
Here's a suggested implementation:
public function run(): void
{
+ try {
+ Db::beginTransaction();
if (env('DB_DRIVER') === 'odbc-sql-server') {
Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] ON;');
}
$this->create($this->data());
if (env('DB_DRIVER') === 'odbc-sql-server') {
Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] OFF;');
}
+ Db::commit();
+ } catch (\Throwable $e) {
+ Db::rollBack();
+ throw $e;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function run(): void | |
| { | |
| if (env('DB_DRIVER') === 'odbc-sql-server') { | |
| Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] ON;'); | |
| } | |
| $this->create($this->data()); | |
| if (env('DB_DRIVER') === 'odbc-sql-server') { | |
| Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] OFF;'); | |
| } | |
| } | |
| public function run(): void | |
| { | |
| try { | |
| Db::beginTransaction(); | |
| if (env('DB_DRIVER') === 'odbc-sql-server') { | |
| Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] ON;'); | |
| } | |
| $this->create($this->data()); | |
| if (env('DB_DRIVER') === 'odbc-sql-server') { | |
| Db::unprepared('SET IDENTITY_INSERT [' . Menu::getModel()->getTable() . '] OFF;'); | |
| } | |
| Db::commit(); | |
| } catch (\Throwable $e) { | |
| Db::rollBack(); | |
| throw $e; | |
| } | |
| } |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes