-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added permission scope to HasRoles trait #454
Added permission scope to HasRoles trait #454
Conversation
This is very similar to what I had come up with. It appears to sufficiently allow the ability to scope users who are authorized for a specified permission whether directly or via a role. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I like the functionality, but I think it can be polished a bit more.
src/Traits/HasRoles.php
Outdated
$permissions = $permissions->toArray(); | ||
} | ||
|
||
if (! is_array($permissions)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use that shiny new array_wrap
function here.
src/Traits/HasRoles.php
Outdated
} | ||
|
||
$rolesWithPermissions = collect([]); | ||
$permissions = array_map(function ($permission) use (&$rolesWithPermissions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be cleaned up greatly. Right now this map operation does two things. Making sure the permissions are models and building up that rolesWithPermissions
. Do this in two steps (with two map operations I guess) for clarity (so when can use that nasty &
)
src/Traits/HasRoles.php
Outdated
$permissions = array_map(function ($permission) use (&$rolesWithPermissions) { | ||
if ($permission instanceof Permission) { | ||
$permissionModel = $permission; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O how I dislike else
statements. Refactor this to it's own protected
function where you can use an early return.
src/Traits/HasRoles.php
Outdated
|
||
$rolesWithPermissions = $rolesWithPermissions->unique(); | ||
|
||
return $query-> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
tests/HasPermissionsTest.php
Outdated
$user1 = User::create(['email' => 'user1@test.com']); | ||
$user2 = User::create(['email' => 'user2@test.com']); | ||
$user1->givePermissionTo(['edit-articles', 'edit-news']); | ||
// giving user2 permission throgth the role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this comment, I think the code is clear enough.
@@ -52,4 +52,107 @@ public function it_can_revoke_a_permission_from_a_user() | |||
|
|||
$this->assertFalse($this->testUser->hasPermissionTo($this->testUserPermission)); | |||
} | |||
|
|||
/** @test */ | |||
public function it_can_scope_users_using_a_string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can always appreciate a PR that adds tests 👍
tests/HasPermissionsTest.php
Outdated
$user1 = User::create(['email' => 'user1@test.com']); | ||
$user2 = User::create(['email' => 'user2@test.com']); | ||
$user1->givePermissionTo(['edit-articles', 'edit-news']); | ||
// giving user2 permission throgth the role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment
tests/HasPermissionsTest.php
Outdated
$user1 = User::create(['email' => 'user1@test.com']); | ||
$user2 = User::create(['email' => 'user2@test.com']); | ||
$user1->givePermissionTo(['edit-articles', 'edit-news']); | ||
// giving user2 permission throgth the role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what do to 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's headed in the good direction, but there's still some more polishing possible
src/Traits/HasRoles.php
Outdated
*/ | ||
public function scopePermission(Builder $query, $permissions): Builder | ||
{ | ||
if ($permissions instanceof Collection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put everything between line 96 and 108 in it's own dedicated protected function convertToPermissionModels
src/Traits/HasRoles.php
Outdated
|
||
$rolesWithPermissions = collect([]); | ||
|
||
foreach ($permissions as $permission) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to me that this is a reduce operation. Let's use array_reduce
here.
Last remark: could you also add a little explanation + example of the new functionality in the readme? |
Thanks! |
my pleasure, thank you for the notes. |
No description provided.