Potential fixes for 2 code quality findings

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

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]