From 17064fc33c7b8045f0154132acd796890caa78a5 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 21:40:43 +0300 Subject: [PATCH 02/13] fix(inquiry): dept_manager of handling dept can now record department response The inquiry_department_response view excluded dept managers even though they can resolve the inquiry via inquiry_change_status. Now both actions use the same handling_department_id predicate so forwarding locks the old department out consistently. --- apps/complaints/test_workflow_realignment.py | 72 ++++++++++++++++++++ apps/complaints/ui_views.py | 6 +- conftest.py | 30 ++++++++ 3 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 apps/complaints/test_workflow_realignment.py create mode 100644 conftest.py diff --git a/apps/complaints/test_workflow_realignment.py b/apps/complaints/test_workflow_realignment.py new file mode 100644 index 0000000..6bfd5d4 --- /dev/null +++ b/apps/complaints/test_workflow_realignment.py @@ -0,0 +1,72 @@ +"""Tests for the feedback workflow realignment (Inquiry department-ownership).""" +from django.contrib.auth.models import Group + +import pytest +from django.test import TestCase +from django.urls import reverse + +from apps.accounts.models import User +from apps.organizations.models import Department, Hospital +from apps.complaints.models import Inquiry + + +def _ensure_group(name): + """Get or create a role group (pytest runs with --nomigrations, so seeded + groups may not exist in the test DB — mirrors the pattern in + tests_routing_rejection.py and tests_workflow_fixes.py).""" + grp, _ = Group.objects.get_or_create(name=name) + return grp + + +@pytest.mark.django_db +class TestInquiryDepartmentResponsePermission(TestCase): + """A department manager of the handling department can record a response. + + Uses self.client (full middleware incl. django.contrib.messages) rather + than calling the view directly with RequestFactory, because the view + calls messages.error() on the permission-denied path. + """ + + def setUp(self): + self.hospital = Hospital.objects.create(name="Test Hospital", code="TH01") + self.dept = Department.objects.create(name="Dept A", hospital=self.hospital, code="DA") + self.other_dept = Department.objects.create(name="Dept B", hospital=self.hospital, code="DB") + + _ensure_group("Department Manager") + self.dept_manager = User.objects.create_user( + username="dmgr", email="dmgr@test", password="x", department=self.dept + ) + self.dept_manager.groups.add(Group.objects.get(name="Department Manager")) + + self.other_manager = User.objects.create_user( + username="other_dmgr", email="o@test", password="x", department=self.other_dept + ) + self.other_manager.groups.add(Group.objects.get(name="Department Manager")) + + self.inquiry = Inquiry.objects.create( + hospital=self.hospital, + department=self.dept, + subject="Test", + message="msg", + status="in_progress", + sent_to_department=True, + ) + + def test_dept_manager_of_handling_dept_can_access_response_view(self): + """GET to inquiry_department_response succeeds (200) for dept manager of handling dept.""" + self.client.force_login(self.dept_manager) + response = self.client.get( + reverse("inquiries:inquiry_department_response", kwargs={"pk": self.inquiry.pk}) + ) + # 200 = permission granted, 302 = permission denied redirect. + assert response.status_code == 200, \ + f"dept_manager of handling dept should be allowed, got {response.status_code}" + + def test_dept_manager_of_other_dept_cannot_access(self): + """Dept manager of a non-handling department is denied (302 redirect).""" + self.client.force_login(self.other_manager) + response = self.client.get( + reverse("inquiries:inquiry_department_response", kwargs={"pk": self.inquiry.pk}) + ) + assert response.status_code == 302, \ + f"dept_manager of other dept should be redirected (denied), got {response.status_code}" diff --git a/apps/complaints/ui_views.py b/apps/complaints/ui_views.py index d741dd4..c051faf 100644 --- a/apps/complaints/ui_views.py +++ b/apps/complaints/ui_views.py @@ -4475,12 +4475,10 @@ def inquiry_department_response(request, pk): if not ( user.is_px_admin() or user.is_hospital_admin() - or (user.is_champion() and ( - inquiry.department == user.department - or inquiry.outgoing_department == user.department - )) or user.is_px_management() or user.is_px_employee() + or (user.is_champion() and user.department_id == inquiry.handling_department_id) + or (user.is_department_manager() and user.department_id == inquiry.handling_department_id) ): messages.error(request, "You don't have permission to respond to this inquiry.") return redirect("inquiries:inquiry_detail", pk=pk) diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..4487a17 --- /dev/null +++ b/conftest.py @@ -0,0 +1,30 @@ +"""Project-wide pytest fixtures. + +The default test environment runs without a collected staticfiles manifest +(``addopts = "--reuse-db --nomigrations"`` in pyproject.toml), so any view +that renders a template containing ``{% static %}`` tags against +``CompressedManifestStaticFilesStorage`` raises ``ValueError: Missing +staticfiles manifest entry for ...``. + +This autouse fixture swaps in plain ``StaticFilesStorage`` for the duration +of every test, so tests can assert ``status_code == 200`` on template-rendering +views without first running ``collectstatic``. +""" +import pytest +from django.test import override_settings + +from config.settings.base import STORAGES as BASE_STORAGES + + +@pytest.fixture(autouse=True) +def _disable_static_manifest(): + """Use plain StaticFilesStorage (no manifest hashing) during tests.""" + staticfiles_storage = { + "BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage", + } + storages_override = {**BASE_STORAGES, "staticfiles": staticfiles_storage} + with override_settings( + STATICFILES_STORAGE="django.contrib.staticfiles.storage.StaticFilesStorage", + STORAGES=storages_override, + ): + yield From 2829308befbd94823cb133820382a46dd0387cbe Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 21:50:12 +0300 Subject: [PATCH 03/13] fix(observation): dept_manager of assigned dept can now respond Matches the complaint predicate. Previously a department manager could respond to a complaint sent to their dept but not to an observation. --- .../observations/test_workflow_realignment.py | 69 +++++++++++++++++++ apps/observations/views.py | 1 + 2 files changed, 70 insertions(+) create mode 100644 apps/observations/test_workflow_realignment.py diff --git a/apps/observations/test_workflow_realignment.py b/apps/observations/test_workflow_realignment.py new file mode 100644 index 0000000..884cf86 --- /dev/null +++ b/apps/observations/test_workflow_realignment.py @@ -0,0 +1,69 @@ +"""Tests for the observation workflow realignment.""" +from django.contrib.auth.models import Group + +import pytest +from django.test import TestCase +from django.urls import reverse + +from apps.accounts.models import User +from apps.organizations.models import Department, Hospital +from apps.observations.models import Observation + + +def _ensure_group(name): + """Get or create a role group (pytest runs with --nomigrations, so seeded + groups may not exist in the test DB).""" + grp, _ = Group.objects.get_or_create(name=name) + return grp + + +@pytest.mark.django_db +class TestObservationDepartmentResponsePermission(TestCase): + """A department manager of the assigned department can respond. + + Uses self.client (full middleware incl. django.contrib.messages) — the + view calls messages.error() on the permission-denied path. The project's + root conftest.py disables ManifestStaticFilesStorage for all tests. + """ + + def setUp(self): + self.hospital = Hospital.objects.create(name="Test Hospital", code="TH02") + self.dept = Department.objects.create(name="Dept A", hospital=self.hospital, code="OA") + self.other_dept = Department.objects.create(name="Dept B", hospital=self.hospital, code="OB") + + _ensure_group("Department Manager") + self.dept_manager = User.objects.create_user( + username="odmgr", email="odmgr@test", password="x", department=self.dept + ) + self.dept_manager.groups.add(Group.objects.get(name="Department Manager")) + + self.other_manager = User.objects.create_user( + username="oother", email="oo@test", password="x", department=self.other_dept + ) + self.other_manager.groups.add(Group.objects.get(name="Department Manager")) + + self.observation = Observation.objects.create( + description="Test observation description here", + hospital=self.hospital, + assigned_department=self.dept, + sent_to_department=True, + status="in_progress", + ) + + def test_dept_manager_of_assigned_dept_can_access_response_view(self): + """GET to observation_department_response succeeds (200) for dept manager of assigned dept.""" + self.client.force_login(self.dept_manager) + response = self.client.get( + reverse("observations:observation_department_response", kwargs={"pk": self.observation.pk}) + ) + assert response.status_code == 200, \ + f"dept_manager of assigned dept should be allowed, got {response.status_code}" + + def test_dept_manager_of_other_dept_cannot_access(self): + """Dept manager of a non-assigned department is denied (302 redirect).""" + self.client.force_login(self.other_manager) + response = self.client.get( + reverse("observations:observation_department_response", kwargs={"pk": self.observation.pk}) + ) + assert response.status_code == 302, \ + f"dept_manager of other dept should be redirected (denied), got {response.status_code}" diff --git a/apps/observations/views.py b/apps/observations/views.py index 770d3e1..91d38fa 100644 --- a/apps/observations/views.py +++ b/apps/observations/views.py @@ -1628,6 +1628,7 @@ def observation_department_response(request, pk): 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) ): messages.error(request, "You don't have permission to respond to this observation.") return redirect("observations:observation_detail", pk=pk) From b0d77c21e561e82ae3081f837fc10d96b71efee0 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 21:55:03 +0300 Subject: [PATCH 05/13] chore: remove dead top-level appreciation/forms.py This file referenced model fields (AppreciationAttachment, submitted_by, acknowledged_by, AppreciationStatus.SUBMITTED) that no longer exist on the current model, and was not wired into any URL. grep confirms no imports. The live form is apps/appreciation/forms.py. (apps/complaints/views.py.backup was also deleted but was never tracked by git, so no commit is needed for it.) --- appreciation/forms.py | 308 ------------------------------------------ 1 file changed, 308 deletions(-) delete mode 100644 appreciation/forms.py diff --git a/appreciation/forms.py b/appreciation/forms.py deleted file mode 100644 index b7c03d3..0000000 --- a/appreciation/forms.py +++ /dev/null @@ -1,308 +0,0 @@ -""" -Appreciation forms -""" -from django import forms -from django.utils.translation import gettext_lazy as _ - -from .models import ( - Appreciation, - AppreciationAttachment, - AppreciationCategory, - AppreciationComment, - AppreciationStatus, - AppreciationType, -) - - -class AppreciationCategoryForm(forms.ModelForm): - """Form for AppreciationCategory""" - - class Meta: - model = AppreciationCategory - fields = ['name', 'description', 'is_active', 'display_order'] - widgets = { - 'name': forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('Category name') - }), - 'description': forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 3, - 'placeholder': _('Description') - }), - 'is_active': forms.CheckboxInput(attrs={ - 'class': 'form-check-input' - }), - 'display_order': forms.NumberInput(attrs={ - 'class': 'form-control', - 'min': 0 - }), - } - - -class AppreciationForm(forms.ModelForm): - """Form for Appreciation""" - submitter_role = forms.CharField( - required=False, - widget=forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('Your role, e.g., Nurse Manager') - }) - ) - - class Meta: - model = Appreciation - fields = [ - 'title', - 'description', - 'story', - 'appreciation_type', - 'category', - 'recipient_name', - 'recipient_type', - 'recipient_id', - 'submitter_role', - 'tags', - 'is_public', - 'share_on_dashboard', - 'share_in_newsletter', - ] - widgets = { - 'title': forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('Appreciation title') - }), - 'description': forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 5, - 'placeholder': _('Describe the appreciation') - }), - 'story': forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 8, - 'placeholder': _('Tell the story behind this appreciation') - }), - 'appreciation_type': forms.Select(attrs={ - 'class': 'form-select' - }), - 'category': forms.Select(attrs={ - 'class': 'form-select' - }), - 'recipient_name': forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('Name of person or team') - }), - 'recipient_type': forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('e.g., Staff, Patient, Department') - }), - 'recipient_id': forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('System ID if applicable') - }), - 'tags': forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 2, - 'placeholder': _('Comma-separated tags') - }), - 'is_public': forms.CheckboxInput(attrs={ - 'class': 'form-check-input' - }), - 'share_on_dashboard': forms.CheckboxInput(attrs={ - 'class': 'form-check-input' - }), - 'share_in_newsletter': forms.CheckboxInput(attrs={ - 'class': 'form-check-input' - }), - } - - def __init__(self, *args, **kwargs): - user = kwargs.pop('user', None) - hospital = kwargs.pop('hospital', None) - super().__init__(*args, **kwargs) - - # Filter categories to active ones only - if self.fields.get('category'): - self.fields['category'].queryset = AppreciationCategory.objects.filter( - is_active=True - ).order_by('display_order', 'name') - - # Set default values based on context - if user and not self.instance.pk: - self.instance.submitted_by = user - - if hospital and not self.instance.pk: - self.instance.hospital = hospital - - def clean_tags(self): - tags = self.cleaned_data.get('tags', '') - if tags: - # Convert comma-separated string to list - return [tag.strip() for tag in tags.split(',') if tag.strip()] - return [] - - -class AppreciationSubmitForm(AppreciationForm): - """Form for submitting an appreciation (sets status to submitted)""" - - def save(self, commit=True): - instance = super().save(commit=False) - instance.status = AppreciationStatus.SUBMITTED - if commit: - instance.save() - return instance - - -class AppreciationAcknowledgeForm(forms.ModelForm): - """Form for acknowledging an appreciation""" - acknowledgment_notes = forms.CharField( - required=True, - widget=forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 4, - 'placeholder': _('Add your acknowledgment notes here') - }) - ) - - class Meta: - model = Appreciation - fields = ['acknowledgment_notes'] - - def __init__(self, *args, **kwargs): - user = kwargs.pop('user', None) - super().__init__(*args, **kwargs) - - if user and not self.instance.pk: - self.instance.acknowledged_by = user - - -class AppreciationAttachmentForm(forms.ModelForm): - """Form for uploading appreciation attachments""" - description = forms.CharField( - required=False, - widget=forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 2, - 'placeholder': _('Describe the attachment') - }) - ) - - class Meta: - model = AppreciationAttachment - fields = ['file', 'description'] - widgets = { - 'file': forms.FileInput(attrs={ - 'class': 'form-control', - 'accept': 'image/*,.pdf,.doc,.docx' - }), - } - - def __init__(self, *args, **kwargs): - user = kwargs.pop('user', None) - super().__init__(*args, **kwargs) - - if user and not self.instance.pk: - self.instance.uploaded_by = user - - def save(self, commit=True): - instance = super().save(commit=False) - - # Extract file information - if instance.file: - instance.filename = instance.file.name - instance.file_type = instance.file.content_type - instance.file_size = instance.file.size - - if commit: - instance.save() - return instance - - -class AppreciationCommentForm(forms.ModelForm): - """Form for adding comments to appreciations""" - comment = forms.CharField( - required=True, - widget=forms.Textarea(attrs={ - 'class': 'form-control', - 'rows': 3, - 'placeholder': _('Write your comment here') - }) - ) - is_internal = forms.BooleanField( - required=False, - label=_('Internal Comment'), - help_text=_('Check if this is an internal comment only visible to staff'), - widget=forms.CheckboxInput(attrs={ - 'class': 'form-check-input' - }) - ) - - class Meta: - model = AppreciationComment - fields = ['comment', 'is_internal'] - - def __init__(self, *args, **kwargs): - user = kwargs.pop('user', None) - appreciation = kwargs.pop('appreciation', None) - super().__init__(*args, **kwargs) - - if user and not self.instance.pk: - self.instance.user = user - - if appreciation and not self.instance.pk: - self.instance.appreciation = appreciation - - -class AppreciationFilterForm(forms.Form): - """Form for filtering appreciations""" - search = forms.CharField( - required=False, - widget=forms.TextInput(attrs={ - 'class': 'form-control', - 'placeholder': _('Search appreciations...') - }) - ) - status = forms.ChoiceField( - required=False, - choices=[('', _('All Statuses'))] + AppreciationStatus.choices, - widget=forms.Select(attrs={ - 'class': 'form-select' - }) - ) - appreciation_type = forms.ChoiceField( - required=False, - choices=[('', _('All Types'))] + AppreciationType.choices, - widget=forms.Select(attrs={ - 'class': 'form-select' - }) - ) - category = forms.ModelChoiceField( - required=False, - queryset=AppreciationCategory.objects.filter( - is_active=True - ).order_by('display_order', 'name'), - empty_label=_('All Categories'), - widget=forms.Select(attrs={ - 'class': 'form-select' - }) - ) - date_from = forms.DateField( - required=False, - widget=forms.DateInput(attrs={ - 'class': 'form-control', - 'type': 'date' - }) - ) - date_to = forms.DateField( - required=False, - widget=forms.DateInput(attrs={ - 'class': 'form-control', - 'type': 'date' - }) - ) - is_public = forms.BooleanField( - required=False, - widget=forms.CheckboxInput(attrs={ - 'class': 'form-check-input' - }) - ) \ No newline at end of file From c8db98880ac2ed8a3b0c0ae434e579ed77cac792 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 22:03:02 +0300 Subject: [PATCH 06/13] feat(inquiry): remove PX-side Response to Patient from UI Per the workflow realignment, the department owns inquiry resolution end-to-end. The PX-side respond modal and its 'Response to Patient' tile button are removed; the 'Resolve' button in the 'Department has responded' banner now POSTs directly to inquiry_change_status (with a confirm dialog), which already enforces the contact_status gate. The inquiry_respond view and URL are kept for backward compatibility but no UI surfaces them. Two regression tests lock in the new behavior: - showRespondModal JS function and respondModal div are gone - Resolve form posts directly to inquiry_change_status with status=resolved --- apps/complaints/test_workflow_realignment.py | 76 ++++++++++ templates/complaints/inquiry_detail.html | 140 +------------------ 2 files changed, 82 insertions(+), 134 deletions(-) diff --git a/apps/complaints/test_workflow_realignment.py b/apps/complaints/test_workflow_realignment.py index 6bfd5d4..7f97e42 100644 --- a/apps/complaints/test_workflow_realignment.py +++ b/apps/complaints/test_workflow_realignment.py @@ -70,3 +70,79 @@ class TestInquiryDepartmentResponsePermission(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 TestInquiryDetailUIRealignment(TestCase): + """The inquiry detail page no longer surfaces the PX-side 'Response to Patient' + action. Resolution happens via direct POST to inquiry_change_status. + + Locks in Task 5 of the workflow realignment: department owns resolution. + """ + + def setUp(self): + self.hospital = Hospital.objects.create(name="Test Hospital UI", code="TH03") + self.dept = Department.objects.create(name="Dept UI", hospital=self.hospital, code="UI") + + _ensure_group("PX Admin") + self.px_admin = User.objects.create_user( + username="pxui", email="pxui@test", password="x", hospital=self.hospital + ) + self.px_admin.groups.add(Group.objects.get(name="PX Admin")) + + def _make_inquiry(self, **kwargs): + defaults = dict( + hospital=self.hospital, + department=self.dept, + subject="UI test", + message="msg", + status="in_progress", + activated_at=kwargs.pop("activated_at", __import__("django.utils.timezone", fromlist=["now"]).now()), + ) + defaults.update(kwargs) + return Inquiry.objects.create(**defaults) + + def test_response_to_patient_button_is_gone(self): + """The 'Response to Patient' tile button must not appear anywhere on the page.""" + inquiry = self._make_inquiry() + self.client.force_login(self.px_admin) + # PX admins are redirected to select-hospital unless the session has a + # selected_hospital_id (HospitalSelectionMiddleware). Set it directly. + session = self.client.session + session["selected_hospital_id"] = str(self.hospital.id) + session.save() + response = self.client.get(reverse("inquiries:inquiry_detail", kwargs={"pk": inquiry.pk})) + assert response.status_code == 200 + # The PX-side respond modal and its trigger are removed. The button label + # may still appear inside {% comment %} blocks (inert), but the live + # onclick="showRespondModal()" button in the actions tile is gone. + content = response.content.decode() + # showRespondModal JS function definition must be gone. + assert "function showRespondModal" not in content, \ + "showRespondModal JS function should be removed" + # The respondModal div must be gone. + assert 'id="respondModal"' not in content, \ + "respondModal div should be removed" + + def test_resolve_uses_direct_status_change_form(self): + """The Resolve button POSTs directly to inquiry_change_status (not a modal).""" + from django.utils import timezone + # Department has responded → "Department has responded" banner shows Resolve. + inquiry = self._make_inquiry( + sent_to_department=True, + department_responded_at=timezone.now(), + ) + self.client.force_login(self.px_admin) + session = self.client.session + session["selected_hospital_id"] = str(self.hospital.id) + session.save() + response = self.client.get(reverse("inquiries:inquiry_detail", kwargs={"pk": inquiry.pk})) + assert response.status_code == 200 + content = response.content.decode() + change_status_url = reverse("inquiries:inquiry_change_status", kwargs={"pk": inquiry.pk}) + # The Resolve button must be a real form posting to inquiry_change_status + # with status=resolved — not a showRespondModal() trigger. + assert f'action="{change_status_url}"' in content, \ + "Resolve must POST to inquiry_change_status" + assert 'name="status" value="resolved"' in content, \ + "Resolve form must include hidden status=resolved input" diff --git a/templates/complaints/inquiry_detail.html b/templates/complaints/inquiry_detail.html index 3e1cba8..1021d18 100644 --- a/templates/complaints/inquiry_detail.html +++ b/templates/complaints/inquiry_detail.html @@ -213,7 +213,11 @@ {% endcomment %} {% if can_respond and inquiry.activated_at %} - +
+ {% csrf_token %} + +
{% endif %} {% endif %} @@ -623,10 +627,6 @@ {% trans "Send to Department" %} - {% endif %} {% endif %} {% endif %} @@ -702,84 +702,7 @@ {% endif %} - - + {% elif observation.department_responded_at and observation.status not in 'resolved,closed' %} -
-
+
+
-

