Skip to content
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

Remove roles count check from UserPermissions #6658

Merged

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Sep 8, 2024

Description

Summary of changes

  • Remove roles count check from UserPermissions::canChangeRole()

Reasoning

It's not a permissions question but a UI thing to disable a button if only one role is available. Permission-wise, a user should be able to change a role even if only to the one available option.

This change is also needed to avoid a circular loop:

  • $user->roles('change') filters the role collection by those that can be changed
  • Check the permission to filter the roles collection
  • The permission method itself would need to call $user->roles('change')

This is why this check should not be part of the permissions method.

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

@distantnative distantnative force-pushed the enhancement/user-permissions-no-count branch from ac44dfd to a034837 Compare September 20, 2024 14:11
@distantnative distantnative marked this pull request as ready for review September 20, 2024 14:20
@distantnative distantnative requested a review from a team September 20, 2024 14:20
@bastianallgeier bastianallgeier added this to the 4.5.0 milestone Sep 23, 2024
@bastianallgeier bastianallgeier merged commit 12abc46 into develop-minor Sep 23, 2024
14 checks passed
@bastianallgeier bastianallgeier deleted the enhancement/user-permissions-no-count branch September 23, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants