Show users with STAFF_AREA_ACCESS as copilot choices.#5752
Open
mikerkelly wants to merge 9 commits intomainfrom
Open
Show users with STAFF_AREA_ACCESS as copilot choices.#5752mikerkelly wants to merge 9 commits intomainfrom
STAFF_AREA_ACCESS as copilot choices.#5752mikerkelly wants to merge 9 commits intomainfrom
Conversation
mikerkelly
force-pushed
the
mikerkelly/project-create/form-reduce-copilot-list
branch
2 times, most recently
from
0551040 to
d9619f3
Compare
To mirror GLOBAL_ROLES. These are not used yet but I intend to use them in future commits, breaking up the changes. The tests were using the & intersection set operator, which tests for intersection, not equality. Checking for equality seems more robust. Therefore the test was missing some new roles. Added these.
This function gets a set of Roles with a specific permission. This can be useful when querying objects with role fields, the permissions cannot be queried directly as they are not represented in the database. Instead, you can query on something like `roles__overlap(permission)`. This is not used yet, but I intend to use it soon.
The list of all users displayed for the `copilot` field in the `ProjectCreateForm` and `ProjectEditForm` is very long in production -- anyone with a GitHub account can log in. Shortening it to a shorter valid set makes it slightly easier to navigate. This also makes manual errors less likely. Ideally we would use a copilot-specific permission or role but that does not exist (yet?). So let's use `STAFF_AREA_ACCESS` as in practice all copilots do currently have that access for accessing the copilot dashboards (and formerly the application process). Adjust the form field QuerySet to reduce the set of choices to those with roles that have the required permission. A function that returns User QuerySet is a User manager method so put it there, and this means that `with_permission` can be used more widely. `potential_copilots` has the permission as a parameter to help with testing. Add `user_with_fake_role_factory` test fixture to aid with making `User` with fake permissions, to help with decoupling tests from specific roles and permissions. The form tests use a fixture to patch the `QuerySet` returned by the `User.objects.potential_copilots` so that the other tests can be decoupled from the specific permission.
This does not seem to break anything and generally it is helpful to delay conversion to string as late as possible in case something downstream wants to handle the original data type. Presumably the form handling code casts to string itself.
We can use the `user_with_fake_role_factory` fixture to generate fake roles with a given permission, for testing purposes. This decouples from the specific roles and focuses on what actually drives access in the application, the permission. The roles are really data represented as code, I feel they should not be under test.
This allows creating a fake role with multiple permissions by passing them in.
This is a bit more ergonomic than:
```python
UserFactory(
roles=[
role_factory(permission=Permission.RELEASE_FILE_VIEW),
role_factory(permission=Permission.SNAPSHOT_PUBLISH),
]
)
```
It also enables the extension to the `user_with_fake_role_factory` fixture. Now
we can just do:
```python
user = user_with_fake_role_factory(
permission=[Permission.RELEASE_FILE_VIEW, Permission.SNAPSHOT_PUBLISH]
)
```
Not changing the function signature to `permissions` when there are many
existing examples. Easiest to accept either single or iterable parameter.
We can use the `user_with_fake_role_factory` fixture to generate fake roles with given permissions, for testing purposes. This decouples from the specific roles and focuses on what actually drives access in the application, the permission. The roles are really data represented as code, I feel they should not be under test.
This will make later commits have a smaller diff.
The ProjectAddMember view is driven by the USER_EDIT_PROJECT_ROLES permission, not a specific role. We can use the `user_with_fake_role_factory` fixture to generate fake roles with a given permission,for testing purposes. This decouples from the specific roles and focuses on what actually drives access in the application, the permission. The roles are really data represented as code, I feel they should not be under test.
mikerkelly
force-pushed
the
mikerkelly/project-create/form-reduce-copilot-list
branch
from
d9619f3 to
ed959fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5750.
The list of all users displayed for the
copilotfield in theProjectCreateFormandProjectEditFormis very long in production -- anyone with a GitHub account can log in. Shortening it to a shorter valid set makes it slightly easier to navigate. This also makes manual errors less likely.Ideally we would use a copilot-specific permission or role but that does not exist (yet?). So let's use
STAFF_AREA_ACCESSas in practice all copilots do currently have that access for accessing the copilot dashboards (and formerly the application process).Adjust the form field QuerySet to reduce the set of choices to those with roles that have the required permission. A function that returns User QuerySet is a User manager method so put it there, and this means that
with_permissioncan be used more widely.potential_copilotshas the permission as a parameter to help with testing.Add
ALL_ROLESandroles_with_permissionutility for making theQuerySetwithout hardcoding in theStaffAreaAdministratorrole. Ideally all application behaviour should be driven by permissions, not roles, to decouple changes to the roles from the specific views et cetera.Add
user_with_fake_role_factorytest fixture to aid with makingUserwith fake permissions, to help with decoupling tests from specific roles and permissions. I updated existing tests using this where needed due to the copilot field change to show how this can be used, and also applied it to the rest of the integration tests and theProjectAddMemberunit tests for illustrative purposes.The form tests use a fixture to patch the
QuerySetreturned by theUser.objects.potential_copilotsso that the other tests can be decoupled from the specific permission.