feat(signals): Signals list API changes for Code inbox#51740
Open
feat(signals): Signals list API changes for Code inbox#51740
Conversation
Contributor
|
Size Change: 0 B Total Size: 110 MB ℹ️ View Unchanged
|
Twixes
changed the title
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: products/signals/backend/serializers.py
Line: 147-152
Comment:
**`AttributeError` on valid-JSON non-dict content**
`data.get("priority")` is called *outside* the `try/except` block. If `art.content` is valid JSON but not a dict (e.g. `'null'`, `'[]'`, or `'"some string"'`) then `json.loads()` succeeds, the except is not triggered, and `data.get(...)` raises `AttributeError` — resulting in a 500 for the list/retrieve endpoint.
```suggestion
try:
data = json.loads(art.content)
except (json.JSONDecodeError, TypeError, ValueError):
return None
if not isinstance(data, dict):
return None
p = data.get("priority")
return p if isinstance(p, str) else None
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: products/signals/backend/test/test_signal_report_api.py
Line: 150-176
Comment:
**Prefer parameterised tests; missing non-dict JSON case**
Per the project convention, these three standalone "priority is null" tests (`invalid_json`, `non_string_priority`, and the no-artefact test above) are good candidates for a single `@parameterized.expand`. A parameterised approach also makes it easy to add the currently-untested case of *valid* JSON that isn't a dict (e.g. `content='null'`), which would expose the `AttributeError` described in the sibling comment.
Example sketch (artefact-content cases only):
```python
@parameterized.expand(
[
("invalid_json", "not-json{"),
("json_null", "null"),
("json_array", "[]"),
("non_string_priority", json.dumps({"priority": 2})),
("missing_priority_key", json.dumps({"choice": "immediately_actionable"})),
]
)
def test_list_priority_null_for_bad_content(self, _name, content):
report = self._create_report()
SignalReportArtefact.objects.create(
team=self.team,
report=report,
type=SignalReportArtefact.ArtefactType.ACTIONABILITY_JUDGMENT,
content=content,
)
response = self.client.get(self._list_url())
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["priority"] is None
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "test(signals): add A..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+150
to
+176
| def test_list_priority_null_when_artefact_json_invalid(self): | ||
| report = self._create_report() | ||
| SignalReportArtefact.objects.create( | ||
| team=self.team, | ||
| report=report, | ||
| type=SignalReportArtefact.ArtefactType.ACTIONABILITY_JUDGMENT, | ||
| content="not-json{", | ||
| ) | ||
|
|
||
| response = self.client.get(self._list_url()) | ||
| 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["priority"] is None | ||
|
|
||
| def test_list_priority_null_when_priority_not_a_string(self): | ||
| report = self._create_report() | ||
| SignalReportArtefact.objects.create( | ||
| team=self.team, | ||
| report=report, | ||
| type=SignalReportArtefact.ArtefactType.ACTIONABILITY_JUDGMENT, | ||
| content=json.dumps({"priority": 2}), | ||
| ) | ||
|
|
||
| response = self.client.get(self._list_url()) | ||
| 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["priority"] is None |
Contributor
There was a problem hiding this comment.
Per the project convention, these three standalone "priority is null" tests (invalid_json, non_string_priority, and the no-artefact test above) are good candidates for a single @parameterized.expand. A parameterised approach also makes it easy to add the currently-untested case of valid JSON that isn't a dict (e.g. content='null'), which would expose the AttributeError described in the sibling comment.
Example sketch (artefact-content cases only):
@parameterized.expand(
[
("invalid_json", "not-json{"),
("json_null", "null"),
("json_array", "[]"),
("non_string_priority", json.dumps({"priority": 2})),
("missing_priority_key", json.dumps({"choice": "immediately_actionable"})),
]
)
def test_list_priority_null_for_bad_content(self, _name, content):
report = self._create_report()
SignalReportArtefact.objects.create(
team=self.team,
report=report,
type=SignalReportArtefact.ArtefactType.ACTIONABILITY_JUDGMENT,
content=content,
)
response = self.client.get(self._list_url())
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["priority"] is NonePrompt To Fix With AI
This is a comment left during a code review.
Path: products/signals/backend/test/test_signal_report_api.py
Line: 150-176
Comment:
**Prefer parameterised tests; missing non-dict JSON case**
Per the project convention, these three standalone "priority is null" tests (`invalid_json`, `non_string_priority`, and the no-artefact test above) are good candidates for a single `@parameterized.expand`. A parameterised approach also makes it easy to add the currently-untested case of *valid* JSON that isn't a dict (e.g. `content='null'`), which would expose the `AttributeError` described in the sibling comment.
Example sketch (artefact-content cases only):
```python
@parameterized.expand(
[
("invalid_json", "not-json{"),
("json_null", "null"),
("json_array", "[]"),
("non_string_priority", json.dumps({"priority": 2})),
("missing_priority_key", json.dumps({"choice": "immediately_actionable"})),
]
)
def test_list_priority_null_for_bad_content(self, _name, content):
report = self._create_report()
SignalReportArtefact.objects.create(
team=self.team,
report=report,
type=SignalReportArtefact.ArtefactType.ACTIONABILITY_JUDGMENT,
content=content,
)
response = self.client.get(self._list_url())
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["priority"] is None
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Twixes
changed the title
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
We're currently saving priority (P0-P4) on the actionability judgement artifact, and PostHog Code needs it on signal reports in list responses without fetching
/artefacts/per report. Code also needs to fetch the reports not just by creation, but by their status in pipeline.Changes
SignalReportSerializergets read-onlypriorityfield from the latestACTIONABILITY_JUDGMENTartefact + gets more robust ordering, now bystatus.To avoid N+1 we're doing artefact prefetching in the list endpoint.
Also, decreasing the buffer wait time from 60s to 5s, as discussed with Olly on Slack.
How did you test this code?
Some new API tests.
Publish to changelog?
do not publish to changelog