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/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..482d1073e5d1 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -550,3 +550,106 @@ 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", + ) + + @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", + ), + ( + "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", + None, + ), + ( + "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 == 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"] == expected_reason + assert content["note"] == expected_note + assert content["user_id"] == self.user.id + assert content["user_uuid"] == str(self.user.uuid) + + @parameterized.expand( + [ + ( + "oversized_dismissal_note", + {"state": "suppressed", "dismissal_reason": "other", "dismissal_note": "x" * 4001}, + ), + ] + ) + 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(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() diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index 2dbaeb877230..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, @@ -799,13 +799,23 @@ 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}) + # 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) @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 (honored when state == "suppressed" or "potential"): + "dismissal_reason": "", + "dismissal_note": "free-form text", + ...other kwargs passed to transition_to + } """ report = cast(SignalReport, self.get_object()) @@ -816,7 +826,31 @@ 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 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): + 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: @@ -832,20 +866,35 @@ def state(self, request, pk=None, **kwargs): status=status.HTTP_400_BAD_REQUEST, ) - report.save(update_fields=updated_fields) + 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) @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