Move staff area access to a permission #5457
Conversation
StevenMaude
force-pushed
the
steve/move-staff-area-access-to-a-permission
branch
2 times, most recently
from
8e92ccd to
530e3c4
Compare
We'll eventually replace checks against the `StaffAreaAdministrator` role with checks against this permission.
For the `StaffAreaAdministrator` role. This does not remove all checks on the role for staff area access, since there is also `has_role` which we'll update. This removes all users of `require_role` which we can now remove.
Instead of `has_role`. This replaces the remaining checks against the `StaffAreaAdministrator` role to access the staff area.
StevenMaude
force-pushed
the
steve/move-staff-area-access-to-a-permission
branch
from
530e3c4 to
9bdb874
Compare
|
|
||
| if application.approved_at and not has_role(request.user, StaffAreaAdministrator): | ||
| if application.approved_at and not has_permission( | ||
| request.user, permissions.staff_area_access |
There was a problem hiding this comment.
thought: I've tried to keep the diffs minimal, and easy to understand, because there are a large number of changes across multiple files.
So:
- sometimes we're using
permissionsas an import, because we already had that imported, and then accessingstaff_area_accesson that - other times, we're importing
staff_area_accessdirectly and using that, which is an exchange forStaffAreaAdministrator
If it's thought preferable to make all the imports using the permissions consistent, perhaps to explicitly mark them as permissions.staff_area_access, that's also fine.
Though, it may be that the permissions get refactored into a class in #5459 subsequently, so I'm not sure it's worth worrying about too much now.
There was a problem hiding this comment.
I think either from is okay, personally I find has_permission(user, staff_area_access) or require_permission(staff_area_access) more readable but permissions.staff_area_access has advantages for some IDEs. Consistency is nice but I am not worried about it here.
I would not bother changing here as you say #5459 will probably change again, and maybe we will make future changes in this area.
Fixes #5445.
TODO:
has_role