Allow admins to "lock" allocations #1811
Conversation
📝 WalkthroughWalkthroughThe changes introduce an allocation locking mechanism preventing users from deleting assigned allocations. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdminUI as Admin UI
participant Model as Allocation Model
participant DB as Database
rect rgb(230, 245, 255)
Note over AdminUI,Model: Allocation Assignment (Server Creation)
AdminUI->>Model: Store allocation with server_id
Model->>Model: Set is_locked = true
Model->>DB: Save allocation
DB-->>Model: Persisted
end
rect rgb(245, 255, 230)
Note over User,Model: Allocation Lock/Unlock
User->>AdminUI: Click lock/unlock action
AdminUI->>Model: Update is_locked state
Model->>DB: Persist change
DB-->>AdminUI: Success
end
rect rgb(255, 240, 230)
Note over User,Model: Allocation Dissociation
User->>AdminUI: Click dissociate/detach
AdminUI->>Model: Clear notes, set is_locked = false, set server_id = null
Model->>DB: Persist changes
DB-->>AdminUI: Success
Note over Model: Auto-assign first allocation to owner if needed
end
Pre-merge checks✅ 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 |
![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
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
138-144: Consider using preferred allocation update pattern for consistency.The code uses a mass update (
whereIn()->update()) to setis_locked = trueafter associating allocations. While this works correctly, the codebase has an established pattern for allocation updates: usingwhereNull('server_id'),lockForUpdate(), and direct property assignment withsave()(as seen inServerCreationService).Based on learnings
Consider refactoring for consistency:
- ->after(function (array $data) { - Allocation::whereIn('id', array_values(array_unique($data['recordId'])))->update(['is_locked' => true]); + ->after(function (array $data) { + Allocation::whereIn('id', array_values(array_unique($data['recordId']))) + ->lockForUpdate() + ->get() + ->each(function (Allocation $allocation) { + $allocation->is_locked = true; + $allocation->save(); + }); if (!$this->getOwnerRecord()->allocation_id) { $this->getOwnerRecord()->update(['allocation_id' => $data['recordId'][0]]); } }),Note: This is optional since at this point the allocations are already associated (no race condition risk) and no critical model events need to fire for
is_lockedchanges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php(2 hunks)app/Filament/Server/Resources/Allocations/AllocationResource.php(2 hunks)app/Models/Allocation.php(4 hunks)app/Services/Servers/ServerCreationService.php(1 hunks)database/migrations/2025_10_14_065517_add_is_locked_to_allocations.php(1 hunks)lang/en/admin/server.php(1 hunks)lang/en/server/network.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Services/Servers/ServerCreationService.phpapp/Models/Allocation.phpapp/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.phpapp/Filament/Server/Resources/Allocations/AllocationResource.php
🧬 Code graph analysis (5)
app/Services/Servers/ServerCreationService.php (1)
app/Models/Server.php (1)
allocation(277-280)
app/Models/Allocation.php (1)
app/Models/Server.php (1)
allocation(277-280)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (2)
app/Models/Allocation.php (1)
Allocation(48-141)app/Models/Server.php (1)
allocation(277-280)
app/Filament/Server/Resources/Allocations/AllocationResource.php (4)
app/Models/Allocation.php (3)
Allocation(48-141)node(137-140)server(129-132)app/Models/Server.php (1)
allocation(277-280)app/helpers.php (1)
user(127-130)app/Models/Permission.php (1)
Permission(11-221)
database/migrations/2025_10_14_065517_add_is_locked_to_allocations.php (1)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
table(32-159)
🔇 Additional comments (15)
lang/en/server/network.php (1)
15-16: LGTM - Clear translation keys.The translation keys appropriately describe the locking feature for end users.
lang/en/admin/server.php (1)
16-19: LGTM - Comprehensive admin translations.The translation keys provide clear labels and context for the admin locking interface.
app/Services/Servers/ServerCreationService.php (1)
187-196: LGTM - Follows preferred allocation assignment pattern.The implementation correctly sets
is_locked = truewhen assigning allocations to servers, and follows the established pattern of usingwhereNull('server_id'),lockForUpdate(), and direct property assignment withsave()to prevent race conditions and ensure model events fire properly.Based on learnings
database/migrations/2025_10_14_065517_add_is_locked_to_allocations.php (1)
14-16: LGTM - Clean migration.The migration properly adds the
is_lockedcolumn with a sensible default offalse, ensuring existing allocations remain unlocked.app/Filament/Server/Resources/Allocations/AllocationResource.php (3)
76-80: LGTM - Clear lock status indicator.The icon column appropriately displays the lock state with intuitive icons and helpful tooltip.
84-100: Verify admin bypass logic is intentional.The visibility condition
!$allocation->is_locked || user()?->can('update', $allocation->node)allows admins with node update permissions to delete locked allocations. This aligns with the translation "Locked allocations can only be deleted by admins", but confirm this is the intended security model.Additionally, the detach action uses a mass update approach (
Allocation::where()->update()) rather than the preferred pattern from the codebase of usinglockForUpdate()with direct property assignment. While this works correctly since model events aren't critical here andis_lockedis explicitly reset, consider whether consistency with the pattern used inServerCreationServiceis valuable.Based on learnings
103-103: Naming consistency improvement.Good update to use snake_case (
add_allocation) for consistency with Laravel/Filament conventions.app/Models/Allocation.php (4)
59-61: LGTM - Proper default value initialization.Setting the default
is_lockedtofalsein the model attributes ensures new allocations are unlocked by default.
76-76: LGTM - Validation rule added.The boolean validation rule for
is_lockedis properly defined.
81-85: Excellent defensive behavior - auto-unlock on dissociation.The
updatinghook automatically resetsis_lockedtofalsewhenserver_idbecomesnull, ensuring locked allocations are always tied to server associations. This prevents orphaned locked allocations and provides a safety net even if explicit unlock logic is missed elsewhere.
98-98: LGTM - Boolean cast added.Properly casts
is_lockedto boolean for consistent type handling.app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (4)
63-67: LGTM - Lock status display.The icon column appropriately displays the allocation lock state in the admin interface.
74-81: LGTM - Lock/unlock actions.The lock and unlock actions are properly implemented with appropriate visibility controls based on the current
is_lockedstate.
84-92: LGTM - Defensive allocation_id assignment.The dissociate after-hook properly clears the allocation state and includes good defensive logic to reassign the server's
allocation_idif it becomes orphaned.Note: The explicit setting of
is_locked = falseon line 86 is technically redundant since the model'sbooted()hook automatically resets it whenserver_idbecomes null. However, this explicit approach is harmless and makes the intent clear.
149-157: Mass update approach is necessary here.The bulk dissociate after-hook correctly uses a mass update to reset state for dissociated allocations. Since these allocations have already been dissociated (bypassing model events), the explicit reset of
is_lockedandnotesis necessary. The defensiveallocation_idreassignment is also a good safety measure.
Closes #1740
If an allocation is locked it can't be removed on the client area/ by normal users. When admins assign allocations to a server they are locked by default but can also be manually locked/ unlocked.