diff --git a/django_forms_workflows/tasks.py b/django_forms_workflows/tasks.py index 5e07390..ce740aa 100644 --- a/django_forms_workflows/tasks.py +++ b/django_forms_workflows/tasks.py @@ -1237,6 +1237,7 @@ def _effective_stage_id(): # resolution for OTHER stages on the workflow. if getattr(notif, "notify_stage_groups", False) and submission is not None: from django.contrib.auth import get_user_model + from django.db.models import Exists, OuterRef from .models import StageApprovalGroup @@ -1254,36 +1255,36 @@ def _effective_stage_id(): .distinct() ) - # Step 2 — narrow to stages whose task took the group-fallback - # path on this submission. - effective_stage_ids = list( + # Step 2 — notify only the groups that actually hold a group-fallback + # task on those stages. A "sequence" stage must not page groups whose + # turn hasn't come yet. Only role="approval" rows count: + # validation/reassignment-role groups must not spawn tasks (engine fix + # in 0.74.6) and must not receive notifications, even if a task created + # before that fix is still around. + group_ids = list( submission.approval_tasks.filter( + Exists( + StageApprovalGroup.objects.filter( + stage_id=OuterRef("workflow_stage_id"), + group_id=OuterRef("assigned_group_id"), + role="approval", + ) + ), workflow_stage_id__in=candidate_stage_ids, assigned_group_id__isnull=False, assigned_to_id__isnull=True, ) - .values_list("workflow_stage_id", flat=True) + .values_list("assigned_group_id", flat=True) .distinct() ) - - if effective_stage_ids: - # Step 3 — only role="approval" rows on those stages count. - # Validation/reassignment-role groups must not spawn tasks - # (engine fix in 0.74.6) and must not receive notifications - # (parity with that fix). - group_ids = list( - StageApprovalGroup.objects.filter( - stage_id__in=effective_stage_ids, role="approval" - ).values_list("group_id", flat=True) - ) - if group_ids: - for email in ( - user_model.objects.filter(groups__id__in=group_ids) - .exclude(email="") - .values_list("email", flat=True) - .distinct() - ): - _add(email) + if group_ids: + for email in ( + user_model.objects.filter(groups__id__in=group_ids) + .exclude(email="") + .values_list("email", flat=True) + .distinct() + ): + _add(email) # 6. Explicit notify_groups M2M (NotificationRule only) if hasattr(notif, "notify_groups"): @@ -1453,14 +1454,6 @@ def send_notification_rules( if _rejecting: task_id = _rejecting.id - submission_url, approval_url = _build_form_field_notification_context( - submission, - ApprovalTask.objects.get(id=task_id) if task_id else None, - ) - - workflow = getattr(submission.form_definition, "workflow", None) - hide_approval_history = bool(getattr(workflow, "hide_approval_history", False)) - # Resolve the task (if any) to scope rules to its workflow task = None if task_id: @@ -1470,6 +1463,14 @@ def send_notification_rules( # A concurrently deleted task simply leaves notification context unscoped. pass + submission_url, approval_url = _build_form_field_notification_context( + submission, + task, + ) + + workflow = getattr(submission.form_definition, "workflow", None) + hide_approval_history = bool(getattr(workflow, "hide_approval_history", False)) + # Build a denormalized "public_comments" list for the template context # (file templates and per-rule body_template overrides alike). Each # entry has stage_name / status / actor / comment so a custom body can @@ -1695,12 +1696,8 @@ def _apply_rule(rule) -> None: # {{ c.comment }}{% endfor %}`` without iterating the model. "public_comments": public_comments, } - if task_id: - try: - base_context["task"] = ApprovalTask.objects.get(id=task_id) - except ApprovalTask.DoesNotExist: - # The notification can still be rendered without optional task context. - pass + if task: + base_context["task"] = task # Pre-fetch User objects for all recipients so templates that # reference ``{{ approver }}`` (e.g. approval_request.html) work diff --git a/poetry.lock b/poetry.lock index 66656e0..05c26d3 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2636,15 +2636,15 @@ files = [ [[package]] name = "pyasn1" -version = "0.6.4" +version = "0.6.5" description = "Pure-Python implementation of ASN.1 types and DER/BER/CER codecs (X.208)" optional = true python-versions = ">=3.8" groups = ["main"] markers = "extra == \"ldap\" or extra == \"all\" or extra == \"gmail\"" files = [ - {file = "pyasn1-0.6.4-py3-none-any.whl", hash = "sha256:deda9277cfd454080ec40b207fb6df82206a3a2688735233cdcd8d3d565f088b"}, - {file = "pyasn1-0.6.4.tar.gz", hash = "sha256:9c447d8431c947fe4c8febc4ed9e760bc29011a5b01e5c74b67025bd9fb8ce81"}, + {file = "pyasn1-0.6.5-py3-none-any.whl", hash = "sha256:4017b0fce125096b8573b39c989fdb1a4b19cf761e683398184550fea0e66587"}, + {file = "pyasn1-0.6.5.tar.gz", hash = "sha256:6b11616a6bcf3a40e635f082bb232fa57b5e0dd7f517bd7ddd69ec3794508a47"}, ] [[package]] diff --git a/tests/test_notifications.py b/tests/test_notifications.py index bf5c1da..aa6463b 100644 --- a/tests/test_notifications.py +++ b/tests/test_notifications.py @@ -2299,3 +2299,110 @@ def test_deadline_scan_has_constant_query_count( result = check_approval_deadlines() assert result == "expired=0, reminders=0, auto_approved=0" assert ApprovalTask.objects.filter(status="pending").count() == task_count + 1 + + +# ── notify_stage_groups: only groups holding a task on the stage ─────────── + + +@pytest.mark.django_db +class TestNotifyStageGroupsOnlyTaskHolders: + @pytest.fixture + def setup(self, form_definition, user, approval_group, second_approval_group): + from django.contrib.auth.models import User + + wf = WorkflowDefinition.objects.create( + form_definition=form_definition, requires_approval=True + ) + stage = WorkflowStage.objects.create( + workflow=wf, name="Sequence", order=1, approval_logic="sequence" + ) + StageApprovalGroup.objects.create( + stage=stage, group=approval_group, position=0, role="approval" + ) + NotificationRule.objects.create( + workflow=wf, + event="approval_request", + use_triggering_stage=True, + notify_stage_groups=True, + ) + User.objects.create_user("first", email="first@example.com").groups.add( + approval_group + ) + User.objects.create_user("second", email="second@example.com").groups.add( + second_approval_group + ) + submission = FormSubmission.objects.create( + form_definition=form_definition, + submitter=user, + form_data={}, + status="pending_approval", + ) + task = ApprovalTask.objects.create( + submission=submission, + step_name="Sequence", + status="pending", + workflow_stage=stage, + stage_number=1, + assigned_group=approval_group, + ) + return stage, submission, task, second_approval_group + + @staticmethod + def _recipients(submission, task): + with patch("django_forms_workflows.tasks._send_html_email") as mock_send: + send_notification_rules(submission.id, "approval_request", task_id=task.id) + return [call.args[1][0] for call in mock_send.call_args_list] + + def test_sequence_stage_skips_groups_not_yet_reached(self, setup): + stage, submission, task, later_group = setup + StageApprovalGroup.objects.create( + stage=stage, group=later_group, position=1, role="approval" + ) + + recipients = self._recipients(submission, task) + + assert "first@example.com" in recipients + assert "second@example.com" not in recipients + + def test_non_approval_role_task_does_not_notify_its_group(self, setup): + stage, submission, task, validation_group = setup + StageApprovalGroup.objects.create( + stage=stage, group=validation_group, role="validation" + ) + # A leftover task from before the 0.74.6 engine fix. + ApprovalTask.objects.create( + submission=submission, + step_name="Sequence", + status="pending", + workflow_stage=stage, + stage_number=1, + assigned_group=validation_group, + ) + + recipients = self._recipients(submission, task) + + assert "first@example.com" in recipients + assert "second@example.com" not in recipients + + +@pytest.mark.django_db +def test_send_notification_rules_tolerates_deleted_task(form_definition, user): + """A task deleted before the Celery job runs must not abort the dispatch.""" + wf = WorkflowDefinition.objects.create(form_definition=form_definition) + NotificationRule.objects.create( + workflow=wf, + event="approval_request", + notify_submitter=True, + ) + submission = FormSubmission.objects.create( + form_definition=form_definition, + submitter=user, + form_data={}, + status="pending_approval", + ) + + with patch("django_forms_workflows.tasks._send_html_email") as mock_send: + send_notification_rules(submission.id, "approval_request", task_id=999999) + + assert mock_send.called + assert "task" not in mock_send.call_args.args[3]