From 615b8467ef865dd5501d0d149da7b37c2c0bce88 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 22:38:12 +0300 Subject: [PATCH] fix(review): observation dept_manager button gate + appreciation status guard Two issues from the whole-branch review: 1. (Important) Observation detail's can_respond_to_department context var did not include is_department_manager, so even though Task 2 let dept managers pass the view's permission check, the 'Submit Response' button stayed hidden from them. Now the context var matches the view predicate. Regression test added. 2. (Minor) appreciation_list had no guard against now-invalid status query params (?status=acknowledged from an old bookmark would show an unexplained empty list). Unknown values are now silently reset to 'all statuses'. --- apps/appreciation/ui_views.py | 5 +++ .../observations/test_workflow_realignment.py | 40 +++++++++++++++++++ apps/observations/views.py | 2 +- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/apps/appreciation/ui_views.py b/apps/appreciation/ui_views.py index 2ab3c2f..9154751 100644 --- a/apps/appreciation/ui_views.py +++ b/apps/appreciation/ui_views.py @@ -63,6 +63,11 @@ def appreciation_list(request): queryset = queryset.none() status_filter_val = request.GET.get("status", "") + # Guard: silently reset unknown/legacy status values (e.g. a bookmarked + # ?status=acknowledged from before the 3-state collapse) to "all" so the + # user doesn't see an unexplained empty list. + if status_filter_val and status_filter_val not in dict(AppreciationStatus.choices): + status_filter_val = "" if status_filter_val: queryset = queryset.filter(status=status_filter_val) diff --git a/apps/observations/test_workflow_realignment.py b/apps/observations/test_workflow_realignment.py index 884cf86..ccee539 100644 --- a/apps/observations/test_workflow_realignment.py +++ b/apps/observations/test_workflow_realignment.py @@ -67,3 +67,43 @@ class TestObservationDepartmentResponsePermission(TestCase): ) assert response.status_code == 302, \ f"dept_manager of other dept should be redirected (denied), got {response.status_code}" + + +@pytest.mark.django_db +class TestObservationDetailRespondButtonGate(TestCase): + """The 'Submit Response' button must be visible to dept_manager of the + assigned department — not just pass the view's permission check. + + Locks in the fix for the Important issue found in whole-branch review: + Task 2 added dept_manager to the view predicate but the template context + var `can_respond_to_department` was missed, so the button stayed hidden. + """ + + def setUp(self): + self.hospital = Hospital.objects.create(name="Test Hospital Gate", code="TH04") + self.dept = Department.objects.create(name="Dept Gate", hospital=self.hospital, code="GT") + + _ensure_group("Department Manager") + self.dept_manager = User.objects.create_user( + username="gdmgr", email="gdmgr@test", password="x", department=self.dept + ) + self.dept_manager.groups.add(Group.objects.get(name="Department Manager")) + + self.observation = Observation.objects.create( + description="Gate test observation", + hospital=self.hospital, + assigned_department=self.dept, + sent_to_department=True, + department_responded_at=None, + status="in_progress", + ) + + def test_dept_manager_sees_respond_button(self): + """The department detail page renders the respond control for dept managers.""" + self.client.force_login(self.dept_manager) + response = self.client.get( + reverse("observations:observation_detail", kwargs={"pk": self.observation.pk}) + ) + assert response.status_code == 200 + assert response.context["can_respond_to_department"] is True, \ + "dept_manager of assigned dept must see can_respond_to_department=True" diff --git a/apps/observations/views.py b/apps/observations/views.py index 91d38fa..0f3dbb5 100644 --- a/apps/observations/views.py +++ b/apps/observations/views.py @@ -653,7 +653,7 @@ def observation_detail(request, pk): "can_triage": user.has_perm("observations.triage_observation") or user.is_px_admin() or user.is_px_employee(), "can_convert": user.is_px_admin() or user.is_hospital_admin() or user.is_px_management() or user.is_px_employee(), "can_send_to_department": user.is_px_admin() or user.is_hospital_admin() or user.is_px_management() or user.is_px_employee() or user.is_department_manager() or user.is_px_management(), - "can_respond_to_department": user.is_px_admin() or user.is_hospital_admin() or user.is_px_management() or user.is_px_employee() or (user.is_champion() and observation.assigned_department == user.department), + "can_respond_to_department": user.is_px_admin() or user.is_hospital_admin() or user.is_px_management() or user.is_px_employee() or (user.is_champion() and observation.assigned_department == user.department) or (user.is_department_manager() and observation.assigned_department == user.department), "can_send_reminder": user.is_px_admin() or user.is_hospital_admin() or user.is_px_management() or user.is_px_employee(), "can_delete": user.is_px_admin() or user.is_hospital_admin() or user.is_px_management() or user.is_px_employee(), "can_admin": user.is_px_admin() or user.is_hospital_admin() or user.is_px_employee(),