From ff4eb2927fddd1587413b76cf86c9c14444b2279 Mon Sep 17 00:00:00 2001 From: Iain Dillingham Date: Tue, 5 Nov 2024 20:29:05 +0000 Subject: [PATCH 1/3] Avoid creating a user unnecessarily We need to create two users to demonstrate that one user was filtered out. However, the `staff_area_administrator` fixture already creates a user, so we need only create one more. --- tests/unit/staff/views/test_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/staff/views/test_users.py b/tests/unit/staff/views/test_users.py index 1860892d4..4f71dab74 100644 --- a/tests/unit/staff/views/test_users.py +++ b/tests/unit/staff/views/test_users.py @@ -589,8 +589,8 @@ def test_userlist_filter_by_invalid_org(rf, staff_area_administrator): def test_userlist_filter_by_role(rf, staff_area_administrator): + # first user is staff area administrator; second user is output publisher UserFactory(roles=[OutputPublisher]) - UserFactory(roles=[ProjectCollaborator]) request = rf.get("/?role=OutputPublisher") request.user = staff_area_administrator From 6f3f0f04601a777627ec8e4cd204fcff5e999207 Mon Sep 17 00:00:00 2001 From: Iain Dillingham Date: Tue, 5 Nov 2024 20:40:48 +0000 Subject: [PATCH 2/3] Parametrize test We parametrize the test to demonstrate that the filter operates on global *and* local roles. Ideally, the test wouldn't depend on us knowing that `OutputPublisher` is a global role and `ProjectDeveloper` is a local role, but making it so would add (arguably unnecessary) complexity. To clarify our intent, we assign descriptive IDs to each parameter set using `pytest.param`. Co-authored-by: Mike Kelly --- tests/unit/staff/views/test_users.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/unit/staff/views/test_users.py b/tests/unit/staff/views/test_users.py index 4f71dab74..5639198dd 100644 --- a/tests/unit/staff/views/test_users.py +++ b/tests/unit/staff/views/test_users.py @@ -588,11 +588,19 @@ def test_userlist_filter_by_invalid_org(rf, staff_area_administrator): assert len(response.context_data["object_list"]) == 0 -def test_userlist_filter_by_role(rf, staff_area_administrator): - # first user is staff area administrator; second user is output publisher - UserFactory(roles=[OutputPublisher]) +@pytest.mark.parametrize( + "role", + [ + pytest.param(OutputPublisher, id="global"), + pytest.param(ProjectDeveloper, id="local"), + ], +) +def test_userlist_filter_by_role(rf, staff_area_administrator, role): + # Set up two users: one as the staff area administrator to request the view and + # be filtered out, the other to be included in the response after filtering. + UserFactory(roles=[role]) - request = rf.get("/?role=OutputPublisher") + request = rf.get(f"/?role={role.__name__}") request.user = staff_area_administrator response = UserList.as_view()(request) From 142d4a6bc97379e6cb6f0120d30f0fd681d03003 Mon Sep 17 00:00:00 2001 From: Iain Dillingham Date: Tue, 5 Nov 2024 20:49:54 +0000 Subject: [PATCH 3/3] Group arrange steps Pedantically, we group arrange steps into a single paragraph because paragraphs are meaningful. --- tests/unit/staff/views/test_users.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/staff/views/test_users.py b/tests/unit/staff/views/test_users.py index 5639198dd..9ec7d970e 100644 --- a/tests/unit/staff/views/test_users.py +++ b/tests/unit/staff/views/test_users.py @@ -599,7 +599,6 @@ def test_userlist_filter_by_role(rf, staff_area_administrator, role): # Set up two users: one as the staff area administrator to request the view and # be filtered out, the other to be included in the response after filtering. UserFactory(roles=[role]) - request = rf.get(f"/?role={role.__name__}") request.user = staff_area_administrator