feat(signals): Record dismissal feedback as report artefact#57768
Merged
Conversation
Contributor
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
Contributor
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 0 Safe | 1 Needs Review | 0 Blocked
|
Twixes
changed the title
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/signals/backend/views.py:877
Inconsistent nullability between `note` and `reason` in the stored artefact. `reason` is stored as `None` when absent, but `note` is stored as `""` (empty string) when absent due to the `or ""` fallback. A downstream consumer must handle `null` for `reason` but would receive `""` for `note`, making the contract asymmetric. Using `None` for both keeps the JSON schema consistent.
```suggestion
"note": dismissal_note,
```
### Issue 2 of 2
products/signals/backend/test/test_signal_report_api.py:554-663
**Prefer parametrised tests** — the team's convention is to use parametrised tests where the same assertion logic repeats across multiple inputs. Several cases here (suppress-with-reason-only, suppress-with-note-only, snooze-with-both, oversized-note, etc.) share the same structure: build a request body, POST, assert status, inspect the artefact. Collapsing the happy-path variants into a single `@pytest.mark.parametrize` call and the error cases into another would reduce duplication without losing coverage.
Reviews (1): Last reviewed commit: "Simplify" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Twixes
force-pushed
the
posthog-code/dismissal-artefact-on-suppress
branch
from
cf3b5f2 to
05740d9
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Contributor
|
Size Change: -17.2 kB (-0.01%) Total Size: 116 MB 📦 View Changed
ℹ️ View Unchanged
|
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
- 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
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/signals/backend/views.py:869-889
**Non-atomic status change and artefact creation**
`report.save()` commits the state transition immediately. If `SignalReportArtefact.objects.create()` then fails (e.g., a DB error or an unhandled exception), the report status is persisted as `suppressed`/`potential` while the dismissal artefact is silently lost. Wrapping both writes in `transaction.atomic()` (and adding `from django.db import transaction` to the imports) keeps them as a single commit-or-rollback unit.
Reviews (2): Last reviewed commit: "fix(signals): expect null dismissal note" | Re-trigger Greptile |
Twixes
force-pushed
the
posthog-code/dismissal-artefact-on-suppress
branch
from
74e28ad to
d61dd13
Compare
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/signals/backend/views.py:874
The `target in ("suppressed", "potential")` check is always `True` at this point — the earlier guard at line 823 already rejects any other value with a 400. The redundant check makes it look like `target` might hold an unexpected value here, which is misleading and violates the "no superfluous parts" simplicity rule.
```suggestion
if dismissal_reason or dismissal_note:
```
Reviews (3): Last reviewed commit: "ruff" | Re-trigger Greptile |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
||
| """Re-ingest a report's signals (same team access as other report actions).""" | ||
| report = cast(SignalReport, self.get_object()) |
There was a problem hiding this comment.
Medium: Reingestion authorization bypass
Any authenticated project member, or a personal API key with task:write, can now POST to this endpoint and start a reingestion workflow for a team report. That workflow soft-deletes the report's existing signals and re-emits them, so this should stay behind a privileged server-side check rather than relying on normal report write access.
Dismissal feedback recorded as report artefactsThis PR adds a dismissal artefact type and records optional dismissal feedback during report state transitions. I checked the state endpoint, artefact serialization, team scoping, and the current reingest authorization lines referenced by prior review comments; I did not find any new security issues in the dismissal artefact handling. Status: 2 open |
|
|
||
| @extend_schema(exclude=True) | ||
| @action(detail=True, methods=["post"], url_path="reingest", required_scopes=["task:write"]) | ||
| def reingest(self, request, pk=None, **kwargs): |
There was a problem hiding this comment.
Medium: Reingestion authorization bypass
Removing the request.user.is_staff guard lets any project user with task:write start SignalReportReingestionWorkflow, which soft-deletes the report's signals and re-emits them through grouping. Keep this server-side staff/admin check, or replace it with an explicit maintenance/admin permission before exposing the frontend action.
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/signals/backend/views.py:875
Redundant guard — `target` can only be `"suppressed"` or `"potential"` at this point, because any other value was already rejected at line 823. The `target in (...)` predicate is always `True` here and adds a superfluous branch (simplicity rule 4).
```suggestion
if dismissal_reason or dismissal_note:
```
### Issue 2 of 2
products/signals/backend/test/test_signal_report_api.py:639-655
**Untested type-validation paths** — the view explicitly rejects non-string `dismissal_reason` and non-string `dismissal_note` with a `400`, but neither branch has a test case. Given the team's preference for parameterised tests, these cases fit naturally into the existing `test_state_transition_rejects_invalid_dismissal` parametrize block alongside `oversized_dismissal_note`: e.g. `{"state": "suppressed", "dismissal_reason": 123}` and `{"state": "suppressed", "dismissal_note": 456}`.
Reviews (4): Last reviewed commit: "Add atomic()" | Re-trigger Greptile |
Member
Author
fasyy612
pushed a commit
that referenced
this pull request
May 13, 2026
## Problem PostHog Code will surface a "Dismiss" dialog on the inbox where a reviewer can mark a report as not worth acting on, as of PostHog/code#2053. We need to record "why" alongside the status change so we can: - distinguish "already fixed" from "wontfix" - stack multiple dismissals over time when a report is unsnoozed and later re-dismissed for a different reason The existing `state` action only flips a status flag and offers no place to attach this metadata. ## Changes Adding a new `dismissal` value on `SignalReportArtefact.ArtefactType`. `POST /api/projects/:team_id/signals/reports/:id/state/` now accepts two optional body fields: - `dismissal_reason`- Arbitrary string code. The set of valid codes is owned by PostHog Code, so we can iterate there without a backend change. - `dismissal_note` - Free-form text, capped at 4000 characters. When either is provided and the target state is `suppressed` **or** `potential`, the action creates a `dismissal` artefact alongside the status change capturing reason, note, and the dismissing user's ID. Important note: Reingestion is now available to all users. We need it e.g. to let people re-run failed research, which we've gotten reports about in Discord. ### Why an artefact rather than a column on the report The existing `SUPPRESSED` status is a single flag. We need history (the same report can be dismissed multiple times for different reasons over its lifetime), and a typed artefact matches the existing pattern for everything else attached to a report. ## How did you test this code? `TestSignalReportSuppressionAPI` ## Publish to changelog? no
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PostHog Code will surface a "Dismiss" dialog on the inbox where a reviewer can mark a report as not worth acting on, as of PostHog/code#2053. We need to record "why" alongside the status change so we can:
The existing
stateaction only flips a status flag and offers no place to attach this metadata.Changes
Adding a new
dismissalvalue onSignalReportArtefact.ArtefactType.POST /api/projects/:team_id/signals/reports/:id/state/now accepts two optional body fields:dismissal_reason- Arbitrary string code. The set of valid codes is owned by PostHog Code, so we can iterate there without a backend change.dismissal_note- Free-form text, capped at 4000 characters.When either is provided and the target state is
suppressedorpotential, the action creates adismissalartefact alongside the status change capturing reason, note, and the dismissing user's ID.Important note: Reingestion is now available to all users. We need it e.g. to let people re-run failed research, which we've gotten reports about in Discord.
Why an artefact rather than a column on the report
The existing
SUPPRESSEDstatus is a single flag. We need history (the same report can be dismissed multiple times for different reasons over its lifetime), and a typed artefact matches the existing pattern for everything else attached to a report.How did you test this code?
TestSignalReportSuppressionAPIPublish to changelog?
no