From 8a13c6e0772711de61ebca9c2f42d4633538ec1d Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Wed, 6 May 2026 08:12:50 +0000 Subject: [PATCH 1/9] feat(signals): record dismissal feedback as a report artefact Extend POST /signals/reports/:id/state/ to optionally accept `dismissal_reason` and `dismissal_note` when transitioning a report to the suppressed state. When provided, persist a new `dismissal`-type SignalReportArtefact alongside the status change so the rationale survives status changes and multiple dismissals can stack over time. Reasons surfaced by the PostHog Code UI: already_fixed, analysis_wrong, wontfix_intentional, wontfix_irrelevant, wrong_reviewer, other. The note field is bounded at 4000 characters. Generated-By: PostHog Code Task-Id: d3d675ab-707a-4a35-939b-9a9ab0f5c5b5 --- .../0018_alter_signalreportartefact_type.py | 27 +++++ .../backend/migrations/max_migration.txt | 2 +- products/signals/backend/models.py | 1 + .../backend/test/test_signal_report_api.py | 105 ++++++++++++++++++ products/signals/backend/views.py | 70 +++++++++++- 5 files changed, 202 insertions(+), 3 deletions(-) create mode 100644 products/signals/backend/migrations/0018_alter_signalreportartefact_type.py diff --git a/products/signals/backend/migrations/0018_alter_signalreportartefact_type.py b/products/signals/backend/migrations/0018_alter_signalreportartefact_type.py new file mode 100644 index 000000000000..b80a7cc93792 --- /dev/null +++ b/products/signals/backend/migrations/0018_alter_signalreportartefact_type.py @@ -0,0 +1,27 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("signals", "0017_add_resolved_signal_report_status"), + ] + + operations = [ + migrations.AlterField( + model_name="signalreportartefact", + name="type", + field=models.CharField( + choices=[ + ("video_segment", "Video Segment"), + ("safety_judgment", "Safety Judgment"), + ("actionability_judgment", "Actionability Judgment"), + ("priority_judgment", "Priority Judgment"), + ("signal_finding", "Signal Finding"), + ("repo_selection", "Repo Selection"), + ("suggested_reviewers", "Suggested Reviewers"), + ("dismissal", "Dismissal"), + ], + max_length=100, + ), + ), + ] diff --git a/products/signals/backend/migrations/max_migration.txt b/products/signals/backend/migrations/max_migration.txt index 78d96ed501e1..f12095583eec 100644 --- a/products/signals/backend/migrations/max_migration.txt +++ b/products/signals/backend/migrations/max_migration.txt @@ -1 +1 @@ -0017_add_resolved_signal_report_status +0018_alter_signalreportartefact_type diff --git a/products/signals/backend/models.py b/products/signals/backend/models.py index b6951d4e45c0..a47d7ce45244 100644 --- a/products/signals/backend/models.py +++ b/products/signals/backend/models.py @@ -319,6 +319,7 @@ class ArtefactType(models.TextChoices): SIGNAL_FINDING = "signal_finding" REPO_SELECTION = "repo_selection" SUGGESTED_REVIEWERS = "suggested_reviewers" + DISMISSAL = "dismissal" team = models.ForeignKey("posthog.Team", on_delete=models.CASCADE) report = models.ForeignKey(SignalReport, on_delete=models.CASCADE, related_name="artefacts") diff --git a/products/signals/backend/test/test_signal_report_api.py b/products/signals/backend/test/test_signal_report_api.py index bf3eeee99a64..3e664727cbac 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -550,3 +550,108 @@ def test_actionability_null_for_legacy_choice_artefact(self): assert response.status_code == status.HTTP_200_OK row = next(r for r in response.json()["results"] if r["id"] == str(report.id)) assert row["actionability"] is None + + +class TestSignalReportSuppressionAPI(APIBaseTest): + def _state_url(self, report_id: str) -> str: + return f"/api/projects/{self.team.id}/signals/reports/{report_id}/state/" + + def _create_report(self, report_status=SignalReport.Status.READY) -> SignalReport: + return SignalReport.objects.create( + team=self.team, + status=report_status, + title="Test report", + summary="Test summary", + ) + + def test_suppress_without_dismissal_creates_no_artefact(self): + report = self._create_report() + response = self.client.post( + self._state_url(str(report.id)), + data=json.dumps({"state": "suppressed"}), + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK, response.json() + report.refresh_from_db() + assert report.status == SignalReport.Status.SUPPRESSED + assert ( + SignalReportArtefact.objects.filter( + report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL + ).count() + == 0 + ) + + def test_suppress_with_dismissal_reason_and_note_creates_artefact(self): + report = self._create_report() + response = self.client.post( + self._state_url(str(report.id)), + data=json.dumps( + { + "state": "suppressed", + "dismissal_reason": "wontfix_intentional", + "dismissal_note": "this is intentional behavior, see RFC-123", + } + ), + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK, response.json() + report.refresh_from_db() + assert report.status == SignalReport.Status.SUPPRESSED + + artefacts = list( + SignalReportArtefact.objects.filter( + report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL + ) + ) + assert len(artefacts) == 1 + content = json.loads(artefacts[0].content) + assert content["reason"] == "wontfix_intentional" + assert content["note"] == "this is intentional behavior, see RFC-123" + assert content["user_id"] == self.user.id + assert content["user_uuid"] == str(self.user.uuid) + + def test_suppress_with_only_dismissal_note_creates_artefact(self): + report = self._create_report() + response = self.client.post( + self._state_url(str(report.id)), + data=json.dumps({"state": "suppressed", "dismissal_note": "free-form note"}), + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK, response.json() + artefact = SignalReportArtefact.objects.get( + report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL + ) + content = json.loads(artefact.content) + assert content["reason"] is None + assert content["note"] == "free-form note" + + def test_suppress_with_invalid_dismissal_reason_400(self): + report = self._create_report() + response = self.client.post( + self._state_url(str(report.id)), + data=json.dumps({"state": "suppressed", "dismissal_reason": "definitely_not_a_real_reason"}), + content_type="application/json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + report.refresh_from_db() + assert report.status == SignalReport.Status.READY + + def test_dismissal_reason_rejected_when_not_suppressing(self): + report = self._create_report(report_status=SignalReport.Status.SUPPRESSED) + response = self.client.post( + self._state_url(str(report.id)), + data=json.dumps({"state": "potential", "dismissal_reason": "other"}), + content_type="application/json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_suppress_with_oversized_dismissal_note_400(self): + report = self._create_report() + response = self.client.post( + self._state_url(str(report.id)), + data=json.dumps( + {"state": "suppressed", "dismissal_reason": "other", "dismissal_note": "x" * 4001} + ), + content_type="application/json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index 2dbaeb877230..5811c315d026 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -799,13 +799,30 @@ def signals(self, request, pk=None, **kwargs): signals_list = fetch_signals_for_report_sync(self.team, str(report.id)) return Response({"report": report_data, "signals": signals_list}) + # Reasons accepted by the dismissal artefact; the UI maps each to a human-readable label. + DISMISSAL_REASONS = { + "already_fixed", + "analysis_wrong", + "wontfix_intentional", + "wontfix_irrelevant", + "wrong_reviewer", + "other", + } + DISMISSAL_NOTE_MAX_LENGTH = 4000 + @extend_schema(exclude=True) @action(detail=True, methods=["post"], url_path="state", required_scopes=["task:write"]) def state(self, request, pk=None, **kwargs): """ Transition a report to a new state. The model validates allowed transitions. - Body: { "state": "suppressed" | "potential", ...kwargs passed to transition_to } + Body: { + "state": "suppressed" | "potential", + # Optional dismissal feedback (only honored when state == "suppressed"): + "dismissal_reason": "already_fixed" | "analysis_wrong" | ..., + "dismissal_note": "free-form text", + ...other kwargs passed to transition_to + } """ report = cast(SignalReport, self.get_object()) @@ -816,7 +833,37 @@ def state(self, request, pk=None, **kwargs): status=status.HTTP_400_BAD_REQUEST, ) - transition_kwargs = {k: v for k, v in request.data.items() if k != "state"} + # Pull dismissal fields out before passing the rest to transition_to. + dismissal_reason = request.data.get("dismissal_reason") + dismissal_note = request.data.get("dismissal_note") + transition_kwargs = { + k: v for k, v in request.data.items() if k not in ("state", "dismissal_reason", "dismissal_note") + } + + if dismissal_reason is not None: + if target != "suppressed": + return Response( + {"error": "dismissal_reason is only valid when state is 'suppressed'."}, + status=status.HTTP_400_BAD_REQUEST, + ) + if dismissal_reason not in self.DISMISSAL_REASONS: + return Response( + {"error": f"dismissal_reason must be one of {sorted(self.DISMISSAL_REASONS)}."}, + status=status.HTTP_400_BAD_REQUEST, + ) + + if dismissal_note is not None: + if not isinstance(dismissal_note, str): + return Response( + {"error": "dismissal_note must be a string."}, + status=status.HTTP_400_BAD_REQUEST, + ) + if len(dismissal_note) > self.DISMISSAL_NOTE_MAX_LENGTH: + return Response( + {"error": f"dismissal_note must be at most {self.DISMISSAL_NOTE_MAX_LENGTH} characters."}, + status=status.HTTP_400_BAD_REQUEST, + ) + try: updated_fields = report.transition_to(SignalReport.Status(target), **transition_kwargs) except InvalidStatusTransition as e: @@ -834,6 +881,25 @@ def state(self, request, pk=None, **kwargs): report.save(update_fields=updated_fields) + # Persist the dismissal feedback as its own artefact so it survives status changes + # and so multiple dismissals (with different rationales) can stack over time. + if target == "suppressed" and (dismissal_reason is not None or dismissal_note): + user = request.user + artefact_content = { + "reason": dismissal_reason, + "note": dismissal_note or "", + "user_id": getattr(user, "id", None) if getattr(user, "is_authenticated", False) else None, + "user_uuid": str(user.uuid) + if getattr(user, "is_authenticated", False) and getattr(user, "uuid", None) + else None, + } + SignalReportArtefact.objects.create( + team=self.team, + report=report, + type=SignalReportArtefact.ArtefactType.DISMISSAL, + content=json.dumps(artefact_content), + ) + return Response(SignalReportSerializer(report, context=self.get_serializer_context()).data) @extend_schema(exclude=True) From 959f6bc86f19e74a8d8bf26e878fe8302e28f9d6 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Wed, 6 May 2026 08:30:17 +0000 Subject: [PATCH 2/9] fix(signals): satisfy ruff format and use READY report in dismissal test - Collapse test filter expressions onto single lines so ruff format no longer wants to reformat them. - The "dismissal_reason rejected when not suppressing" test was creating a SUPPRESSED report, which is filtered out of the default queryset before validation runs and so returned 404 rather than 400. Use a READY report and target state=potential to exercise the validation path the test was meant to cover. Generated-By: PostHog Code Task-Id: d3d675ab-707a-4a35-939b-9a9ab0f5c5b5 --- .../backend/test/test_signal_report_api.py | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/products/signals/backend/test/test_signal_report_api.py b/products/signals/backend/test/test_signal_report_api.py index 3e664727cbac..4bef608993f5 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -575,9 +575,7 @@ def test_suppress_without_dismissal_creates_no_artefact(self): report.refresh_from_db() assert report.status == SignalReport.Status.SUPPRESSED assert ( - SignalReportArtefact.objects.filter( - report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL - ).count() + SignalReportArtefact.objects.filter(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL).count() == 0 ) @@ -599,9 +597,7 @@ def test_suppress_with_dismissal_reason_and_note_creates_artefact(self): assert report.status == SignalReport.Status.SUPPRESSED artefacts = list( - SignalReportArtefact.objects.filter( - report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL - ) + SignalReportArtefact.objects.filter(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) ) assert len(artefacts) == 1 content = json.loads(artefacts[0].content) @@ -618,9 +614,7 @@ def test_suppress_with_only_dismissal_note_creates_artefact(self): content_type="application/json", ) assert response.status_code == status.HTTP_200_OK, response.json() - artefact = SignalReportArtefact.objects.get( - report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL - ) + artefact = SignalReportArtefact.objects.get(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) content = json.loads(artefact.content) assert content["reason"] is None assert content["note"] == "free-form note" @@ -637,7 +631,9 @@ def test_suppress_with_invalid_dismissal_reason_400(self): assert report.status == SignalReport.Status.READY def test_dismissal_reason_rejected_when_not_suppressing(self): - report = self._create_report(report_status=SignalReport.Status.SUPPRESSED) + # Suppressed reports are filtered out of the default queryset (404 before validation), + # so use a READY report and target state=potential to exercise the validation path. + report = self._create_report(report_status=SignalReport.Status.READY) response = self.client.post( self._state_url(str(report.id)), data=json.dumps({"state": "potential", "dismissal_reason": "other"}), @@ -649,9 +645,7 @@ def test_suppress_with_oversized_dismissal_note_400(self): report = self._create_report() response = self.client.post( self._state_url(str(report.id)), - data=json.dumps( - {"state": "suppressed", "dismissal_reason": "other", "dismissal_note": "x" * 4001} - ), + data=json.dumps({"state": "suppressed", "dismissal_reason": "other", "dismissal_note": "x" * 4001}), content_type="application/json", ) assert response.status_code == status.HTTP_400_BAD_REQUEST From 4c6b679a707fc842d90b13bc8504524d808361e0 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 7 May 2026 11:10:59 +0200 Subject: [PATCH 3/9] Simplify --- .../backend/test/test_signal_report_api.py | 32 +++++++++++------ products/signals/backend/views.py | 34 ++++++------------- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/products/signals/backend/test/test_signal_report_api.py b/products/signals/backend/test/test_signal_report_api.py index 4bef608993f5..e5d61f022e7d 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -619,27 +619,39 @@ def test_suppress_with_only_dismissal_note_creates_artefact(self): assert content["reason"] is None assert content["note"] == "free-form note" - def test_suppress_with_invalid_dismissal_reason_400(self): + def test_suppress_accepts_arbitrary_dismissal_reason(self): + # The caller (PostHog Code) owns the set of valid reason codes; the API persists whatever it gets. report = self._create_report() response = self.client.post( self._state_url(str(report.id)), - data=json.dumps({"state": "suppressed", "dismissal_reason": "definitely_not_a_real_reason"}), + data=json.dumps({"state": "suppressed", "dismissal_reason": "some_brand_new_code"}), content_type="application/json", ) - assert response.status_code == status.HTTP_400_BAD_REQUEST - report.refresh_from_db() - assert report.status == SignalReport.Status.READY + assert response.status_code == status.HTTP_200_OK, response.json() + artefact = SignalReportArtefact.objects.get(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) + content = json.loads(artefact.content) + assert content["reason"] == "some_brand_new_code" - def test_dismissal_reason_rejected_when_not_suppressing(self): - # Suppressed reports are filtered out of the default queryset (404 before validation), - # so use a READY report and target state=potential to exercise the validation path. + def test_snooze_with_dismissal_reason_and_note_creates_artefact(self): report = self._create_report(report_status=SignalReport.Status.READY) response = self.client.post( self._state_url(str(report.id)), - data=json.dumps({"state": "potential", "dismissal_reason": "other"}), + data=json.dumps( + { + "state": "potential", + "dismissal_reason": "wontfix_irrelevant", + "dismissal_note": "snoozing for now", + } + ), content_type="application/json", ) - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_200_OK, response.json() + report.refresh_from_db() + assert report.status == SignalReport.Status.POTENTIAL + artefact = SignalReportArtefact.objects.get(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) + content = json.loads(artefact.content) + assert content["reason"] == "wontfix_irrelevant" + assert content["note"] == "snoozing for now" def test_suppress_with_oversized_dismissal_note_400(self): report = self._create_report() diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index 5811c315d026..c5cfa630d72e 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -799,15 +799,8 @@ def signals(self, request, pk=None, **kwargs): signals_list = fetch_signals_for_report_sync(self.team, str(report.id)) return Response({"report": report_data, "signals": signals_list}) - # Reasons accepted by the dismissal artefact; the UI maps each to a human-readable label. - DISMISSAL_REASONS = { - "already_fixed", - "analysis_wrong", - "wontfix_intentional", - "wontfix_irrelevant", - "wrong_reviewer", - "other", - } + # The set of allowed reason codes is owned by PostHog Code (the calling UI), + # not validated here -- we just persist whatever the client sends. DISMISSAL_NOTE_MAX_LENGTH = 4000 @extend_schema(exclude=True) @@ -818,8 +811,8 @@ def state(self, request, pk=None, **kwargs): Body: { "state": "suppressed" | "potential", - # Optional dismissal feedback (only honored when state == "suppressed"): - "dismissal_reason": "already_fixed" | "analysis_wrong" | ..., + # Optional dismissal feedback (honored when state == "suppressed" or "potential"): + "dismissal_reason": "", "dismissal_note": "free-form text", ...other kwargs passed to transition_to } @@ -840,17 +833,11 @@ def state(self, request, pk=None, **kwargs): k: v for k, v in request.data.items() if k not in ("state", "dismissal_reason", "dismissal_note") } - if dismissal_reason is not None: - if target != "suppressed": - return Response( - {"error": "dismissal_reason is only valid when state is 'suppressed'."}, - status=status.HTTP_400_BAD_REQUEST, - ) - if dismissal_reason not in self.DISMISSAL_REASONS: - return Response( - {"error": f"dismissal_reason must be one of {sorted(self.DISMISSAL_REASONS)}."}, - status=status.HTTP_400_BAD_REQUEST, - ) + if dismissal_reason is not None and not isinstance(dismissal_reason, str): + return Response( + {"error": "dismissal_reason must be a string."}, + status=status.HTTP_400_BAD_REQUEST, + ) if dismissal_note is not None: if not isinstance(dismissal_note, str): @@ -883,7 +870,8 @@ def state(self, request, pk=None, **kwargs): # Persist the dismissal feedback as its own artefact so it survives status changes # and so multiple dismissals (with different rationales) can stack over time. - if target == "suppressed" and (dismissal_reason is not None or dismissal_note): + # Captured for both suppress and snooze (transition to potential) flows. + if target in ("suppressed", "potential") and (dismissal_reason is not None or dismissal_note): user = request.user artefact_content = { "reason": dismissal_reason, From ecea90279e8c2c2a7afcd755e58a68a5cb0c796e Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 7 May 2026 13:09:23 +0200 Subject: [PATCH 4/9] Allow everyone to reingest --- frontend/src/scenes/inbox/InboxScene.tsx | 40 ++++++++++-------------- products/signals/ARCHITECTURE.md | 2 +- products/signals/backend/views.py | 8 +---- 3 files changed, 19 insertions(+), 31 deletions(-) diff --git a/frontend/src/scenes/inbox/InboxScene.tsx b/frontend/src/scenes/inbox/InboxScene.tsx index 6e5e54752d6a..0d6e0b50a038 100644 --- a/frontend/src/scenes/inbox/InboxScene.tsx +++ b/frontend/src/scenes/inbox/InboxScene.tsx @@ -48,7 +48,6 @@ import { PersonDisplay } from 'scenes/persons/PersonDisplay' import { preflightLogic } from 'scenes/PreflightCheck/preflightLogic' import { SceneExport } from 'scenes/sceneTypes' import { urls } from 'scenes/urls' -import { userLogic } from 'scenes/userLogic' import { SceneContent } from '~/layout/scenes/components/SceneContent' import { SceneTitleSection } from '~/layout/scenes/components/SceneTitleSection' @@ -531,7 +530,6 @@ function ReportDetailPane(): JSX.Element { selectedReportReviewers, } = useValues(inboxSceneLogic) const { deleteReport, reingestReport, setActiveDetailTab } = useActions(inboxSceneLogic) - const { user } = useValues(userLogic) const { hasNoSources } = useValues(signalSourcesLogic) const { openSourcesModal } = useActions(signalSourcesLogic) @@ -630,27 +628,23 @@ function ReportDetailPane(): JSX.Element { overlay={ - LemonDialog.open({ - title: `Re-ingest signals from "${selectedReport.title}"?`, - className: 'max-w-120', - description: - 'This will delete the report, then re-run all its signals through the grouping pipeline. Signals may end up in different reports.', - primaryButton: { - children: 'Re-ingest signals', - onClick: () => reingestReport(selectedReport.id), - }, - secondaryButton: { - children: 'Cancel', - }, - }), - }, - ] - : []), + { + label: 'Re-ingest signals', + onClick: () => + LemonDialog.open({ + title: `Re-ingest signals from "${selectedReport.title}"?`, + className: 'max-w-120', + description: + 'This will delete the report, then re-run all its signals through the grouping pipeline. Signals may end up in different reports.', + primaryButton: { + children: 'Re-ingest signals', + onClick: () => reingestReport(selectedReport.id), + }, + secondaryButton: { + children: 'Cancel', + }, + }), + }, { label: 'Delete report & signals', status: 'danger', diff --git a/products/signals/ARCHITECTURE.md b/products/signals/ARCHITECTURE.md index ae916c4fd810..76cf87d9bdf7 100644 --- a/products/signals/ARCHITECTURE.md +++ b/products/signals/ARCHITECTURE.md @@ -603,7 +603,7 @@ Read + delete + state transitions. Uses `IsAuthenticated` + `APIScopePermission` | GET | `signals/reports/{id}/` | Retrieve a single report | | DELETE | `signals/reports/{id}/` | Soft-delete a report and its signals. Starts `SignalReportDeletionWorkflow`. On success returns `202`. If the workflow is already running, returns `200 {"status": "already_running"}`. The API immediately transitions the Postgres report to `deleted` to hide it from list results while ClickHouse cleanup runs asynchronously. | | POST | `signals/reports/{id}/state/` | Transition report state. Body: `{ "state": "suppressed" \| "potential", ...transition_to kwargs }`. Only `suppressed` and `potential` are exposed via API. Returns `409` on invalid transitions and `400` on invalid arguments. | -| POST | `signals/reports/{id}/reingest/` | **Staff-only.** Delete a report and re-ingest its signals. Starts `SignalReportReingestionWorkflow`. On success returns `202`. If already running, returns `200 {"status": "already_running"}`. Returns `403` for non-staff users. | +| POST | `signals/reports/{id}/reingest/` | Delete a report and re-ingest its signals. Starts `SignalReportReingestionWorkflow`. On success returns `202`. If already running, returns `200 {"status": "already_running"}`. Same team access as other report endpoints; personal API keys need `task:write`. | | GET | `signals/reports/{id}/artefacts/` | List **all** artefacts for a report, ordered by `-created_at` | | GET | `signals/reports/{id}/signals/` | Fetch all signals for a report from ClickHouse, including full metadata | | GET | `signals/reports/available_reviewers/` | List available suggested reviewers for the team | diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index c5cfa630d72e..a920c2d83124 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -893,13 +893,7 @@ def state(self, request, pk=None, **kwargs): @extend_schema(exclude=True) @action(detail=True, methods=["post"], url_path="reingest", required_scopes=["task:write"]) def reingest(self, request, pk=None, **kwargs): - """Re-ingest a report's signals. Staff-only.""" - if not request.user.is_staff: - return Response( - {"error": "Only staff users can reingest reports."}, - status=status.HTTP_403_FORBIDDEN, - ) - + """Re-ingest a report's signals (same team access as other report actions).""" report = cast(SignalReport, self.get_object()) report_id = str(report.id) team_id = self.team.id From af64756df8b4ef6d01b214e3343f41db666fdcd7 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 7 May 2026 12:39:27 +0100 Subject: [PATCH 5/9] Update products/signals/backend/views.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --- products/signals/backend/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index a920c2d83124..e0b6804195fb 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -875,7 +875,7 @@ def state(self, request, pk=None, **kwargs): user = request.user artefact_content = { "reason": dismissal_reason, - "note": dismissal_note or "", + "note": dismissal_note, "user_id": getattr(user, "id", None) if getattr(user, "is_authenticated", False) else None, "user_uuid": str(user.uuid) if getattr(user, "is_authenticated", False) and getattr(user, "uuid", None) From a350b6f1b70f17c82f507d229275f1f06ecc0921 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 7 May 2026 12:40:41 +0100 Subject: [PATCH 6/9] Update products/signals/backend/views.py Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> --- products/signals/backend/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index e0b6804195fb..42410328d2e0 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -871,7 +871,8 @@ def state(self, request, pk=None, **kwargs): # Persist the dismissal feedback as its own artefact so it survives status changes # and so multiple dismissals (with different rationales) can stack over time. # Captured for both suppress and snooze (transition to potential) flows. - if target in ("suppressed", "potential") and (dismissal_reason is not None or dismissal_note): + if target in ("suppressed", "potential") and (dismissal_reason or dismissal_note): + user = request.user artefact_content = { "reason": dismissal_reason, From 5426508ffbfd4907876f2d65d63c8b1f8f0cc890 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 7 May 2026 15:14:48 +0200 Subject: [PATCH 7/9] Parametrize test --- .../backend/test/test_signal_report_api.py | 142 +++++++++--------- 1 file changed, 67 insertions(+), 75 deletions(-) diff --git a/products/signals/backend/test/test_signal_report_api.py b/products/signals/backend/test/test_signal_report_api.py index e5d61f022e7d..77cbae7fb46b 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -564,100 +564,92 @@ def _create_report(self, report_status=SignalReport.Status.READY) -> SignalRepor summary="Test summary", ) - def test_suppress_without_dismissal_creates_no_artefact(self): - report = self._create_report() - response = self.client.post( - self._state_url(str(report.id)), - data=json.dumps({"state": "suppressed"}), - content_type="application/json", - ) - assert response.status_code == status.HTTP_200_OK, response.json() - report.refresh_from_db() - assert report.status == SignalReport.Status.SUPPRESSED - assert ( - SignalReportArtefact.objects.filter(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL).count() - == 0 - ) - - def test_suppress_with_dismissal_reason_and_note_creates_artefact(self): - report = self._create_report() - response = self.client.post( - self._state_url(str(report.id)), - data=json.dumps( + @parameterized.expand( + [ + # name, body, expected_final_status, expected_reason, expected_note (None = no artefact) + ( + "suppress_without_dismissal_creates_no_artefact", + {"state": "suppressed"}, + SignalReport.Status.SUPPRESSED, + None, + None, + ), + ( + "suppress_with_reason_and_note", { "state": "suppressed", "dismissal_reason": "wontfix_intentional", "dismissal_note": "this is intentional behavior, see RFC-123", - } + }, + SignalReport.Status.SUPPRESSED, + "wontfix_intentional", + "this is intentional behavior, see RFC-123", ), - content_type="application/json", + ( + "suppress_with_only_note", + {"state": "suppressed", "dismissal_note": "free-form note"}, + SignalReport.Status.SUPPRESSED, + None, + "free-form note", + ), + # The caller (PostHog Code) owns the set of valid reason codes; the API persists whatever it gets. + ( + "suppress_accepts_arbitrary_reason", + {"state": "suppressed", "dismissal_reason": "some_brand_new_code"}, + SignalReport.Status.SUPPRESSED, + "some_brand_new_code", + "", + ), + ( + "snooze_with_reason_and_note", + { + "state": "potential", + "dismissal_reason": "wontfix_irrelevant", + "dismissal_note": "snoozing for now", + }, + SignalReport.Status.POTENTIAL, + "wontfix_irrelevant", + "snoozing for now", + ), + ] + ) + def test_state_transition_with_dismissal(self, _name, body, expected_final_status, expected_reason, expected_note): + report = self._create_report() + response = self.client.post( + self._state_url(str(report.id)), data=json.dumps(body), content_type="application/json" ) assert response.status_code == status.HTTP_200_OK, response.json() report.refresh_from_db() - assert report.status == SignalReport.Status.SUPPRESSED + assert report.status == expected_final_status artefacts = list( SignalReportArtefact.objects.filter(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) ) + if expected_reason is None and expected_note is None: + assert artefacts == [] + return + assert len(artefacts) == 1 content = json.loads(artefacts[0].content) - assert content["reason"] == "wontfix_intentional" - assert content["note"] == "this is intentional behavior, see RFC-123" + assert content["reason"] == expected_reason + assert content["note"] == expected_note assert content["user_id"] == self.user.id assert content["user_uuid"] == str(self.user.uuid) - def test_suppress_with_only_dismissal_note_creates_artefact(self): - report = self._create_report() - response = self.client.post( - self._state_url(str(report.id)), - data=json.dumps({"state": "suppressed", "dismissal_note": "free-form note"}), - content_type="application/json", - ) - assert response.status_code == status.HTTP_200_OK, response.json() - artefact = SignalReportArtefact.objects.get(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) - content = json.loads(artefact.content) - assert content["reason"] is None - assert content["note"] == "free-form note" - - def test_suppress_accepts_arbitrary_dismissal_reason(self): - # The caller (PostHog Code) owns the set of valid reason codes; the API persists whatever it gets. - report = self._create_report() - response = self.client.post( - self._state_url(str(report.id)), - data=json.dumps({"state": "suppressed", "dismissal_reason": "some_brand_new_code"}), - content_type="application/json", - ) - assert response.status_code == status.HTTP_200_OK, response.json() - artefact = SignalReportArtefact.objects.get(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) - content = json.loads(artefact.content) - assert content["reason"] == "some_brand_new_code" - - def test_snooze_with_dismissal_reason_and_note_creates_artefact(self): - report = self._create_report(report_status=SignalReport.Status.READY) - response = self.client.post( - self._state_url(str(report.id)), - data=json.dumps( - { - "state": "potential", - "dismissal_reason": "wontfix_irrelevant", - "dismissal_note": "snoozing for now", - } + @parameterized.expand( + [ + ( + "oversized_dismissal_note", + {"state": "suppressed", "dismissal_reason": "other", "dismissal_note": "x" * 4001}, ), - content_type="application/json", - ) - assert response.status_code == status.HTTP_200_OK, response.json() - report.refresh_from_db() - assert report.status == SignalReport.Status.POTENTIAL - artefact = SignalReportArtefact.objects.get(report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL) - content = json.loads(artefact.content) - assert content["reason"] == "wontfix_irrelevant" - assert content["note"] == "snoozing for now" - - def test_suppress_with_oversized_dismissal_note_400(self): + ] + ) + def test_state_transition_rejects_invalid_dismissal(self, _name, body): report = self._create_report() response = self.client.post( - self._state_url(str(report.id)), - data=json.dumps({"state": "suppressed", "dismissal_reason": "other", "dismissal_note": "x" * 4001}), - content_type="application/json", + self._state_url(str(report.id)), data=json.dumps(body), content_type="application/json" ) assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not SignalReportArtefact.objects.filter( + report=report, type=SignalReportArtefact.ArtefactType.DISMISSAL + ).exists() From d61dd13426efe76423de5a6a53e96f733d605b72 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 7 May 2026 15:36:29 +0200 Subject: [PATCH 8/9] ruff --- products/signals/backend/test/test_signal_report_api.py | 2 +- products/signals/backend/views.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/products/signals/backend/test/test_signal_report_api.py b/products/signals/backend/test/test_signal_report_api.py index 77cbae7fb46b..482d1073e5d1 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -598,7 +598,7 @@ def _create_report(self, report_status=SignalReport.Status.READY) -> SignalRepor {"state": "suppressed", "dismissal_reason": "some_brand_new_code"}, SignalReport.Status.SUPPRESSED, "some_brand_new_code", - "", + None, ), ( "snooze_with_reason_and_note", diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index 42410328d2e0..e21d3450b8dc 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -872,7 +872,6 @@ def state(self, request, pk=None, **kwargs): # and so multiple dismissals (with different rationales) can stack over time. # Captured for both suppress and snooze (transition to potential) flows. if target in ("suppressed", "potential") and (dismissal_reason or dismissal_note): - user = request.user artefact_content = { "reason": dismissal_reason, From 5a4bed20bd7db788511224a1e49001f79f61a346 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Sun, 10 May 2026 16:22:26 +0100 Subject: [PATCH 9/9] Add atomic() --- products/signals/backend/views.py | 45 ++++++++++++++++--------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index e21d3450b8dc..f26343f1b7a5 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -4,7 +4,7 @@ from typing import cast from django.conf import settings -from django.db import IntegrityError +from django.db import IntegrityError, transaction from django.db.models import ( BooleanField, Case, @@ -866,27 +866,28 @@ def state(self, request, pk=None, **kwargs): status=status.HTTP_400_BAD_REQUEST, ) - report.save(update_fields=updated_fields) - - # Persist the dismissal feedback as its own artefact so it survives status changes - # and so multiple dismissals (with different rationales) can stack over time. - # Captured for both suppress and snooze (transition to potential) flows. - if target in ("suppressed", "potential") and (dismissal_reason or dismissal_note): - user = request.user - artefact_content = { - "reason": dismissal_reason, - "note": dismissal_note, - "user_id": getattr(user, "id", None) if getattr(user, "is_authenticated", False) else None, - "user_uuid": str(user.uuid) - if getattr(user, "is_authenticated", False) and getattr(user, "uuid", None) - else None, - } - SignalReportArtefact.objects.create( - team=self.team, - report=report, - type=SignalReportArtefact.ArtefactType.DISMISSAL, - content=json.dumps(artefact_content), - ) + with transaction.atomic(): + report.save(update_fields=updated_fields) + + # Persist the dismissal feedback as its own artefact so it survives status changes + # and so multiple dismissals (with different rationales) can stack over time. + # Captured for both suppress and snooze (transition to potential) flows. + if target in ("suppressed", "potential") and (dismissal_reason or dismissal_note): + user = request.user + artefact_content = { + "reason": dismissal_reason, + "note": dismissal_note, + "user_id": getattr(user, "id", None) if getattr(user, "is_authenticated", False) else None, + "user_uuid": str(user.uuid) + if getattr(user, "is_authenticated", False) and getattr(user, "uuid", None) + else None, + } + SignalReportArtefact.objects.create( + team=self.team, + report=report, + type=SignalReportArtefact.ArtefactType.DISMISSAL, + content=json.dumps(artefact_content), + ) return Response(SignalReportSerializer(report, context=self.get_serializer_context()).data)