Fix/control calls #18
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces several targeted changes across the codebase. Stub file aliasing is corrected to match actual filenames, and unused import statements are removed from stub files. In the core logic, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Policy
participant Control
participant Model
User->>Policy: Calls policy method (e.g., view, create)
Policy->>Control: Calls applies()
Control->>Model: Checks for static control() method
alt control() exists
Model-->>Control: Returns custom control class
else control() does not exist
Control->>Control: Uses fallback logic to resolve control class
end
Control-->>Policy: Returns authorization result
Policy-->>User: Returns true/false
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
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. 🪧 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: 1
🧹 Nitpick comments (2)
tests/Feature/PoliciesTest.php (2)
91-103: Fix typo in test method nameThere's a typo in the test method name:
test_policies_calls_force_delete_methoforceDeleted_properlyshould likely betest_policies_calls_force_delete_method_properly.Otherwise, the test correctly verifies the forceDelete policy method.
- public function test_policies_calls_force_delete_methoforceDeleted_properly(): void + public function test_policies_calls_force_delete_method_properly(): void
1-104: Consider adding negative test casesThe current tests verify that policy methods return true when Gates are defined to return true. Consider adding negative test cases where Gates return false to ensure the full policy behavior is tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/AccessServiceProvider.php(1 hunks)src/Console/ControlMakeCommand.php(0 hunks)src/Console/PerimeterMakeCommand.php(0 hunks)src/Console/stubs/control.stub(0 hunks)src/Console/stubs/perimeter.overlay.stub(0 hunks)src/Console/stubs/perimeter.plain.stub(0 hunks)src/Controls/Control.php(1 hunks)src/Policies/ControlledPolicy.php(7 hunks)tests/Feature/PoliciesTest.php(1 hunks)tests/Support/Models/Model.php(2 hunks)tests/Support/Policies/ModelPolicy.php(1 hunks)tests/Unit/StubsTest.php(1 hunks)
💤 Files with no reviewable changes (5)
- src/Console/stubs/perimeter.plain.stub
- src/Console/PerimeterMakeCommand.php
- src/Console/ControlMakeCommand.php
- src/Console/stubs/perimeter.overlay.stub
- src/Console/stubs/control.stub
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/Support/Policies/ModelPolicy.php (1)
tests/Support/Models/Model.php (1)
Model(12-43)
src/Controls/Control.php (1)
src/Controls/HasControl.php (1)
control(22-27)
src/Policies/ControlledPolicy.php (1)
src/Controls/Control.php (1)
applies(49-68)
tests/Feature/PoliciesTest.php (4)
tests/Support/Models/Model.php (1)
Model(12-43)tests/Support/Models/User.php (1)
User(10-64)tests/Support/Policies/ModelPolicy.php (1)
ModelPolicy(8-11)src/Policies/ControlledPolicy.php (7)
view(59-62)viewAny(46-49)create(71-74)update(84-87)delete(97-100)restore(110-113)forceDelete(123-126)
🔇 Additional comments (18)
src/AccessServiceProvider.php (1)
89-89: Corrected stub file registration alias.The change fixes the stub file alias from 'controller.stub' to 'control.stub', ensuring that the alias matches the actual filename being published. This correction aligns the registration with the actual stub file path and provides consistency in the stub naming convention.
tests/Unit/StubsTest.php (1)
15-15: Updated test expectation to match corrected stub alias.This change aligns with the fix in AccessServiceProvider.php, ensuring that the test correctly verifies that the control stub is properly registered with its matching alias. The test expectation now correctly matches the implementation.
tests/Support/Policies/ModelPolicy.php (1)
6-6: Updated Model import to use custom test Model.The import change properly connects the policy to the custom test Model class instead of Laravel's base Model. This change aligns with the enhancement of the control-based authorization system where the Model now has an explicit reference to its control class.
tests/Support/Models/Model.php (2)
9-9: Added import for ModelControl class.Properly imports the ModelControl class needed for the static control property.
18-19: Added explicit control class reference.This addition establishes a direct link between the Model and its corresponding Control class through a static property. This supports the updated control resolution logic that checks for a static
controlproperty or method on the model class before falling back to conventional resolution.src/Policies/ControlledPolicy.php (7)
48-48: Method name changed from 'should' to 'applies'The policy method now uses
appliesinstead ofshouldmethod on the Control instance. This change is consistent with the new method name in the Control class that evaluates whether a control applies to a user and model.
61-61: Method name changed from 'should' to 'applies'Correctly updated to use the
appliesmethod for theviewpolicy.
73-73: Method name changed from 'should' to 'applies'Correctly updated to use the
appliesmethod for thecreatepolicy.
86-86: Method name changed from 'should' to 'applies'Correctly updated to use the
appliesmethod for theupdatepolicy.
99-99: Method name changed from 'should' to 'applies'Correctly updated to use the
appliesmethod for thedeletepolicy.
112-112: Method name changed from 'should' to 'applies'Correctly updated to use the
appliesmethod for therestorepolicy.
125-125: Method name changed from 'should' to 'applies'Correctly updated to use the
appliesmethod for theforceDeletepolicy.tests/Feature/PoliciesTest.php (6)
11-23: Test verifies 'view' policy method properlyGreat test for verifying the view policy method. The test creates a model, gets the current user, instantiates the policy, and verifies the expected result.
25-35: Test verifies 'viewAny' policy method properlyGood test for the viewAny policy method. This is an important test as it verifies proper handling of policies that don't require a specific model instance.
37-47: Test verifies 'create' policy method properlyThe test correctly verifies that the create policy method functions as expected.
49-61: Test verifies 'update' policy method properlyGood test for the update policy method. The test appropriately creates a model instance for testing.
63-75: Test verifies 'delete' policy method properlyThis test properly verifies the delete policy function.
77-89: Test verifies 'restore' policy method properlyThis test properly verifies the restore policy function.
| // @TODO: The auto guess here is strange, we specify the models / controls everywhere, is there a better way of doing this ? (In policies guess the model as Laravel is doing ?) | ||
| // @TODO: Discussed with Lucas G | ||
|
|
||
| if (method_exists($modelName, 'control')) { | ||
| return $modelName::control()::class; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhanced control resolution by checking for static control method
Good enhancement! The resolveControlName method now checks if the model class has a static control method and uses it if available, before falling back to the conventional naming resolution.
However, the TODO comments suggest there are concerns about the auto-guessing approach. Consider addressing these TODOs before finalizing this PR or creating follow-up tickets to track these concerns.
🏁 Script executed:
#!/bin/bash
# Check if any models in the codebase implement the static control method
echo "Looking for models that implement static control method..."
rg -l "public static function control\(" --type phpLength of output: 208
🏁 Script executed:
#!/bin/bash
# Inspect the HasControl trait definition
echo "Inspecting HasControl trait:"
rg -Hn "trait HasControl" --type php
# List all references to HasControl to see where it's used
echo "Listing all references to HasControl:"
rg -Hn "HasControl" --type phpLength of output: 677
🏁 Script executed:
#!/bin/bash
# Check for any control() method definitions in HasControl trait
echo "Inspecting HasControl trait for control() methods:"
rg -Hn "function control" -C3 src/Controls/HasControl.phpLength of output: 589
Resolve TODOs for auto-guess behavior in resolveControlName
We’ve confirmed that no application models override the static control() method (only the HasControl trait provides it), so the fallback “auto guess” path remains lightly tested and the TODO comments at lines 237–242 still stand:
// @TODO: The auto guess here is strange, we specify the models / controls everywhere, is there a better way of doing this ? (In policies guess the model as Laravel is doing ?)
// @TODO: Discussed with Lucas G
if (method_exists($modelName, 'control')) {
return $modelName::control()::class;
}Please address these by one of the following:
- Implement a more robust naming strategy (e.g., Laravel‐style policy guessing).
- Remove or resolve the lingering TODOs.
- If it’s out of scope for this PR, convert them into tracked tickets for follow-up.
closes #15
closes #16
Summary by CodeRabbit
Bug Fixes
Refactor
Tests