feat(signals): Record dismissal feedback as report artefact#57768
Open
feat(signals): Record dismissal feedback as report artefact#57768
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}
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
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: +6.7 kB (+0.01%) Total Size: 112 MB 📦 View Changed
ℹ️ View Unchanged
|
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>
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
1 participant
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