Add user server deletion capability #33
Conversation
Introduces the ability for users to delete their own servers if permitted by configuration. Adds related configuration, language strings, UI actions, and updates resource limit logic for improved robustness. Also updates plugin settings to allow toggling this feature.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds server deletion support: new config flag, six translation strings, a delete action on ServerResourcePage using ServerDeletionService with auth checks and notifications, resource-limit clamping in UserResourceLimits, plugin settings toggle persistence, and a trailing newline in plugin.json. Changes
Sequence DiagramsequenceDiagram
participant User
participant Page as ServerResourcePage
participant Auth as Authorization
participant Service as ServerDeletionService
participant DB as Database
User->>Page: Click "Delete Server"
Page->>Auth: Verify config & permissions
alt authorized
Page->>Service: Request server deletion
Service->>DB: Remove server and related data
DB-->>Service: Deletion result
alt success
Service-->>Page: Success
Page->>User: Show success notification and redirect
else failure
Service-->>Page: Error/exception
Page->>User: Show error notification
end
else not authorized
Auth-->>Page: Deny
Page->>User: Show authorization error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php (1)
13-64: Consider consolidating duplicate deletion logic.This
DeleteServerActionclass duplicates the deletion logic already implemented inServerResourcePage.php(lines 122-156). Both perform the same visibility checks, authorization, service invocation, and notifications. Consider whether this separate action class is necessary, or if the page's inline implementation is sufficient. If both are needed, consider extracting the common logic to avoid duplication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
user-creatable-servers/config/user-creatable-servers.php(1 hunks)user-creatable-servers/lang/en/strings.php(1 hunks)user-creatable-servers/plugin.json(1 hunks)user-creatable-servers/src/Filament/App/Widgets/UserResourceLimitsOverview.php(2 hunks)user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php(1 hunks)user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php(3 hunks)user-creatable-servers/src/Models/UserResourceLimits.php(9 hunks)user-creatable-servers/src/UserCreatableServersPlugin.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
user-creatable-servers/src/Filament/App/Widgets/UserResourceLimitsOverview.php (1)
user-creatable-servers/src/Models/UserResourceLimits.php (2)
user(37-40)UserResourceLimits(25-180)
🔇 Additional comments (17)
user-creatable-servers/lang/en/strings.php (1)
16-22: LGTM!The translation strings for the server deletion workflow are clear, appropriately cautionary, and follow the existing naming conventions in the file.
user-creatable-servers/config/user-creatable-servers.php (1)
9-9: LGTM!The configuration option follows the same pattern as
can_users_update_serversand provides appropriate control over the deletion feature.user-creatable-servers/src/UserCreatableServersPlugin.php (2)
60-65: LGTM!The toggle field for server deletion capability is properly configured and consistent with the existing
can_users_update_serverstoggle. The hint and tooltip provide clear guidance to administrators.
76-76: LGTM!The persistence logic correctly converts the boolean value to a string representation, consistent with the handling of
can_users_update_servers.user-creatable-servers/src/Filament/App/Widgets/UserResourceLimitsOverview.php (2)
15-25: Defensive null handling is appropriate.The added null checks for the authenticated user and resource limits prevent potential errors. The change from
auth()->user()->serversto$user->servers()->get()is more explicit and consistent with the pattern used in the UserResourceLimits model.
42-47: LGTM!The null check in
canView()is consistent with the changes ingetStats()and appropriately prevents the widget from being displayed when the user is not authenticated or has no resource limits.user-creatable-servers/plugin.json (1)
15-18: LGTM!The
metaobject addition appears to be standard plugin metadata for the Pelican panel system, with appropriate initial values.user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (4)
5-5: LGTM!The new imports appropriately support the server deletion functionality.
Also applies to: 9-9, 12-12
109-119: LGTM!The refactoring to build an actions array is clean and allows for conditional action inclusion.
122-156: Delete action implementation is solid.The delete action correctly:
- Uses translation keys for all user-facing text
- Checks configuration and ownership
- Uses try-catch for error handling
- Provides appropriate user feedback via notifications
- Redirects to the app root (as noted in comment on line 143)
One minor inconsistency:
DeleteServerAction.phpredirects toListServers::getUrl()while this implementation redirects to'/'. Consider standardizing the redirect behavior across both implementations.
159-159: LGTM!The method correctly returns the assembled actions array.
user-creatable-servers/src/Models/UserResourceLimits.php (6)
35-35: LGTM!Eager loading the
userrelationship is a good practice that prevents N+1 query issues, especially since the user is accessed in most methods of this model.
45-52: LGTM!The defensive null check, explicit query builder usage, and
max(0, ...)guard against negative values are all solid improvements that make the code more robust.
61-68: LGTM!The same defensive improvements applied to
getMemoryLeft()andgetDiskLeft()ensure consistency across all resource calculation methods.Also applies to: 77-84
92-95: LGTM!The early null check for the user in
canCreateServer()is appropriate and prevents unnecessary computation when the user relationship is missing.
97-97: LGTM!The consistent use of query builder methods (
$user->servers()->sum()and$user->servers()->count()) throughoutcanCreateServer()aligns with the patterns established in the resource calculation methods.Also applies to: 106-106, 117-117, 128-128
162-164: Important fix for configuration key consistency.These lines correct the config keys from
usercreatableservers.*touser-creatable-servers.*, matching the actual config file structure. Without this fix, the config lookups would fail and returnnull, potentially breaking server creation with missing resource limits.
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds the ability for users to delete their own servers when permitted by configuration. It introduces a configuration toggle, language strings for the UI, delete action implementations, and makes several robustness improvements to the resource limit calculation logic.
Key Changes:
- Adds
can_users_delete_serversconfiguration option with plugin settings UI - Implements server deletion functionality in ServerResourcePage with proper authorization checks
- Refactors resource limit calculations to use query builder methods and adds null-safety checks
- Fixes configuration key typo from
usercreatableserverstouser-creatable-servers
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| UserCreatableServersPlugin.php | Adds toggle for delete servers permission in plugin settings form and saves it to environment config |
| UserResourceLimits.php | Adds eager loading for user relationship, changes collection access to query builder, adds null checks, wraps calculations in max(0, ...) for safety, and fixes config key typo |
| ServerResourcePage.php | Implements delete action in header with authorization check, uses translation strings, handles success/error notifications, and redirects to home after deletion |
| DeleteServerAction.php | Creates a reusable delete action component (though currently unused) with hardcoded strings and some issues |
| UserResourceLimitsOverview.php | Adds null checks for authenticated user to prevent errors |
| plugin.json | Adds meta field with status information (appears unrelated to main feature) |
| config/user-creatable-servers.php | Adds configuration entry for can_users_delete_servers feature flag |
| lang/en/strings.php | Adds translation strings for delete functionality UI messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <?php | ||
|
|
||
| namespace Boy132\UserCreatableServers\Filament\Components\Actions; | ||
|
|
||
| use App\Filament\App\Resources\Servers\Pages\ListServers; | ||
| use App\Models\Server; | ||
| use App\Services\Servers\ServerDeletionService; | ||
| use Boy132\UserCreatableServers\Models\UserResourceLimits; | ||
| use Exception; | ||
| use Filament\Actions\DeleteAction; | ||
| use Filament\Notifications\Notification; | ||
|
|
||
| class DeleteServerAction extends DeleteAction | ||
| { | ||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
|
|
||
| $this->label('Delete Server'); | ||
|
|
||
| $this->modalHeading('Delete Server'); | ||
|
|
||
| $this->modalDescription('Are you sure you want to delete this server? This action cannot be undone.'); | ||
|
|
||
| $this->modalSubmitActionLabel('Delete'); | ||
|
|
||
| $this->visible(function () { | ||
| if (!config('user-creatable-servers.can_users_delete_servers')) { | ||
| return false; | ||
| } | ||
|
|
||
| return UserResourceLimits::where('user_id', auth()->user()->id)->exists(); | ||
| }); | ||
|
|
||
| $this->authorize(function (Server $record) { | ||
| return $record->owner_id === auth()->user()->id; | ||
| }); | ||
|
|
||
| $this->action(function (Server $record) { | ||
| try { | ||
| /** @var ServerDeletionService $service */ | ||
| $service = app(ServerDeletionService::class); | ||
|
|
||
| $service->handle($record); | ||
|
|
||
| Notification::make() | ||
| ->title('Server Deleted') | ||
| ->body('The server has been successfully deleted.') | ||
| ->success() | ||
| ->send(); | ||
|
|
||
| redirect(ListServers::getUrl()); | ||
| } catch (Exception $exception) { | ||
| report($exception); | ||
|
|
||
| Notification::make() | ||
| ->title('Could not delete server') | ||
| ->body('An error occurred while deleting the server. Please contact the panel admin.') | ||
| ->danger() | ||
| ->send(); | ||
| } | ||
| }); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This DeleteServerAction class is created but never registered or used anywhere in the codebase. The UserCreatableServersPluginProvider doesn't register it (unlike CreateServerAction on line 23), and the delete functionality is instead implemented directly in ServerResourcePage.php lines 122-156. This appears to be dead code that should either be removed or properly integrated. If it's intended for future use or optional registration, that should be documented.
| $this->label('Delete Server'); | ||
|
|
||
| $this->modalHeading('Delete Server'); | ||
|
|
||
| $this->modalDescription('Are you sure you want to delete this server? This action cannot be undone.'); | ||
|
|
||
| $this->modalSubmitActionLabel('Delete'); |
There was a problem hiding this comment.
These UI strings are hardcoded instead of using translation keys. The ServerResourcePage implementation correctly uses trans('user-creatable-servers::strings.delete_server') and related keys. For consistency with the rest of the codebase and to support internationalization, these should use the translation strings defined in lang/en/strings.php.
| Notification::make() | ||
| ->title('Server Deleted') | ||
| ->body('The server has been successfully deleted.') | ||
| ->success() | ||
| ->send(); | ||
|
|
||
| redirect(ListServers::getUrl()); | ||
| } catch (Exception $exception) { | ||
| report($exception); | ||
|
|
||
| Notification::make() | ||
| ->title('Could not delete server') | ||
| ->body('An error occurred while deleting the server. Please contact the panel admin.') | ||
| ->danger() | ||
| ->send(); |
There was a problem hiding this comment.
The notification messages are hardcoded instead of using translation keys. The ServerResourcePage implementation correctly uses trans('user-creatable-servers::strings.server_deleted'), trans('user-creatable-servers::strings.server_deleted_success'), etc. For consistency with the rest of the codebase and to support internationalization, these should use the translation strings defined in lang/en/strings.php.
| protected $with = ['user']; | ||
|
|
There was a problem hiding this comment.
Adding eager loading of the 'user' relationship may cause unnecessary queries in contexts where the user is not needed, such as in UserResourceLimitsResource table listings (line 20-61 of UserResourceLimitsResource.php) or when querying multiple UserResourceLimits at once. The null checks added in this PR suggest the user relationship might not always be available. Consider removing this eager loading and instead use explicit ->with(['user']) only where needed, or ensure the relationship is actually required in all contexts where UserResourceLimits is queried.
| protected $with = ['user']; |
user-creatable-servers/plugin.json
Outdated
| "composer_packages": null, | ||
| "meta": { | ||
| "status": "enabled", | ||
| "status_message": null | ||
| } |
There was a problem hiding this comment.
The meta field with status information appears to be unrelated to the stated purpose of this PR (adding user server deletion capability). This looks like debugging or temporary state that was accidentally included. Other plugin.json files in this repository (tickets, billing) do not include this meta field. Unless there's a specific reason this metadata is needed for the deletion feature, it should be removed.
| "composer_packages": null, | |
| "meta": { | |
| "status": "enabled", | |
| "status_message": null | |
| } | |
| "composer_packages": null |
There was a problem hiding this comment.
Rabbit is correct. Never include the meta.
| return UserResourceLimits::where('user_id', auth()->user()->id)->exists(); | ||
| }); | ||
|
|
||
| $this->authorize(function (Server $record) { | ||
| return $record->owner_id === auth()->user()->id; |
There was a problem hiding this comment.
The visible callback accesses auth()->user()->id without checking if the user is authenticated. While Filament typically ensures authentication, this should be defensive and check for null to prevent potential errors. The UserResourceLimitsOverview widget in this PR properly implements this check at lines 15-18 and 42-45.
| return UserResourceLimits::where('user_id', auth()->user()->id)->exists(); | |
| }); | |
| $this->authorize(function (Server $record) { | |
| return $record->owner_id === auth()->user()->id; | |
| $user = auth()->user(); | |
| if (! $user) { | |
| return false; | |
| } | |
| return UserResourceLimits::where('user_id', $user->id)->exists(); | |
| }); | |
| $this->authorize(function (Server $record) { | |
| $user = auth()->user(); | |
| if (! $user) { | |
| return false; | |
| } | |
| return $record->owner_id === $user->id; |
| return UserResourceLimits::where('user_id', auth()->user()->id)->exists(); | ||
| }); | ||
|
|
||
| $this->authorize(function (Server $record) { | ||
| return $record->owner_id === auth()->user()->id; |
There was a problem hiding this comment.
The authorize callback accesses auth()->user()->id without checking if the user is authenticated. While Filament typically ensures authentication, this should be defensive and check for null to prevent potential errors. The UserResourceLimitsOverview widget in this PR properly implements this check at lines 42-45.
| return UserResourceLimits::where('user_id', auth()->user()->id)->exists(); | |
| }); | |
| $this->authorize(function (Server $record) { | |
| return $record->owner_id === auth()->user()->id; | |
| $user = auth()->user(); | |
| if (! $user) { | |
| return false; | |
| } | |
| return UserResourceLimits::where('user_id', $user->id)->exists(); | |
| }); | |
| $this->authorize(function (Server $record) { | |
| $user = auth()->user(); | |
| if (! $user) { | |
| return false; | |
| } | |
| return $record->owner_id === $user->id; |
| ->success() | ||
| ->send(); | ||
|
|
||
| redirect(ListServers::getUrl()); |
There was a problem hiding this comment.
The redirect call is missing a return statement, so the notification will still be sent after redirection. Compare with ServerResourcePage.php line 144 which correctly returns the redirect. This should be: return redirect(ListServers::getUrl());
| redirect(ListServers::getUrl()); | |
| return redirect(ListServers::getUrl()); |
|
|
||
| namespace Boy132\UserCreatableServers\Filament\Server\Pages; | ||
|
|
||
| use App\Filament\App\Resources\Servers\Pages\ListServers; |
There was a problem hiding this comment.
ListServers is imported but never used in this file. The comment on line 143 mentions "Redirect to app panel home instead of ListServers" and the code redirects to '/' instead. This import should be removed.
| use App\Filament\App\Resources\Servers\Pages\ListServers; |
| ]; | ||
|
|
||
| // Add delete action if enabled and user owns the server | ||
| if (config('user-creatable-servers.can_users_delete_servers') && $server->owner_id === auth()->user()->id) { |
There was a problem hiding this comment.
This authorization check accesses auth()->user()->id without first verifying the user is authenticated. While Filament typically ensures authentication in this context, this should be defensive and check for null to prevent potential errors. For consistency with the null-safety improvements made elsewhere in this PR (see UserResourceLimitsOverview.php lines 15-18, 42-45), consider adding a null check here.
| if (config('user-creatable-servers.can_users_delete_servers') && $server->owner_id === auth()->user()->id) { | |
| if (config('user-creatable-servers.can_users_delete_servers') && auth()->check() && $server->owner_id === auth()->id()) { |
Updated the delete action logic to permit root admins to delete any server, not just those they own. This change ensures that both server owners and admins can perform deletions when enabled in the configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (2)
5-5: Remove unused import.The
ListServersimport is not used anywhere in this file. Line 147 redirects to'/'directly instead of using this class.🔎 Proposed fix
-use App\Filament\App\Resources\Servers\Pages\ListServers; use App\Filament\Server\Pages\ServerFormPage;
121-122: Add null check for authenticated user.Lines 121-122 call
auth()->user()->idandauth()->user()->isRootAdmin()without verifying the user is authenticated. While Filament pages are typically behind authentication, this is inconsistent with the defensive null checks added elsewhere in this PR.🔎 Proposed fix for null safety
- $canDelete = config('user-creatable-servers.can_users_delete_servers') && - ($server->owner_id === auth()->user()->id || auth()->user()->isRootAdmin()); + $user = auth()->user(); + $canDelete = config('user-creatable-servers.can_users_delete_servers') && + $user && ($server->owner_id === $user->id || $user->isRootAdmin());
🧹 Nitpick comments (1)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (1)
134-159: Consider more specific exception handling.The try-catch block catches generic
Exception, which is quite broad. While this ensures all errors are handled and reported, catching more specific exceptions (e.g.,ConnectionException, service-specific exceptions) would provide better error context and allow for more targeted error messages.That said, the current approach is acceptable for user-facing error handling, as it ensures users always receive feedback and exceptions are properly reported.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php(3 hunks)
🔇 Additional comments (1)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (1)
125-159: Well-structured delete action implementation.The delete action is properly implemented with:
- Configuration-driven feature flag
- Appropriate confirmation modal with translated strings
- Separation of concerns using
ServerDeletionService- Proper error handling and user notifications
- Reasonable admin bypass allowing root admins to delete any server
The integration follows Filament best practices and provides good user experience.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php (3)
19-25: Hardcoded strings should use translation keys.This issue was previously flagged and remains unaddressed. The labels and modal text should use the translation keys defined in
lang/en/strings.php(e.g.,trans('user-creatable-servers::strings.delete_server')) for localization consistency.
40-42: Missing null check in authorization callback.This issue was previously flagged and remains unaddressed. Line 41 calls
auth()->user()->idwithout verifying authentication, which can cause a fatal error. Apply the same null-check pattern used in thevisible()callback above.🔎 Proposed fix
$this->authorize(function (Server $record) { - return $record->owner_id === auth()->user()->id; + $user = auth()->user(); + if (!$user) { + return false; + } + return $record->owner_id === $user->id; });
44-67: Multiple previously flagged issues remain unaddressed.
- Line 57: The
redirect()call should be returned for explicit navigation handling.- Lines 51-55 & 61-65: Notification strings are hardcoded instead of using the translation keys
server_deleted,server_deleted_success,server_delete_error, andserver_delete_error_messagefromstrings.php.🔎 Proposed fixes
Notification::make() - ->title('Server Deleted') - ->body('The server has been successfully deleted.') + ->title(trans('user-creatable-servers::strings.server_deleted')) + ->body(trans('user-creatable-servers::strings.server_deleted_success')) ->success() ->send(); - redirect(ListServers::getUrl()); + return redirect(ListServers::getUrl()); } catch (Exception $exception) { report($exception); Notification::make() - ->title('Could not delete server') - ->body('An error occurred while deleting the server. Please contact the panel admin.') + ->title(trans('user-creatable-servers::strings.server_delete_error')) + ->body(trans('user-creatable-servers::strings.server_delete_error_message')) ->danger() ->send(); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php (1)
user-creatable-servers/src/Models/UserResourceLimits.php (2)
UserResourceLimits(25-180)user(37-40)
🔇 Additional comments (1)
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php (1)
27-38: Null check properly implemented.The visibility callback now correctly guards against null user before accessing
$user->id. This addresses the previously flagged issue.
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ttps://github.com/invalidfelix/plugins into feature/user-creatable-servers-delete-own-servers
user-creatable-servers/src/Filament/App/Widgets/UserResourceLimitsOverview.php
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Components/Actions/DeleteServerAction.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php
Outdated
Show resolved
Hide resolved
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php
Outdated
Show resolved
Hide resolved
| protected $with = ['user']; | ||
|
|
user-creatable-servers/plugin.json
Outdated
| "composer_packages": null, | ||
| "meta": { | ||
| "status": "enabled", | ||
| "status_message": null | ||
| } |
There was a problem hiding this comment.
Rabbit is correct. Never include the meta.
Deleted the DeleteServerAction class from the Filament components. This action is no longer needed or has been replaced elsewhere in the codebase.
Eliminated the unnecessary null check for the authenticated user in the canView method, as auth()->user() is assumed to always return a user in this context.
Replaces isRootAdmin() with can('delete servers') for determining if a user can delete a server, allowing for more flexible permission management.
Updated the post-creation redirect to use Filament's default panel URL instead of the root path. This ensures users are redirected to the appropriate panel home after creating a server.
Eliminates an unnecessary check for a null user in the cpuLeft method, assuming the user relationship is always present when this method is called.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (2)
5-5: Remove unused import.
ListServersis imported but never used in this file. The redirect now usesFilament::getDefaultPanel()->getUrl()instead.-use App\Filament\App\Resources\Servers\Pages\ListServers;
120-124: Add null check for authenticated user.The condition accesses
auth()->user()->idandauth()->user()->can()without verifying the user is authenticated. Useauth()->check()andauth()->id()for null-safe access.🔎 Proposed fix
// Add delete action if enabled and (user owns the server OR user is admin) - $canDelete = config('user-creatable-servers.can_users_delete_servers') && - ($server->owner_id === auth()->user()->id || auth()->user()->can('delete servers')); + $user = auth()->user(); + $canDelete = config('user-creatable-servers.can_users_delete_servers') && + $user && ($server->owner_id === $user->id || $user->can('delete servers'));user-creatable-servers/src/Models/UserResourceLimits.php (1)
35-36: Remove eager loading as previously discussed.Per the earlier review feedback (which Boy132 agreed with), this eager loading should be removed. It causes unnecessary queries in contexts where the user relationship isn't needed. Use explicit
->with(['user'])only where required.
🧹 Nitpick comments (3)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (1)
124-160: Consider using Filament'sauthorizemethod on the action.Per earlier feedback, Filament Actions support
->authorize()for built-in authorization checks. This is more idiomatic and ensures the permission is enforced server-side when the action executes, not just for visibility.🔎 Proposed refactor
- if ($canDelete) { - $actions[] = Action::make('delete') + $user = auth()->user(); + $actions[] = Action::make('delete') ->label(trans('user-creatable-servers::strings.delete_server')) ->color('danger') ->icon('tabler-trash') + ->authorize(fn () => config('user-creatable-servers.can_users_delete_servers') && + $user && ($server->owner_id === $user->id || $user->can('delete servers'))) ->requiresConfirmation() ->modalHeading(trans('user-creatable-servers::strings.delete_server')) ->modalDescription(trans('user-creatable-servers::strings.delete_server_warning')) ->modalSubmitActionLabel(trans('user-creatable-servers::strings.delete_server')) ->action(function () use ($server) { // ... action logic unchanged }); - }user-creatable-servers/src/Models/UserResourceLimits.php (2)
42-52: Inconsistent null-check handling across resource methods.
getCpuLeft()lacks the null check for$userthatgetMemoryLeft()andgetDiskLeft()have. Given Boy132's earlier comment that "the user should never be null," consider either:
- Removing the null checks from all methods (preferred if user is guaranteed), or
- Adding the null check here for consistency if defensive coding is desired.
86-131: Consider consolidating database queries for efficiency.The method makes up to 4 separate database queries:
count(),sum('cpu'),sum('memory'),sum('disk'). These could be consolidated into a single query:🔎 Potential optimization
$aggregates = $user->servers()->selectRaw('COUNT(*) as count, SUM(cpu) as cpu, SUM(memory) as memory, SUM(disk) as disk')->first();Then use
$aggregates->count,$aggregates->cpu, etc. This reduces database round-trips from 4 to 1. However, the current approach is clearer and may be acceptable for the expected load.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php(3 hunks)user-creatable-servers/src/Models/UserResourceLimits.php(9 hunks)
🔇 Additional comments (4)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (1)
133-159: LGTM on the delete action implementation.The action logic is well-structured:
- Service resolution via
app()follows Laravel conventions- Exception handling with
report()ensures errors are logged- Returning
nullon failure keeps the user on the page appropriately- The redirect to
Filament::getDefaultPanel()->getUrl()addresses earlier feedbackuser-creatable-servers/src/Models/UserResourceLimits.php (3)
54-68: Good use of query builder for aggregation.Using
$user->servers()->sum('memory')pushes the aggregation to the database, which is more efficient than loading all servers into memory. Themax(0, ...)clamping is also a good defensive measure.
70-84: LGTM!Same pattern as
getMemoryLeft()with query builder and defensive clamping.
158-160: Verify config keys match the configuration file.The config keys have been updated to use
user-creatable-servers.*namespace. Ensure these keys (database_limit,allocation_limit,backup_limit) are actually defined in the config file atuser-creatable-servers/config/user-creatable-servers.php.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (1)
119-119: Add null check for authenticated user (duplicate concern).The authorization check accesses
auth()->user()->idwithout verifying the user is authenticated. While Filament typically ensures authentication, this should be defensive to prevent potential null pointer exceptions.🔎 Recommended fix
- ->authorize(fn () => $server->owner_id === auth()->user()->id || auth()->user()->can('delete server', $server)) + ->authorize(fn () => $server->owner_id === auth()->id() || auth()->user()?->can('delete server', $server))Or alternatively, using a more explicit null check:
- ->authorize(fn () => $server->owner_id === auth()->user()->id || auth()->user()->can('delete server', $server)) + ->authorize(function () use ($server) { + $user = auth()->user(); + return $user && ($server->owner_id === $user->id || $user->can('delete server', $server)); + })
🧹 Nitpick comments (1)
user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (1)
113-113: Consider using__()helper for consistency with modern Laravel conventions.While
trans()works fine, the__()helper is the newer shorthand introduced in Laravel 5.4+. Using__()aligns better with current Laravel best practices.🔎 Suggested change
- ->label(trans('filament-panels::resources/pages/edit-record.form.actions.save.label')) + ->label(__('filament-panels::resources/pages/edit-record.form.actions.save.label'))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
user-creatable-servers/config/user-creatable-servers.phpuser-creatable-servers/lang/en/strings.phpuser-creatable-servers/plugin.jsonuser-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.phpuser-creatable-servers/src/Models/UserResourceLimits.php
🚧 Files skipped from review as they are similar to previous changes (3)
- user-creatable-servers/plugin.json
- user-creatable-servers/lang/en/strings.php
- user-creatable-servers/src/Models/UserResourceLimits.php
🔇 Additional comments (4)
user-creatable-servers/config/user-creatable-servers.php (1)
9-9: LGTM! Safe default for destructive operation.The new configuration option follows existing naming conventions and appropriately defaults to
falsefor this destructive capability.user-creatable-servers/src/Filament/Server/Pages/ServerResourcePage.php (3)
8-8: LGTM! Required imports for deletion functionality.Both imports are appropriately used in the delete action implementation.
Also applies to: 11-11
108-109: Good addition for authorization context.Retrieving the server instance here allows the delete action to perform proper ownership and permission checks.
127-147: Deletion flow implementation looks solid.The action properly:
- Delegates to
ServerDeletionServicefor the actual deletion logic- Reports exceptions for monitoring
- Provides clear user feedback via notifications
- Redirects using the proper Filament panel URL method (addressing previous feedback)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
user-creatable-servers/src/Models/UserResourceLimits.php (2)
45-45: Good defensive change for preventing negative resource counts.The
max(0, ...)clamping ensures that "left" calculations never return negative values, which is semantically correct and particularly important when servers are deleted (the purpose of this PR). This prevents edge cases where resource sums might temporarily exceed limits due to race conditions or timing windows.However, if clamping occurs frequently, it may indicate underlying data integrity issues that should be investigated.
💡 Optional: Add logging when clamping occurs to detect data integrity issues
For better observability, consider logging when the calculated value would be negative:
public function getCpuLeft(): ?int { if ($this->cpu > 0) { $sum_cpu = $this->user->servers->sum('cpu'); - return max(0, $this->cpu - $sum_cpu); + $remaining = $this->cpu - $sum_cpu; + if ($remaining < 0) { + \Log::warning('CPU usage exceeds limit', [ + 'user_id' => $this->user_id, + 'limit' => $this->cpu, + 'used' => $sum_cpu, + ]); + } + return max(0, $remaining); } return null; }Apply similar changes to
getMemoryLeft()andgetDiskLeft().Also applies to: 56-56, 67-67
73-113: Consider refactoring to reduce code duplication.The
canCreateServermethod duplicates resource calculation logic that already exists ingetCpuLeft(),getMemoryLeft(), andgetDiskLeft(). This duplication makes maintenance harder and creates inconsistency risk—for instance, the newmax(0, ...)clamping logic is in the helper methods but not used here.💡 Optional: Refactor to use helper methods
public function canCreateServer(int $cpu, int $memory, int $disk): bool { if ($this->server_limit && $this->user->servers->count() >= $this->server_limit) { return false; } if ($this->cpu > 0) { if ($cpu <= 0) { return false; } - $sum_cpu = $this->user->servers->sum('cpu'); - if ($sum_cpu + $cpu > $this->cpu) { + $cpu_left = $this->getCpuLeft(); + if ($cpu_left === null || $cpu > $cpu_left) { return false; } } if ($this->memory > 0) { if ($memory <= 0) { return false; } - $sum_memory = $this->user->servers->sum('memory'); - if ($sum_memory + $memory > $this->memory) { + $memory_left = $this->getMemoryLeft(); + if ($memory_left === null || $memory > $memory_left) { return false; } } if ($this->disk > 0) { if ($disk <= 0) { return false; } - $sum_disk = $this->user->servers->sum('disk'); - if ($sum_disk + $disk > $this->disk) { + $disk_left = $this->getDiskLeft(); + if ($disk_left === null || $disk > $disk_left) { return false; } } return true; }This reduces duplication and ensures the
max(0, ...)clamping behavior is consistent across all resource checks.
2 participants
Introduces the ability for users to delete their own servers if permitted by configuration. Adds related configuration, language strings, UI actions, and updates resource limit logic for improved robustness. Also updates plugin settings to allow toggling this feature. #23
Summary by CodeRabbit
New Features
Bug Fixes
Documentation / Minor
✏️ Tip: You can customize this high-level summary in your review settings.