{% trans "Department has responded" %}

{% trans "Review the response and resolve this observation." %}

+
+

{% trans "Department has responded" %}

+

{% trans "Decide what to do next, then resolve and inform the reporter." %}

+
{% if can_convert %} - +
+ {% if not observation.action_id %} + + + {% trans "Create Action" %} + {% trans "Minor / isolated" %} + + {% else %} +
+ + {% trans "Action exists" %} +
+ {% endif %} + + + + {% trans "Start RCA" %} + {% trans "Serious / pattern" %} + + + {% if can_admin %} + + + {% trans "QI Project" %} + {% trans "Systemic trend" %} + + {% endif %} + + + {% csrf_token %} + + + +
+ {% else %} + {# Non-admin viewers see only the simple resolve form #} +
{% csrf_token %} - +
{% endif %}
{% elif observation.status in 'resolved,closed' %} -
-
-

{% trans "This observation is resolved" %}

+
+
+
+
+

{% trans "This observation is resolved" %}

+ {% if observation.response_sent_at %} +

{% trans "Reporter informed on" %} {{ observation.response_sent_at|date:"M d, Y H:i" }}

+ {% else %} +

{% trans "Reporter has not yet been informed." %}

+ {% endif %} +
+
+ {% if can_convert and not observation.response_sent_at and observation.status == 'resolved' %} + + {% endif %}
{% endif %} From b1776494f9926f08dd4257c475d024969d3f923c Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 22:13:22 +0300 Subject: [PATCH 09/13] feat(suggestion): optional implemented-notice SMS on close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When PX closes a suggestion that was actually implemented, ticking the checkbox sends a high-value 'Good news — your suggestion was implemented' SMS instead of the generic closed message. The flag is ignored for any status other than CLOSED. Acknowledge-on-submit SMS already existed; this completes the closed-loop value path. The checkbox appears in the status-change form in feedback_detail.html. Three tests cover: implemented flag on close, flag off on close, flag ignored when transitioning to a non-closed status. --- apps/feedback/test_workflow_realignment.py | 91 ++++++++++++++++++++++ apps/feedback/views.py | 23 +++++- templates/feedback/feedback_detail.html | 4 + 3 files changed, 115 insertions(+), 3 deletions(-) create mode 100644 apps/feedback/test_workflow_realignment.py diff --git a/apps/feedback/test_workflow_realignment.py b/apps/feedback/test_workflow_realignment.py new file mode 100644 index 0000000..309a179 --- /dev/null +++ b/apps/feedback/test_workflow_realignment.py @@ -0,0 +1,91 @@ +"""Tests for the suggestion workflow realignment (optional implemented-notice SMS).""" +from unittest.mock import patch +from django.contrib.auth.models import Group + +import pytest +from django.test import TestCase +from django.urls import reverse + +from apps.accounts.models import User +from apps.organizations.models import Hospital +from apps.feedback.models import Feedback, FeedbackStatus, FeedbackType + + +def _ensure_group(name): + """Get or create a role group (pytest runs with --nomigrations).""" + grp, _ = Group.objects.get_or_create(name=name) + return grp + + +@pytest.mark.django_db +class TestSuggestionImplementedNotice(TestCase): + """Closing a suggestion with implemented_notice=True sends the high-value SMS. + + Uses self.client because feedback_change_status calls messages.success/error + (needs MessageMiddleware) and redirects (302) — no template rendering. + """ + + def setUp(self): + self.hospital = Hospital.objects.create(name="Test Hospital", code="TH09") + # Use PX Employee (not PX Admin) so HospitalSelectionMiddleware uses + # user.hospital directly instead of requiring session['selected_hospital_id']. + _ensure_group("PX Employee") + self.user = User.objects.create_user( + username="pxsug", email="pxsug@test", password="x", hospital=self.hospital + ) + self.user.groups.add(Group.objects.get(name="PX Employee")) + self.client.force_login(self.user) + + self.feedback = Feedback.objects.create( + hospital=self.hospital, + feedback_type=FeedbackType.SUGGESTION, + status=FeedbackStatus.REVIEWED, + contact_phone="+966500000000", + contact_name="Patient", + message="Add online check-in", + ) + + @patch("apps.feedback.views.NotificationService.send_sms") + def test_close_with_implemented_notice_sends_implemented_sms(self, mock_sms): + """When implemented_notice=on and status=closed, SMS body mentions 'implemented'.""" + response = self.client.post( + reverse("feedback:feedback_change_status", kwargs={"pk": self.feedback.pk}), + data={"status": "closed", "implemented_notice": "on"}, + ) + assert response.status_code == 302, f"expected redirect, got {response.status_code}" + self.feedback.refresh_from_db() + assert self.feedback.status == FeedbackStatus.CLOSED + assert mock_sms.called, "send_sms should have been called" + last_call_body = mock_sms.call_args[0][1] + assert "implemented" in last_call_body.lower(), \ + f"Expected 'implemented' in SMS body, got: {last_call_body}" + + @patch("apps.feedback.views.NotificationService.send_sms") + def test_close_without_implemented_notice_sends_generic_sms(self, mock_sms): + """Without the flag, the generic closed message fires (no 'implemented').""" + response = self.client.post( + reverse("feedback:feedback_change_status", kwargs={"pk": self.feedback.pk}), + data={"status": "closed"}, + ) + assert response.status_code == 302 + self.feedback.refresh_from_db() + assert self.feedback.status == FeedbackStatus.CLOSED + assert mock_sms.called + last_call_body = mock_sms.call_args[0][1] + assert "implemented" not in last_call_body.lower(), \ + f"Did not expect 'implemented' in generic SMS, got: {last_call_body}" + + @patch("apps.feedback.views.NotificationService.send_sms") + def test_implemented_notice_ignored_when_status_not_closed(self, mock_sms): + """The flag has no effect when transitioning to a non-closed status (e.g. acknowledged).""" + response = self.client.post( + reverse("feedback:feedback_change_status", kwargs={"pk": self.feedback.pk}), + data={"status": "acknowledged", "implemented_notice": "on"}, + ) + assert response.status_code == 302 + self.feedback.refresh_from_db() + assert self.feedback.status == FeedbackStatus.ACKNOWLEDGED + # SMS fires for acknowledged too (existing behavior), but must NOT say 'implemented'. + if mock_sms.called: + last_call_body = mock_sms.call_args[0][1] + assert "implemented" not in last_call_body.lower() diff --git a/apps/feedback/views.py b/apps/feedback/views.py index 62a7ed7..930e261 100644 --- a/apps/feedback/views.py +++ b/apps/feedback/views.py @@ -913,13 +913,30 @@ def feedback_change_status(request, pk): # Notify the suggester on acknowledge/close (closed-loop communication). # Only for suggestions with a patient-facing contact channel; internal / # source-user submissions have no patient to notify. + implemented_notice = ( + request.POST.get("implemented_notice") in ("on", "true", "True", "1") + and new_status == FeedbackStatus.CLOSED + ) if new_status in (FeedbackStatus.ACKNOWLEDGED, FeedbackStatus.CLOSED) and feedback.contact_phone: try: - label = "acknowledged" if new_status == FeedbackStatus.ACKNOWLEDGED else "closed" + if implemented_notice: + body = ( + f"Good news — your suggestion (ref {feedback.reference_number}) " + f"has been implemented. Thank you for helping us improve." + ) + elif new_status == FeedbackStatus.ACKNOWLEDGED: + body = ( + f"Your suggestion (ref {feedback.reference_number}) has been acknowledged. " + f"Thank you for helping us improve." + ) + else: # CLOSED without implemented flag + body = ( + f"Your suggestion (ref {feedback.reference_number}) has been closed. " + f"Thank you for helping us improve." + ) NotificationService.send_sms( feedback.contact_phone, - f"Your suggestion (ref {feedback.reference_number}) has been {label}. " - f"Thank you for helping us improve.", + body, related_object=feedback, ) except Exception: diff --git a/templates/feedback/feedback_detail.html b/templates/feedback/feedback_detail.html index f6dd2c5..173d5b1 100644 --- a/templates/feedback/feedback_detail.html +++ b/templates/feedback/feedback_detail.html @@ -352,6 +352,10 @@ {% endfor %} + From 295ff298a8b2ff1605ae9c9e1470561a870d9d29 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 22:20:29 +0300 Subject: [PATCH 10/13] =?UTF-8?q?refactor(appreciation):=20collapse=20to?= =?UTF-8?q?=203-state=20DRAFT=20=E2=86=92=20ACTIVATED=20=E2=86=92=20SENT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops the dead AI_ANALYZED status (a prior data migration already converted existing rows; the choice was never removed from the model) and the unreachable ACKNOWLEDGED status (is_recipient was hardcoded False so the acknowledge button never rendered). Changes: - AppreciationStatus: DRAFT, ACTIVATED, SENT only. SENT is terminal. - VALID_APPRECIATION_TRANSITIONS: DRAFT→ACTIVATED→SENT, SENT terminal. - Removed the model.acknowledge() method. - DRF viewset acknowledge action: returns 410 Gone (deprecated). - UI appreciation_acknowledge view: redirects with deprecation message. - Removed URL routes: appreciation_acknowledge, appreciation_send_dept_reminder. - appreciation_send_dept_reminder status check no longer references ACKNOWLEDGED. - appreciation_list stats + status filter + badge: dropped ai_analyzed/acknowledged. - appreciation_detail status badge + sent banner: dropped acknowledged references. - Migration 0010_drop_acknowledged_status: AlterField on status choices. AI analysis still runs on activation and is stored in ai_analysis JSON; it just no longer has its own status. Badges and leaderboard fire at SENT. 6 tests lock in the new lifecycle and removed URLs. --- .../0010_drop_acknowledged_status.py | 18 ++++++++ apps/appreciation/models.py | 28 +++++------- .../appreciation/test_workflow_realignment.py | 45 +++++++++++++++++++ apps/appreciation/ui_views.py | 35 ++++++--------- apps/appreciation/urls.py | 2 - apps/appreciation/views.py | 26 ++++------- .../appreciation/appreciation_detail.html | 6 +-- templates/appreciation/appreciation_list.html | 4 -- 8 files changed, 96 insertions(+), 68 deletions(-) create mode 100644 apps/appreciation/migrations/0010_drop_acknowledged_status.py create mode 100644 apps/appreciation/test_workflow_realignment.py diff --git a/apps/appreciation/migrations/0010_drop_acknowledged_status.py b/apps/appreciation/migrations/0010_drop_acknowledged_status.py new file mode 100644 index 0000000..a452966 --- /dev/null +++ b/apps/appreciation/migrations/0010_drop_acknowledged_status.py @@ -0,0 +1,18 @@ +# Generated by Django 6.0.1 on 2026-07-20 19:17 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('appreciation', '0009_add_national_id'), + ] + + operations = [ + migrations.AlterField( + model_name='appreciation', + name='status', + field=models.CharField(choices=[('draft', 'Draft'), ('activated', 'Activated'), ('sent', 'Sent')], db_index=True, default='draft', max_length=20), + ), + ] diff --git a/apps/appreciation/models.py b/apps/appreciation/models.py index 25aeed3..ce24dbc 100644 --- a/apps/appreciation/models.py +++ b/apps/appreciation/models.py @@ -17,21 +17,24 @@ from apps.core.models import SoftDeleteModel, TimeStampedModel, UUIDModel class AppreciationStatus(models.TextChoices): - """Appreciation status choices""" + """Appreciation status choices. + + Collapsed to a 3-state lifecycle: an appreciation is drafted, PX activates + it, and sending it to the department/manager is terminal. AI analysis still + runs on activation but is stored in ``ai_analysis`` JSON rather than + surfaced as a separate status. There is no acknowledge step — the recipient + is notified on send, and recognition (badges, leaderboard) fires at SENT. + """ DRAFT = "draft", "Draft" ACTIVATED = "activated", "Activated" - AI_ANALYZED = "ai_analyzed", "AI Analyzed" SENT = "sent", "Sent" - ACKNOWLEDGED = "acknowledged", "Acknowledged" VALID_APPRECIATION_TRANSITIONS = { AppreciationStatus.DRAFT: {AppreciationStatus.ACTIVATED}, - AppreciationStatus.ACTIVATED: {AppreciationStatus.AI_ANALYZED, AppreciationStatus.SENT}, - AppreciationStatus.AI_ANALYZED: {AppreciationStatus.SENT}, - AppreciationStatus.SENT: {AppreciationStatus.ACKNOWLEDGED}, - AppreciationStatus.ACKNOWLEDGED: set(), + AppreciationStatus.ACTIVATED: {AppreciationStatus.SENT}, + AppreciationStatus.SENT: set(), # terminal } @@ -350,17 +353,6 @@ class Appreciation(UUIDModel, TimeStampedModel, SoftDeleteModel): self.send_notification() - def acknowledge(self): - """Mark appreciation as acknowledged""" - from django.utils import timezone - - if self.status != AppreciationStatus.SENT: - raise ValueError(f"Cannot acknowledge appreciation in '{self.status}' status. Must be in 'sent'.") - - self.status = AppreciationStatus.ACKNOWLEDGED - self.acknowledged_at = timezone.now() - self.save(update_fields=["status", "acknowledged_at"]) - def send_notification(self): """Send notification to recipient — handled by post_save signal.""" pass diff --git a/apps/appreciation/test_workflow_realignment.py b/apps/appreciation/test_workflow_realignment.py new file mode 100644 index 0000000..584bb8a --- /dev/null +++ b/apps/appreciation/test_workflow_realignment.py @@ -0,0 +1,45 @@ +"""Tests for the appreciation 3-state lifecycle realignment.""" +import pytest +from django.test import TestCase +from django.urls import reverse, NoReverseMatch + +from apps.appreciation.models import AppreciationStatus, VALID_APPRECIATION_TRANSITIONS + + +class TestAppreciationThreeStateLifecycle(TestCase): + """Appreciation has exactly 3 statuses: DRAFT, ACTIVATED, SENT.""" + + def test_only_three_statuses_exist(self): + choices = [value for value, _ in AppreciationStatus.choices] + assert set(choices) == {"draft", "activated", "sent"}, \ + f"Expected exactly 3 statuses, got: {choices}" + + def test_no_ai_analyzed_or_acknowledged(self): + assert not hasattr(AppreciationStatus, "AI_ANALYZED"), \ + "AI_ANALYZED should be removed" + assert not hasattr(AppreciationStatus, "ACKNOWLEDGED"), \ + "ACKNOWLEDGED should be removed" + + def test_sent_is_terminal(self): + """SENT has no outgoing transitions.""" + assert VALID_APPRECIATION_TRANSITIONS.get(AppreciationStatus.SENT) == set(), \ + "SENT should be terminal" + + def test_activated_can_go_to_sent(self): + """ACTIVATED transitions directly to SENT (no AI_ANALYZED intermediate).""" + assert AppreciationStatus.SENT in \ + VALID_APPRECIATION_TRANSITIONS[AppreciationStatus.ACTIVATED] + + +class TestAppreciationRemovedUrls(TestCase): + """The acknowledge and send-dept-reminder URL routes are gone.""" + + def test_acknowledge_url_does_not_resolve(self): + with pytest.raises(NoReverseMatch): + reverse("appreciation:appreciation_acknowledge", + kwargs={"pk": "00000000-0000-0000-0000-000000000000"}) + + def test_send_dept_reminder_url_does_not_resolve(self): + with pytest.raises(NoReverseMatch): + reverse("appreciation:appreciation_send_dept_reminder", + kwargs={"pk": "00000000-0000-0000-0000-000000000000"}) diff --git a/apps/appreciation/ui_views.py b/apps/appreciation/ui_views.py index 863ba29..2ab3c2f 100644 --- a/apps/appreciation/ui_views.py +++ b/apps/appreciation/ui_views.py @@ -100,7 +100,7 @@ def appreciation_list(request): "activated": base_qs.filter( status=AppreciationStatus.ACTIVATED ).count(), - "sent": base_qs.filter(status__in=[AppreciationStatus.SENT, AppreciationStatus.ACKNOWLEDGED]).count(), + "sent": base_qs.filter(status=AppreciationStatus.SENT).count(), } paginator = Paginator(queryset, 25) @@ -598,27 +598,18 @@ def appreciation_send_to(request, pk): @login_required @require_http_methods(["POST"]) def appreciation_acknowledge(request, pk): - """Acknowledge appreciation""" - appreciation = get_object_or_404(Appreciation, pk=pk) + """DEPRECATED: ACKNOWLEDGED status was removed; SENT is terminal. - # Check if user is recipient - user_content_type = ContentType.objects.get_for_model(request.user) - if not ( - appreciation.recipient_content_type == user_content_type and - appreciation.recipient_object_id == request.user.id - ): - messages.error(request, "You can only acknowledge appreciations sent to you.") - return redirect('appreciation:appreciation_detail', pk=pk) - - if appreciation.status != AppreciationStatus.SENT: - messages.error(request, "This appreciation cannot be acknowledged in its current status.") - return redirect('appreciation:appreciation_detail', pk=pk) - - # Acknowledge - appreciation.acknowledge() - - messages.success(request, "Appreciation acknowledged successfully.") - return redirect('appreciation:appreciation_detail', pk=pk) + The URL route was removed too. This view function is kept only so that + any stale import or bookmarked URL that somehow resolves does not crash + the worker; it redirects with a clear message. Safe to delete once no + references remain. + """ + messages.error( + request, + _("Appreciation acknowledge is no longer supported — sending is the final step."), + ) + return redirect("appreciation:appreciation_detail", pk=pk) @login_required @@ -635,7 +626,7 @@ def appreciation_send_dept_reminder(request, pk): messages.error(request, _("You don't have permission to send reminders.")) return redirect("appreciation:appreciation_detail", pk=pk) - if appreciation.status not in (AppreciationStatus.SENT, AppreciationStatus.ACKNOWLEDGED): + if appreciation.status != AppreciationStatus.SENT: messages.warning(request, _("Appreciation must be sent before sending reminders.")) return redirect("appreciation:appreciation_detail", pk=pk) diff --git a/apps/appreciation/urls.py b/apps/appreciation/urls.py index 6ba6ea4..96a0069 100644 --- a/apps/appreciation/urls.py +++ b/apps/appreciation/urls.py @@ -37,8 +37,6 @@ urlpatterns = [ path('detail//select-recipient/', ui_views.appreciation_select_recipient, name='appreciation_select_recipient'), path('detail//send/', ui_views.appreciation_send, name='appreciation_send'), path('detail//send-to/', ui_views.appreciation_send_to, name='appreciation_send_to'), - path('detail//send-reminder/', ui_views.appreciation_send_dept_reminder, name='appreciation_send_dept_reminder'), - path('acknowledge//', ui_views.appreciation_acknowledge, name='appreciation_acknowledge'), path('leaderboard/', ui_views.leaderboard_view, name='leaderboard_view'), path('badges/', ui_views.my_badges_view, name='my_badges_view'), diff --git a/apps/appreciation/views.py b/apps/appreciation/views.py index a2624c2..952c1f4 100644 --- a/apps/appreciation/views.py +++ b/apps/appreciation/views.py @@ -211,25 +211,15 @@ class AppreciationViewSet(viewsets.ModelViewSet): @action(detail=True, methods=["post"]) def acknowledge(self, request, pk=None): - """Acknowledge an appreciation""" - appreciation = self.get_object() + """DEPRECATED: ACKNOWLEDGED status was removed; SENT is terminal. - # Check if user is the recipient - user_content_type = ContentType.objects.get_for_model(request.user) - if not ( - appreciation.recipient_content_type == user_content_type - and appreciation.recipient_object_id == request.user.id - ): - return Response( - {"error": "You can only acknowledge appreciations sent to you"}, status=status.HTTP_403_FORBIDDEN - ) - - # Acknowledge - appreciation.acknowledge() - - # Serialize and return - serializer = AppreciationSerializer(appreciation) - return Response(serializer.data) + Kept as a 410 Gone endpoint so old API clients get a clear error + instead of a 404. Will be removed in a future release. + """ + return Response( + {"error": "Appreciation acknowledge is no longer supported — SENT is terminal."}, + status=status.HTTP_410_GONE, + ) @action(detail=False, methods=["get"]) def my_appreciations(self, request): diff --git a/templates/appreciation/appreciation_detail.html b/templates/appreciation/appreciation_detail.html index 4439437..7907870 100644 --- a/templates/appreciation/appreciation_detail.html +++ b/templates/appreciation/appreciation_detail.html @@ -20,9 +20,7 @@ {{ appreciation.get_status_display }} @@ -130,8 +128,8 @@
-

{% trans "Appreciation acknowledged" %}

-

{% trans "Sent on" %} {{ appreciation.sent_at|date:"Y-m-d H:i" }} · {% trans "Acknowledged" %} {{ appreciation.acknowledged_at|date:"Y-m-d H:i" }}

+

{% trans "Appreciation sent" %}

+

{% trans "Sent on" %} {{ appreciation.sent_at|date:"Y-m-d H:i" }}

{% endif %} diff --git a/templates/appreciation/appreciation_list.html b/templates/appreciation/appreciation_list.html index 4056494..81a3e25 100644 --- a/templates/appreciation/appreciation_list.html +++ b/templates/appreciation/appreciation_list.html @@ -171,9 +171,7 @@ - -
{% endif %} + {% if dept_appreciations_count > 0 %} + +
+
+
+
+ +
+

{% trans "Appreciations Received" %}

+
+ {{ dept_appreciations_count }} +
+
    + {% for apr in dept_appreciations %} +
  • + + + {{ apr.message_en|default:apr.message_ar|truncatewords:18 }} + + {{ apr.sent_at|date:"M d, Y" }} + {% if apr.sender %}· {{ apr.sender.get_full_name }}{% endif %} + + +
    • + {% endfor %} +
+
+ {% endif %} + {% if can_respond and pending_routing_involvements %} {% for inv in pending_routing_involvements %} From c88c5cf97330ac60e5ff16b5d30ba26a7ece8db6 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 20 Jul 2026 22:26:05 +0300 Subject: [PATCH 12/13] ux(complaint): surface resolution_category and resolution_outcome in resolve modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These fields already existed on the Complaint model and are read by ComplaintService.change_status, but PX had to fill them via separate endpoints after resolving. Now they're captured in the same modal as the resolution text. Also fixed the view (complaint_change_status) to extract resolution_category from POST and pass it to the service — the service accepted the kwarg but the view never forwarded it. --- apps/complaints/ui_views.py | 2 ++ templates/complaints/complaint_detail.html | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/apps/complaints/ui_views.py b/apps/complaints/ui_views.py index c051faf..7c0a8ea 100644 --- a/apps/complaints/ui_views.py +++ b/apps/complaints/ui_views.py @@ -1589,6 +1589,7 @@ def complaint_change_status(request, pk): new_status = request.POST.get("status") note = request.POST.get("note", "") resolution = request.POST.get("resolution", "") + resolution_category = request.POST.get("resolution_category", "") resolution_outcome = request.POST.get("resolution_outcome", "") resolution_outcome_other = request.POST.get("resolution_outcome_other", "") @@ -1600,6 +1601,7 @@ def complaint_change_status(request, pk): request=request, note=note, resolution=resolution, + resolution_category=resolution_category, resolution_outcome=resolution_outcome, resolution_outcome_other=resolution_outcome_other, ) diff --git a/templates/complaints/complaint_detail.html b/templates/complaints/complaint_detail.html index 275d388..4345b0f 100644 --- a/templates/complaints/complaint_detail.html +++ b/templates/complaints/complaint_detail.html @@ -961,6 +961,28 @@
+
+
+ + +
+
+ + +
+