Conversation
|
@freekmurze |
There was a problem hiding this comment.
Pull request overview
Adds first-class “team” accessors/scopes to the HasRoles trait so consumers can query models by team (and retrieve teams), with supporting configuration, exceptions, and tests.
Changes:
- Added
teams()relationship plusscopeTeam()/scopeWithoutTeam()toHasRoles. - Introduced
TeamsNotEnabledandTeamModelNotConfiguredexceptions and addedmodels.teamconfiguration. - Added a new Pest test suite validating team scoping behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Traits/TeamScopeTest.php | Adds coverage for team() / withoutTeam() scopes and related exception behavior. |
| src/Traits/HasRoles.php | Implements teams() relationship and team scoping query helpers. |
| src/PermissionRegistrar.php | Adds team model accessors/mutator on the registrar. |
| src/Exceptions/TeamsNotEnabled.php | New exception for calling team APIs when teams are disabled. |
| src/Exceptions/TeamModelNotConfigured.php | New exception for calling team APIs without a configured team model. |
| config/permission.php | Adds models.team configuration entry (default null). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| $teamIds = array_map(fn ($team) => $team instanceof $teamClass ? $team->getKey() : $team, $teams); | ||
|
|
There was a problem hiding this comment.
scopeTeam()'s phpdoc says it accepts an \Illuminate\Database\Eloquent\Model, but the implementation only converts instances of the configured team class; any other Eloquent model instance will be left as an object in $teamIds, which will later be passed into whereIn() and can error at runtime. Consider either (a) validating and throwing an explicit exception when a non-team model is provided, or (b) updating the accepted types/docs to only allow IDs, Collections/arrays of IDs, or instances of the configured team model.
| $teamIds = array_map(fn ($team) => $team instanceof $teamClass ? $team->getKey() : $team, $teams); | |
| $teamIds = array_map(function ($team) use ($teamClass) { | |
| if ($team instanceof $teamClass) { | |
| return $team->getKey(); | |
| } | |
| if (is_int($team) || is_string($team)) { | |
| return $team; | |
| } | |
| throw new \InvalidArgumentException( | |
| 'The scopeTeam() method expects team IDs, an array/Collection of IDs, or instances of the configured team model.' | |
| ); | |
| }, $teams); |
| if ($teamClass) { | ||
| app()->bind(config('permission.models.team'), $teamClass); | ||
| } |
There was a problem hiding this comment.
setTeamClass() binds config('permission.models.team') to $teamClass immediately after setting that config value, which effectively binds the class name to itself and doesn't match the existing pattern used by setRoleClass()/setPermissionClass() (binding a contract/interface to an implementation). This binding is likely a no-op and may confuse users; consider removing it or introducing/binding to a dedicated Team contract/abstract instead.
| if ($teamClass) { | |
| app()->bind(config('permission.models.team'), $teamClass); | |
| } |
| /** | ||
| * A model may be part of multiple teams. | ||
| */ | ||
| public function teams(): BelongsToMany | ||
| { | ||
| if (! app(PermissionRegistrar::class)->teams) { | ||
| throw TeamsNotEnabled::create(); | ||
| } | ||
|
|
||
| if (! config('permission.models.team')) { | ||
| throw TeamModelNotConfigured::create(); | ||
| } | ||
|
|
||
| return $this->morphToMany( | ||
| config('permission.models.team'), | ||
| 'model', | ||
| config('permission.table_names.model_has_roles'), | ||
| config('permission.column_names.model_morph_key'), | ||
| app(PermissionRegistrar::class)->teamsKey | ||
| )->distinct(); | ||
| } |
There was a problem hiding this comment.
New teams() relationship behavior (including the distinct() de-duplication) is not covered by tests: current tests only cover scopeTeam/scopeWithoutTeam and the exception path. Adding at least one test asserting that $user->teams() returns the expected unique team models (and that multiple roles in the same team do not duplicate teams) would help prevent regressions.
No description provided.