Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use ->withPivot() for teamed relationships #2679

Merged
merged 3 commits into from
Jun 22, 2024
Merged

Conversation

juliangums
Copy link
Contributor

The previous implementation is giving me issues when querying ->getPivotColumns() for example. It should probably use ->withPivot() to indicate there is a pivot value.

@juliangums
Copy link
Contributor Author

Breaking tests so I’ll close but I am wondering if this is something to look in to?

@juliangums juliangums closed this Jun 10, 2024
@erikn69
Copy link
Contributor

erikn69 commented Jun 10, 2024

Breaking tests

that's because you did it wrong, must be:

if (! app(PermissionRegistrar::class)->teams) {
    return $relation;
}

$teamsKey = app(PermissionRegistrar::class)->teamsKey;
$relation->withPivot($teamsKey);
$teamField = config('permission.table_names.roles').'.'.$teamsKey;

return $relation->wherePivot($teamsKey, getPermissionsTeamId())
    ->where(fn ($q) => $q->whereNull($teamField)->orWhere($teamField, getPermissionsTeamId()));

Also, there is more places, not only there

if (! app(PermissionRegistrar::class)->teams) {
return $relation;
}
return $relation->wherePivot(app(PermissionRegistrar::class)->teamsKey, getPermissionsTeamId());

@juliangums
Copy link
Contributor Author

juliangums commented Jun 11, 2024

Oh yeah of course, I should have checked if teams is activated first before trying to access the config values. Corrected this now.

@juliangums juliangums reopened this Jun 11, 2024
@juliangums
Copy link
Contributor Author

Also added this for permissions

@juliangums
Copy link
Contributor Author

@erikn69 good to merge?

@erikn69
Copy link
Contributor

erikn69 commented Jun 21, 2024

I am not a maintainer,
I only reviewed the code for you, and I have not reviewed this functionality

@juliangums
Copy link
Contributor Author

@freekmurze @drbyte is this ok to merge?

@drbyte drbyte merged commit ee0817f into spatie:main Jun 22, 2024
52 checks passed
@juliangums
Copy link
Contributor Author

@drbyte thanks

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants