Skip to content

Document attaching screenshots to pull requests, and checking new endpoints for per-row queries #1474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mihow merged 2 commits into from
Oct 6, 2026
Merged

Document attaching screenshots to pull requests, and checking new endpoints for per-row queries #1474

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37c3e34
docs: attach PR screenshots with gh --attach [skip ci]
mihow Oct 5, 2026
c9b1057
docs: add the per-row permission check to the endpoint checklist [ski…
mihow Oct 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .agents/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ Every PR body opens with:
1. **`## Summary`** — a short, plain-language paragraph stating the purpose of the change and its effect for the user, operator, or system. Written so the whole team can read it; implementation detail belongs in `## Detailed Description` below.
2. **`### List of Changes`** — a numbered list or a table. Each change has, at minimum, a plain user-effect description. Optionally add a column for the technical/implementation detail, plus any other helpful columns (affected area, risk, migration). Lead with the user-effect; do not reduce it to a bare list of class or method names.

### Screenshots: attach them with `gh --attach`

UI changes get screenshots in the PR body. GitHub CLI 2.99 and later uploads them directly, so there is no need to host images on a branch, a fork, or a bucket:

1. Write the body with a relative reference where each image belongs: `![The job filter, open](./03-job-filter.png)`.
2. From the directory holding the screenshots, run `gh pr edit <N> --body-file body.md --attach './03-job-filter.png'`, repeating `--attach` for each file. Each referenced file is uploaded and its reference is rewritten in place to a `github.com/user-attachments/assets/...` URL; files the body does not reference are appended at the end. The same flag works on `gh pr create`, `gh pr comment`, and the `gh issue` commands.
3. Check the result: no `](./` references should remain in the body, and each asset URL should load.

This repository is public. Before attaching, look at each screenshot for real project, station, or job names from production data, and prefer a test project; an uploaded asset stays reachable by its URL even after the reference is removed.

### Examples

Real titles from this repository, drafted mechanism-first and then rewritten to lead with the effect:
Expand Down Expand Up @@ -422,6 +432,7 @@ These map 1:1 to the most frequent review findings across this repo's history. R
- [ ] No PII in serializers: nested user serializers expose name/image only — never `email`.
- [ ] Every query param parsed via `SingleParamSerializer` (`ami/base/serializers.py`) or `url_boolean_param` (`ami/utils/fields.py`) so invalid input returns 400, not 500. Test the `?param=abc` case.
- [ ] Aggregation happens in SQL. Add an `assertNumQueries` test with a **multi-row** fixture — single-row fixtures cannot catch N+1 (example: `ami/ml/tests.py:1006`). Use strict `==` counts in assertions.
- [ ] Per-row permissions: `DefaultSerializer` resolves the user's object permissions for **every row** it serializes (a project lookup, Guardian `get_perms`, and custom job permissions), so a new list, nested, or dropdown serializer silently costs several queries per row. A serializer that does not need permissions overrides `get_permissions` to return an empty list (`ClassificationNestedSerializer`); one that does caches them once per request (`TaxaListSerializer.get_permissions`). Cachalot hides the repeats in manual testing, so pin it with a test, with cachalot disabled, that the query count is the same at two list sizes (example: `test_query_count_does_not_grow_with_the_number_of_jobs` in `ami/jobs/tests/test_jobs.py`, added in #1471).
- [ ] Reuse existing patterns before writing new ones — see `docs/claude/reference/canonical-patterns.md`.

### Any model change
Expand Down