Dev #1712
Dev #1712
Conversation
Docstrings generation was requested by @dbarzin. * #1710 (comment) The following files were modified: * `app/Http/Controllers/API/NetworkSwitchController.php` * `app/Http/Controllers/Admin/ExplorerController.php` * `app/Http/Controllers/Admin/NetworkSwitchController.php` * `app/Models/NetworkSwitch.php` * `app/Models/Vlan.php`
📝 Add docstrings to `dev`
WalkthroughMass deletion functionality is added to both API and Admin NetworkSwitchController classes via new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/Admin/NetworkSwitchController.php (1)
123-134: Missing authorization check - critical security issue.The
massDestroymethod lacks an authorization check, unlike the corresponding API controller method (line 110 inapp/Http/Controllers/API/NetworkSwitchController.php). This allows any authenticated user to perform mass deletion of NetworkSwitch records without the requirednetwork_switch_deletepermission.Apply this diff to add the authorization check:
public function massDestroy(MassDestroyNetworkSwitchRequest $request) { + abort_if(Gate::denies('network_switch_delete'), Response::HTTP_FORBIDDEN, '403 Forbidden'); + NetworkSwitch::whereIn('id', request('ids'))->delete(); return response(null, Response::HTTP_NO_CONTENT); }Additionally, prefer using the injected
$requestparameter:- NetworkSwitch::whereIn('id', request('ids'))->delete(); + NetworkSwitch::whereIn('id', $request->input('ids'))->delete();
🧹 Nitpick comments (1)
app/Http/Controllers/API/NetworkSwitchController.php (1)
100-115: Prefer the injected request parameter.The implementation is correct and includes proper authorization. However, line 112 uses the global
request()helper instead of the injected$requestparameter, which is inconsistent with Laravel best practices.Apply this diff:
- NetworkSwitch::whereIn('id', request('ids'))->delete(); + NetworkSwitch::whereIn('id', $request->input('ids'))->delete();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
app/Http/Controllers/API/NetworkSwitchController.php(3 hunks)app/Http/Controllers/Admin/ExplorerController.php(1 hunks)app/Http/Controllers/Admin/NetworkSwitchController.php(2 hunks)app/Models/NetworkSwitch.php(1 hunks)app/Models/Vlan.php(1 hunks)package.json(1 hunks)version.txt(1 hunks)
🧰 Additional context used
🪛 PHPStan (2.1.31)
app/Models/Vlan.php
59-59: Method App\Models\Vlan::networkSwitches() should return Illuminate\Database\Eloquent\Relations\BelongsToMany<App\Models\NetworkSwitch, App\Models\Vlan, Illuminate\Database\Eloquent\Relations\Pivot, 'pivot'> but returns Illuminate\Database\Eloquent\Relations\BelongsToMany<App\Models\NetworkSwitch, $this(App\Models\Vlan), Illuminate\Database\Eloquent\Relations\Pivot, 'pivot'>.
Template type TDeclaringModel on class Illuminate\Database\Eloquent\Relations\BelongsToMany is not covariant. Learn more: https://phpstan.org/blog/whats-up-with-template-covariant
(return.type)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
package.json (1)
15-15: LGTM!Version bump is consistent with
version.txtand aligns with the PR's changes.version.txt (1)
1-1: LGTM!Version is consistent with
package.json.app/Http/Controllers/Admin/ExplorerController.php (1)
956-970: LGTM!The
formatIdhelper method reduces code duplication and is well-documented. The implementation correctly handles null IDs by returning null, which is appropriate for the graph construction logic.app/Models/NetworkSwitch.php (1)
42-46: LGTM!Documentation improvements enhance clarity by using the explicit class name and simplifying the return type annotation.
app/Models/Vlan.php (2)
55-55: LGTM!Using fully-qualified class names in the return annotation improves clarity and explicitness.
63-65: LGTM!The wording adjustment and fully-qualified return type annotation enhance documentation clarity.
Note: The PHPStan warning about template type covariance is a false positive related to Eloquent's generic type system and can be safely ignored.
app/Http/Controllers/API/NetworkSwitchController.php (2)
17-31: LGTM!The added docblock clearly describes the method's behavior, authorization requirements, and return value.
54-67: LGTM!The docblock comprehensively documents the method's authorization check and return type.
app/Http/Controllers/Admin/NetworkSwitchController.php (1)
18-32: LGTM!The docblock clearly describes the authorization requirement and return value for the index method.
Summary by CodeRabbit
New Features
Chores