diff --git a/CLAUDE.md b/CLAUDE.md index ed1ee3d..7752a9a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -195,3 +195,4 @@ Pytest settings are in `pyproject.toml` under `[tool.pytest.ini_options]` (`DJAN - **Disabling composite widgets**: a composite widget (e.g. `SearchSelect`) carries its `id` on a wrapper `
`, which has no `disabled` state — setting `.disabled` on it is a no-op. Disable the inner control (for `SearchSelect`, the `[data-search-select-search]` input); the wrapper fades itself via `DISABLED_WITHIN_CLASS`, so callers toggle only the control's `disabled`, never styles. - **Platform icons** are SVG snippets in `games/templates/icons/.html`. Add new ones there and reference them by slug in `Platform.icon`. - **Name compound types explicitly** — if a `tuple`, `dict`, or other compound value is passed between functions or appears in multiple signatures, give it a named type (`TypedDict`, `NamedTuple`, or a `type` alias) rather than repeating the structural annotation. This applies even to small types used in only a few places; the name carries intent that the structure cannot. Examples: `LabeledOption = tuple[str, str]` instead of repeating `tuple[str, str]` for (value, label) pairs; `RangeValues(min, max)` instead of `tuple[str, str]` for range bounds. +- **Name primitive roles too** — when a bare `str`/`int` stands for a domain concept (an id, a key, a token, a field name), give it a PEP 695 transparent alias (`type SortKey = str`) so signatures say *which* string/int goes where instead of a wall of `str`. These are zero-cost and need no wrapping (unlike `NewType`); reach for them especially when several distinct string roles meet in one function (e.g. a `dict[SortKey, SortSpec]` whose values reference an `AnnotationName`). Add a trailing comment on the alias noting an example value. Use `NewType` only when you actually want the checker to reject cross-assignment and are willing to wrap every literal. diff --git a/docs/superpowers/plans/2026-06-21-list-view-sort-param.md b/docs/superpowers/plans/2026-06-21-list-view-sort-param.md new file mode 100644 index 0000000..6820d1b --- /dev/null +++ b/docs/superpowers/plans/2026-06-21-list-view-sort-param.md @@ -0,0 +1,780 @@ +# List-view `sort` query param Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `list_games`, `list_sessions`, `list_purchases` honor a signed comma-list `?sort=` query param (e.g. `?sort=-playtime,name`), with every visible column sortable plus stats-parity aggregates (playtime, finish date). + +**Architecture:** A new `games/sorting.py` defines a per-model whitelist (`SortKey → SortSpec`), a pure parser (`parse_sort_terms`), and an applier (`apply_sort`) that annotates-then-orders and reports unknown keys. Each list view replaces its hardcoded `order_by(...)` with `apply_sort(...)`, eager-loads row relations, and turns unknown keys into warning toasts. Backend only — clickable-header UI is #73. + +**Tech Stack:** Django 6, Python 3.13, pytest + pytest-django. Components/views per `CLAUDE.md`. + +## Global Constraints + +- **Never write to `GeneratedField`s** (`duration_calculated`, `duration_total`, `price_per_game`, `days_to_finish`) — they are read/order-only. +- **Complete-word identifiers** (Python + JS): `descending` not `desc_flag`, `queryset` not `qs` in real code. +- **Cross-relation sorts use annotated aggregates** (`Sum`/`Max`/`Min`) — never bare `order_by("relation__field")` for to-many relations (avoids row duplication). To-one relations (`game__sort_name`, `device__name`) may be ordered directly. +- **Name primitive roles** with PEP 695 transparent aliases (`type SortKey = str`), per the new `CLAUDE.md` convention. +- **`sorting.py` stays HTTP-free** — it reports unknown keys; the view emits `messages.warning`. +- Tests run with `uv run --with pytest-django pytest`. +- Spec: `docs/superpowers/specs/2026-06-21-list-view-sort-param-design.md`. + +--- + +## File Structure + +- **Create** `games/sorting.py` — all sorting logic (aliases, `SortSpec`, `SortTerm`, `ParsedSort`, `SortResult`, the three `*_SORTS` maps + `*_DEFAULT_SORT`, `parse_sort_terms`, `apply_sort`, `parse_find_filter`). +- **Create** `tests/test_sorting.py` — unit + integration tests. +- **Modify** `games/views/game.py` — wire sort into `list_games` + `select_related`. +- **Modify** `games/views/session.py` — wire sort into `list_sessions` + `select_related`. +- **Modify** `games/views/purchase.py` — wire sort into `list_purchases` + `prefetch_related`. + +--- + +## Task 1: `sorting.py` core types + `parse_sort_terms` + +**Files:** +- Create: `games/sorting.py` +- Test: `tests/test_sorting.py` + +**Interfaces:** +- Consumes: nothing. +- Produces: + - `type SortKey = str`, `type SortString = str`, `type AnnotationName = str`, `type OrderField = str`, `type Annotations = dict[AnnotationName, Aggregate]` + - `SortSpec(expression: OrderField, annotate: Annotations | None = None)` (frozen dataclass) + - `SortTerm(NamedTuple)`: `key: SortKey`, `descending: bool` + - `type SortMap = dict[SortKey, SortSpec]` + - `ParsedSort(NamedTuple)`: `terms: list[SortTerm]`, `unknown: list[SortKey]` + - `parse_sort_terms(raw: SortString, sort_map: SortMap) -> ParsedSort` + +- [ ] **Step 1: Write the failing test** + +```python +# tests/test_sorting.py +"""Tests for the list-view sorting system (games/sorting.py).""" + +from games.sorting import SortSpec, SortTerm, parse_sort_terms + +# A minimal map; parse_sort_terms only checks key membership, not spec internals. +SAMPLE_MAP = {"name": SortSpec("name"), "date": SortSpec("created_at")} + + +class TestParseSortTerms: + def test_bare_key_is_ascending(self): + parsed = parse_sort_terms("name", SAMPLE_MAP) + assert parsed.terms == [SortTerm("name", False)] + assert parsed.unknown == [] + + def test_dash_prefix_is_descending(self): + parsed = parse_sort_terms("-date", SAMPLE_MAP) + assert parsed.terms == [SortTerm("date", True)] + + def test_multi_column_preserves_order(self): + parsed = parse_sort_terms("date,-name", SAMPLE_MAP) + assert parsed.terms == [SortTerm("date", False), SortTerm("name", True)] + + def test_unknown_key_is_reported_not_raised(self): + parsed = parse_sort_terms("bogus", SAMPLE_MAP) + assert parsed.terms == [] + assert parsed.unknown == ["bogus"] + + def test_mixed_valid_and_unknown(self): + parsed = parse_sort_terms("-name,bogus", SAMPLE_MAP) + assert parsed.terms == [SortTerm("name", True)] + assert parsed.unknown == ["bogus"] + + def test_whitespace_and_empty_tokens_ignored(self): + parsed = parse_sort_terms(" name , , -date ", SAMPLE_MAP) + assert parsed.terms == [SortTerm("name", False), SortTerm("date", True)] + + def test_empty_string_yields_nothing(self): + parsed = parse_sort_terms("", SAMPLE_MAP) + assert parsed.terms == [] + assert parsed.unknown == [] +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py -v` +Expected: FAIL — `ModuleNotFoundError: No module named 'games.sorting'`. + +- [ ] **Step 3: Write minimal implementation** + +```python +# games/sorting.py +"""Structured sorting for list views (Stash-inspired, paired with games/filters.py). + +A list view maps a public sort key to a SortSpec; the URL ?sort= param is a +signed comma-list of those keys (e.g. "-playtime,name"). See +docs/superpowers/specs/2026-06-21-list-view-sort-param-design.md. +""" + +from dataclasses import dataclass +from typing import NamedTuple + +from django.db.models import Aggregate + +type SortKey = str # public column key in a *_SORTS map and in a URL term ("playtime", "name") +type SortString = str # comma-list of signed SortKeys: the URL ?sort= value and *_DEFAULT_SORT ("-date,created") +type AnnotationName = str # an alias added via .annotate(), then referenced by SortSpec.expression +type OrderField = str # SortSpec.expression: a real model field path OR an AnnotationName + +# alias name -> the ORM aggregate that computes it, applied via queryset.annotate() +# e.g. {"total_playtime": Sum("sessions__duration_total")} +type Annotations = dict[AnnotationName, Aggregate] + + +@dataclass(frozen=True) +class SortSpec: + expression: OrderField # unsigned; a real column path or an AnnotationName + annotate: Annotations | None = None + + +class SortTerm(NamedTuple): + key: SortKey + descending: bool # True = "-key" (desc), False = bare key (asc) + + +type SortMap = dict[SortKey, SortSpec] + + +class ParsedSort(NamedTuple): + terms: list[SortTerm] + unknown: list[SortKey] # keys not in the map — the view turns these into warnings + + +def parse_sort_terms(raw: SortString, sort_map: SortMap) -> ParsedSort: + terms: list[SortTerm] = [] + unknown: list[SortKey] = [] + for token in raw.split(","): + token = token.strip() + if not token: + continue + descending = token.startswith("-") + key = token.lstrip("-") + if key in sort_map: + terms.append(SortTerm(key, descending)) + else: + unknown.append(key) + return ParsedSort(terms, unknown) +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py -v` +Expected: PASS (7 tests). + +- [ ] **Step 5: Commit** + +```bash +git add games/sorting.py tests/test_sorting.py +git commit -m "feat(sorting): SortSpec/SortTerm types + parse_sort_terms (#68)" +``` + +--- + +## Task 2: Per-model maps + `apply_sort` + `parse_find_filter` + +**Files:** +- Modify: `games/sorting.py` +- Test: `tests/test_sorting.py` + +**Interfaces:** +- Consumes: Task 1's `SortSpec`, `SortTerm`, `SortMap`, `parse_sort_terms`; `FindFilter` from `games.filters`. +- Produces: + - `GAME_SORTS: SortMap`, `GAME_DEFAULT_SORT = "-created"` + - `SESSION_SORTS: SortMap`, `SESSION_DEFAULT_SORT = "-date,created"` + - `PURCHASE_SORTS: SortMap`, `PURCHASE_DEFAULT_SORT = "-purchased,-created"` + - `SortResult(NamedTuple)`: `queryset: QuerySet`, `terms: list[SortTerm]`, `unknown: list[SortKey]` + - `apply_sort(queryset, find: FindFilter, sort_map: SortMap, default_sort: SortString) -> SortResult` + - `parse_find_filter(request: HttpRequest) -> FindFilter` + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_sorting.py`: + +```python +from datetime import datetime, timedelta +from zoneinfo import ZoneInfo + +import pytest +from django.conf import settings +from django.test import RequestFactory + +from games.filters import FindFilter +from games.models import Game, Platform, Purchase, Session +from games.sorting import ( + GAME_DEFAULT_SORT, + GAME_SORTS, + PURCHASE_DEFAULT_SORT, + PURCHASE_SORTS, + SESSION_DEFAULT_SORT, + SESSION_SORTS, + apply_sort, + parse_find_filter, +) + +ZONEINFO = ZoneInfo(settings.TIME_ZONE) + + +def _find(sort=None): + return FindFilter(sort=sort) + + +@pytest.fixture +def two_games(db): + platform = Platform.objects.create(name="P", icon="p") + alpha = Game.objects.create(name="Alpha", sort_name="Alpha", platform=platform) + beta = Game.objects.create(name="Beta", sort_name="Beta", platform=platform) + return alpha, beta + + +class TestApplySortGames: + def test_name_ascending(self, two_games): + alpha, beta = two_games + result = apply_sort(Game.objects.all(), _find("name"), GAME_SORTS, GAME_DEFAULT_SORT) + assert list(result.queryset) == [alpha, beta] + assert result.terms[0].key == "name" + assert result.unknown == [] + + def test_name_descending(self, two_games): + alpha, beta = two_games + result = apply_sort(Game.objects.all(), _find("-name"), GAME_SORTS, GAME_DEFAULT_SORT) + assert list(result.queryset) == [beta, alpha] + + def test_default_sort_when_absent_is_created_desc(self, two_games): + alpha, beta = two_games # beta created after alpha + result = apply_sort(Game.objects.all(), _find(None), GAME_SORTS, GAME_DEFAULT_SORT) + assert list(result.queryset) == [beta, alpha] + + def test_unknown_key_reported_and_falls_back(self, two_games): + result = apply_sort(Game.objects.all(), _find("bogus"), GAME_SORTS, GAME_DEFAULT_SORT) + assert result.unknown == ["bogus"] + assert result.queryset.count() == 2 # still returns rows (default order) + + def test_playtime_annotation_no_duplicate_rows(self, two_games): + alpha, _ = two_games + device = None + Session.objects.create( + game=alpha, + timestamp_start=datetime(2022, 1, 1, 10, tzinfo=ZONEINFO), + timestamp_end=datetime(2022, 1, 1, 12, tzinfo=ZONEINFO), + ) + Session.objects.create( + game=alpha, + timestamp_start=datetime(2022, 1, 2, 10, tzinfo=ZONEINFO), + timestamp_end=datetime(2022, 1, 2, 11, tzinfo=ZONEINFO), + ) + result = apply_sort(Game.objects.all(), _find("-playtime"), GAME_SORTS, GAME_DEFAULT_SORT) + # two sessions on alpha must not duplicate the alpha row + assert result.queryset.count() == 2 + assert list(result.queryset)[0] == alpha # most playtime first + + +class TestParseFindFilter: + def test_reads_sort_param(self): + request = RequestFactory().get("/x", {"sort": "-playtime,name"}) + assert parse_find_filter(request).sort == "-playtime,name" + + def test_absent_sort_is_none(self): + request = RequestFactory().get("/x") + assert parse_find_filter(request).sort is None + + +class TestSortMapShapes: + def test_default_sort_keys_exist_in_maps(self): + # every key referenced by a default sort string must be defined in its map + for default, sort_map in [ + (GAME_DEFAULT_SORT, GAME_SORTS), + (SESSION_DEFAULT_SORT, SESSION_SORTS), + (PURCHASE_DEFAULT_SORT, PURCHASE_SORTS), + ]: + for token in default.split(","): + assert token.lstrip("-") in sort_map +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py -v` +Expected: FAIL — `ImportError: cannot import name 'GAME_SORTS'` (and friends). + +- [ ] **Step 3: Write minimal implementation** + +Add imports at the top of `games/sorting.py` (merge with existing): + +```python +from django.db.models import Aggregate, Max, Min, QuerySet, Sum +from django.http import HttpRequest + +from games.filters import FindFilter +``` + +Append to `games/sorting.py`: + +```python +# ── Per-model sort maps ───────────────────────────────────────────────────── +# Cross-relation sorts use annotated aggregates (group by PK → no row dup). +# To-one relations (game__sort_name, device__name) are ordered directly. + +GAME_SORTS: SortMap = { + "name": SortSpec("name"), + "sort_name": SortSpec("sort_name"), + "year": SortSpec("year_released"), + "status": SortSpec("status"), + "wikidata": SortSpec("wikidata"), + "created": SortSpec("created_at"), + "playtime": SortSpec("total_playtime", {"total_playtime": Sum("sessions__duration_total")}), + "finished": SortSpec("last_finished", {"last_finished": Max("playevents__ended")}), +} +GAME_DEFAULT_SORT: SortString = "-created" + +SESSION_SORTS: SortMap = { + "name": SortSpec("game__sort_name"), + "date": SortSpec("timestamp_start"), + "duration": SortSpec("duration_total"), + "device": SortSpec("device__name"), + "created": SortSpec("created_at"), +} +SESSION_DEFAULT_SORT: SortString = "-date,created" + +PURCHASE_SORTS: SortMap = { + "name": SortSpec("first_game_name", {"first_game_name": Min("games__name")}), + "type": SortSpec("type"), + "price": SortSpec("converted_price"), + "infinite": SortSpec("infinite"), + "purchased": SortSpec("date_purchased"), + "refunded": SortSpec("date_refunded"), + "created": SortSpec("created_at"), + "finished": SortSpec("last_finished", {"last_finished": Max("games__playevents__ended")}), +} +PURCHASE_DEFAULT_SORT: SortString = "-purchased,-created" + + +# ── Apply ─────────────────────────────────────────────────────────────────── + + +class SortResult(NamedTuple): + queryset: QuerySet + terms: list[SortTerm] # the order actually applied — #73's header UI consumes this + unknown: list[SortKey] # rejected keys — the view turns these into warning toasts + + +def apply_sort( + queryset: QuerySet, find: FindFilter, sort_map: SortMap, default_sort: SortString +) -> SortResult: + terms, unknown = parse_sort_terms(find.sort or "", sort_map) + if not terms: + # default_sort is trusted developer config — ignore any "unknown" from it + terms, _ = parse_sort_terms(default_sort, sort_map) + annotations: Annotations = {} + order_by: list[OrderField] = [] + for term in terms: + spec = sort_map[term.key] + if spec.annotate: + annotations.update(spec.annotate) + order_by.append(("-" if term.descending else "") + spec.expression) + if annotations: + queryset = queryset.annotate(**annotations) + return SortResult(queryset.order_by(*order_by), terms, unknown) + + +def parse_find_filter(request: HttpRequest) -> FindFilter: + return FindFilter(sort=request.GET.get("sort") or None) # FindFilter.sort holds a SortString +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py -v` +Expected: PASS (all Task 1 + Task 2 tests). + +- [ ] **Step 5: Commit** + +```bash +git add games/sorting.py tests/test_sorting.py +git commit -m "feat(sorting): per-model maps, apply_sort, parse_find_filter (#68)" +``` + +--- + +## Task 3: Wire `list_games` (sort + N+1 + warnings) + +**Files:** +- Modify: `games/views/game.py` (`list_games`, starts line 57; base queryset line 59) +- Test: `tests/test_sorting.py` + +**Interfaces:** +- Consumes: `apply_sort`, `parse_find_filter`, `GAME_SORTS`, `GAME_DEFAULT_SORT` from `games.sorting`. +- Produces: `GET /games/?sort=<...>` honored; unknown key → warning message. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_sorting.py`: + +```python +from django.contrib.messages import get_messages +from django.urls import reverse + + +@pytest.fixture +def logged_client(client, django_user_model): + user = django_user_model.objects.create_user(username="u", password="p") + client.force_login(user) + return client + + +class TestListGamesSort: + def test_sort_param_orders_rows(self, logged_client, two_games): + alpha, beta = two_games + response = logged_client.get(reverse("games:list_games"), {"sort": "-name"}) + assert response.status_code == 200 + body = response.content.decode() + assert body.index("Beta") < body.index("Alpha") + + def test_unknown_sort_emits_warning_message(self, logged_client, two_games): + response = logged_client.get(reverse("games:list_games"), {"sort": "bogus"}) + assert response.status_code == 200 + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert any("bogus" in w for w in warnings) + + def test_valid_sort_emits_no_warning(self, logged_client, two_games): + response = logged_client.get(reverse("games:list_games"), {"sort": "name"}) + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert warnings == [] +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestListGamesSort -v` +Expected: FAIL — `test_sort_param_orders_rows` fails (rows still ordered by `-created_at`, so `-name` ignored); `test_unknown_sort_emits_warning_message` fails (no message). + +- [ ] **Step 3: Write minimal implementation** + +In `games/views/game.py`, add to the imports from `django.contrib`: + +```python +from django.contrib import messages +``` + +Add to the `games.sorting` import (new import line near the other `games.*` imports, e.g. after `from games.filters import parse_game_filter`): + +```python +from games.sorting import GAME_DEFAULT_SORT, GAME_SORTS, apply_sort, parse_find_filter +``` + +Change the base queryset (line 59) from: + +```python + games = Game.objects.order_by("-created_at") +``` + +to: + +```python + games = Game.objects.select_related("platform") +``` + +Then, immediately before `games, page_obj, elided_page_range = paginate(request, games)` (line 89), insert: + +```python + sort = apply_sort(games, parse_find_filter(request), GAME_SORTS, GAME_DEFAULT_SORT) + games = sort.queryset + for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestListGamesSort -v` +Expected: PASS (3 tests). + +- [ ] **Step 5: Commit** + +```bash +git add games/views/game.py tests/test_sorting.py +git commit -m "feat(games): honor ?sort= on list_games + eager-load platform (#68)" +``` + +--- + +## Task 4: Wire `list_sessions` (sort + N+1 + warnings) + +**Files:** +- Modify: `games/views/session.py` (`list_sessions`, starts line 120; base queryset line 122) +- Test: `tests/test_sorting.py` + +**Interfaces:** +- Consumes: `apply_sort`, `parse_find_filter`, `SESSION_SORTS`, `SESSION_DEFAULT_SORT`. +- Produces: `GET /sessions/?sort=<...>` honored; unknown key → warning. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_sorting.py`: + +```python +class TestListSessionsSort: + def test_sort_by_duration_descending(self, logged_client, two_games): + alpha, beta = two_games + Session.objects.create( + game=alpha, + timestamp_start=datetime(2022, 1, 1, 10, tzinfo=ZONEINFO), + timestamp_end=datetime(2022, 1, 1, 10, 30, tzinfo=ZONEINFO), # 30 min + ) + Session.objects.create( + game=beta, + timestamp_start=datetime(2022, 1, 2, 10, tzinfo=ZONEINFO), + timestamp_end=datetime(2022, 1, 2, 13, tzinfo=ZONEINFO), # 3 h + ) + response = logged_client.get(reverse("games:list_sessions"), {"sort": "-duration"}) + assert response.status_code == 200 + body = response.content.decode() + assert body.index("Beta") < body.index("Alpha") # longer session first + + def test_unknown_sort_emits_warning(self, logged_client, two_games): + response = logged_client.get(reverse("games:list_sessions"), {"sort": "nope"}) + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert any("nope" in w for w in warnings) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestListSessionsSort -v` +Expected: FAIL — sort ignored / no warning. + +- [ ] **Step 3: Write minimal implementation** + +In `games/views/session.py`, add the imports: + +```python +from django.contrib import messages +from games.sorting import ( + SESSION_DEFAULT_SORT, + SESSION_SORTS, + apply_sort, + parse_find_filter, +) +``` + +Change the base queryset (line 122) from: + +```python + sessions = Session.objects.order_by("-timestamp_start", "created_at") +``` + +to: + +```python + sessions = Session.objects.select_related("game", "game__platform", "device") +``` + +Then, immediately before `sessions, page_obj, elided_page_range = paginate(request, sessions)` (line 148), insert: + +```python + sort = apply_sort(sessions, parse_find_filter(request), SESSION_SORTS, SESSION_DEFAULT_SORT) + sessions = sort.queryset + for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") +``` + +> Note: `last_session = sessions.latest()` (line 145) runs before pagination and is unaffected by `order_by` (`.latest()` uses the model's `get_latest_by`); leave it as-is. The `apply_sort` call goes after it, before `paginate`. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestListSessionsSort -v` +Expected: PASS (2 tests). + +- [ ] **Step 5: Commit** + +```bash +git add games/views/session.py tests/test_sorting.py +git commit -m "feat(sessions): honor ?sort= on list_sessions + eager-load relations (#68)" +``` + +--- + +## Task 5: Wire `list_purchases` (sort + N+1 + warnings) + +**Files:** +- Modify: `games/views/purchase.py` (`list_purchases`, starts line 121; base queryset line 122) +- Test: `tests/test_sorting.py` + +**Interfaces:** +- Consumes: `apply_sort`, `parse_find_filter`, `PURCHASE_SORTS`, `PURCHASE_DEFAULT_SORT`. +- Produces: `GET /purchases/?sort=<...>` honored; unknown key → warning; `name`/`finished` aggregate sorts do not duplicate rows. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_sorting.py`: + +```python +class TestListPurchasesSort: + @pytest.fixture + def two_purchases(self, db, two_games): + alpha, beta = two_games + cheap = Purchase.objects.create( + date_purchased=datetime(2022, 1, 1, tzinfo=ZONEINFO), + price=10, + converted_price=10, + platform=alpha.platform, + ) + cheap.games.add(alpha) + dear = Purchase.objects.create( + date_purchased=datetime(2022, 1, 2, tzinfo=ZONEINFO), + price=90, + converted_price=90, + platform=beta.platform, + ) + dear.games.add(beta) + return cheap, dear + + def test_sort_by_price_descending(self, logged_client, two_purchases): + response = logged_client.get(reverse("games:list_purchases"), {"sort": "-price"}) + assert response.status_code == 200 + body = response.content.decode() + assert body.index("Beta") < body.index("Alpha") # 90 before 10 + + def test_name_aggregate_sort_no_duplicate_rows(self, logged_client, two_purchases): + # a multi-game purchase must still render exactly one row + cheap, _ = two_purchases + from games.models import Game + extra = Game.objects.create(name="Aaa", sort_name="Aaa", platform=cheap.platform) + cheap.games.add(extra) + response = logged_client.get(reverse("games:list_purchases"), {"sort": "name"}) + body = response.content.decode() + assert body.count("purchase-row-") == 2 # exactly two purchase rows + + def test_unknown_sort_emits_warning(self, logged_client, two_purchases): + response = logged_client.get(reverse("games:list_purchases"), {"sort": "nope"}) + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert any("nope" in w for w in warnings) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestListPurchasesSort -v` +Expected: FAIL — sort ignored / no warning. + +- [ ] **Step 3: Write minimal implementation** + +In `games/views/purchase.py`, add the imports: + +```python +from django.contrib import messages +from games.sorting import ( + PURCHASE_DEFAULT_SORT, + PURCHASE_SORTS, + apply_sort, + parse_find_filter, +) +``` + +Change the base queryset (line 122) from: + +```python + purchases = Purchase.objects.order_by("-date_purchased", "-created_at") +``` + +to: + +```python + purchases = Purchase.objects.prefetch_related("games", "games__platform") +``` + +Then, immediately before `purchases, page_obj, elided_page_range = paginate(request, purchases)` (line 132), insert: + +```python + sort = apply_sort(purchases, parse_find_filter(request), PURCHASE_SORTS, PURCHASE_DEFAULT_SORT) + purchases = sort.queryset + for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") +``` + +> If `apply_sort` ever yields duplicate purchase rows for an aggregate sort (it should not — `Min`/`Max` group by PK), add `.distinct()` after `order_by` in the view; the `test_name_aggregate_sort_no_duplicate_rows` test guards this. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestListPurchasesSort -v` +Expected: PASS (3 tests). + +- [ ] **Step 5: Run the full suite + lint** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py -v && make lint` +Expected: all sorting tests PASS; ruff clean. + +- [ ] **Step 6: Commit** + +```bash +git add games/views/purchase.py tests/test_sorting.py +git commit -m "feat(purchases): honor ?sort= on list_purchases + eager-load games (#68)" +``` + +--- + +## Task 6: Regression smoke + full suite + +**Files:** +- Test: `tests/test_sorting.py` + +**Interfaces:** +- Consumes: all prior tasks. +- Produces: confidence that default order is unchanged and every map key returns 200. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_sorting.py`: + +```python +class TestDefaultOrderUnchanged: + """The default sort strings must reproduce the pre-#68 hardcoded order.""" + + def test_games_default_is_created_descending(self, logged_client, two_games): + alpha, beta = two_games # beta newer + response = logged_client.get(reverse("games:list_games")) + body = response.content.decode() + assert body.index("Beta") < body.index("Alpha") + + +class TestEverySortKeyReturns200: + def test_all_game_keys(self, logged_client, two_games): + for key in GAME_SORTS: + for raw in (key, f"-{key}"): + response = logged_client.get(reverse("games:list_games"), {"sort": raw}) + assert response.status_code == 200, raw + + def test_all_session_keys(self, logged_client, two_games): + for key in SESSION_SORTS: + response = logged_client.get(reverse("games:list_sessions"), {"sort": key}) + assert response.status_code == 200, key + + def test_all_purchase_keys(self, logged_client, two_games): + for key in PURCHASE_SORTS: + response = logged_client.get(reverse("games:list_purchases"), {"sort": key}) + assert response.status_code == 200, key +``` + +- [ ] **Step 2: Run to verify it passes (these assert already-correct behavior)** + +Run: `uv run --with pytest-django pytest tests/test_sorting.py::TestDefaultOrderUnchanged tests/test_sorting.py::TestEverySortKeyReturns200 -v` +Expected: PASS. If any `?sort=` returns 500, that key's expression/annotation is wrong — fix the offending `SortSpec` in `games/sorting.py` and re-run. + +- [ ] **Step 3: Run the entire project test suite** + +Run: `make test` +Expected: PASS (no regressions; note `make test` also collects `e2e/` — a browser must be available, or run `uv run --with pytest-django pytest tests/` to scope to unit/integration tests). + +- [ ] **Step 4: Lint + format check** + +Run: `make check` +Expected: ruff + format + tests clean. + +- [ ] **Step 5: Commit** + +```bash +git add tests/test_sorting.py +git commit -m "test(sorting): default-order regression + all-keys smoke (#68)" +``` + +--- + +## Post-implementation (not tasks) + +- **PR description** must call out the #65 coordination (per spec): each stats "view all" link's `sort=` must use a key in the target model's `*_SORTS` map. Verify when #65 lands. +- Follow-ups already filed: #73 (header UI), #74 (FindFilter unify + dead `direction`/`page`/`per_page`), #75 (purchase free-text search), #76 (shared `list_view` helper), #77 (presets persist/restore sort). Do not address them here. diff --git a/docs/superpowers/specs/2026-06-21-list-view-sort-param-design.md b/docs/superpowers/specs/2026-06-21-list-view-sort-param-design.md new file mode 100644 index 0000000..82c47c3 --- /dev/null +++ b/docs/superpowers/specs/2026-06-21-list-view-sort-param-design.md @@ -0,0 +1,310 @@ +# Honor `sort`/`direction` query params on list views (#68) + +## Problem + +The stats page (#65) adds "View all (N) →" links from its sorted lists to the +filtered list views. The list views (`list_games`, `list_sessions`, +`list_purchases`) hardcode `order_by(...)` and ignore any sort parameter, so a +"view all" link lands on the right *set* of rows but not in the same *order* the +stats page showed. + +`FindFilter` (in `games/filters.py`) already models `sort` / `direction` but is +never parsed or applied anywhere. + +Hardcoded orders today: + +- `games/views/game.py:59` — `Game.objects.order_by("-created_at")` +- `games/views/session.py:122` — `Session.objects.order_by("-timestamp_start", "created_at")` +- `games/views/purchase.py:122` — `Purchase.objects.order_by("-date_purchased", "-created_at")` + +## Goal + +Make all three list views honor a `sort` query param (a signed comma-list of +column keys), with every visible column sortable plus the aggregate sorts the +stats page needs for "view all" parity (games-by-playtime, finish-date). Backend +only — no clickable header UI in this issue (tracked in #73). + +## Scope decisions (from brainstorming) + +- **Sortable set:** all visible list columns, plus stats-parity aggregates + (playtime, finish date) even where they are not visible columns. +- **UI:** backend only. Honor the query param; do not add clickable column + headers. File a follow-up issue for the header UI. +- **URL contract:** a single signed comma-list of **public keys** — + `?sort=-playtime,name`. The `-` attaches to the public key and sets that + key's direction; the whitelist still maps the key to its internal + expression+annotation. Bare key = **ascending** (Django semantics, no hidden + per-key defaults); leading `-` = descending. Rationale: natural multi-column, + one param, each term self-describes direction, mirrors Django `order_by`, and + is **forward-compatible to multi-column with zero contract change**. +- **Multi-column:** the backend parses and applies the **full** term list now + (server-side multi-column works immediately). Only the UI that *generates* + multi-term URLs (clickable headers, shift-click to add a column) is deferred + to the follow-up issue. +- **Default ordering:** since bare keys are ascending and signs are explicit, + each view's default order is itself just a default signed sort string parsed + by the same machinery — no separate `default_direction` field, no per-key + tiebreak concept. +- **Module:** new `games/sorting.py` (keeps the already-large `filters.py` + focused; imports `FindFilter` from it). +- **Purchase name sort:** annotated `Min("games__name")` — one row per purchase, + no M2M duplication. + +## Prerequisite: fix N+1 queries on list rows + +The list querysets currently have **no** `select_related`/`prefetch_related`, so +each rendered row lazy-loads its relations. The sort work touches these exact +querysets (and adds aggregate annotations), so the eager-loading fix lands here: + +- `list_games` — `Game.objects.select_related("platform")` (icon via + `NameWithIcon`). +- `list_sessions` — `Session.objects.select_related("game", "game__platform", "device")`. +- `list_purchases` — `Purchase.objects.prefetch_related("games", "games__platform")` + (M2M rendered by `LinkedPurchase`; confirm `games__platform` need during impl). + +These are added to the **base** queryset (before filtering/annotating), so they +compose with `apply_sort`'s annotations. + +## Design + +### URL contract + +`?sort=[,...]` on `list_games`, `list_sessions`, +`list_purchases`. Examples: `?sort=-playtime`, `?sort=status,name`, +`?sort=-date,created`. + +- Each term is a public key, optionally prefixed `-`. Bare = ascending; `-` = + descending (Django `order_by` semantics). +- Terms whose key is not in the model's map are **ignored, and a user-facing + warning toast is shown** ("Unknown sort field '' was ignored.") — never a + 400. Any remaining valid terms still apply; the page renders normally. +- If `sort` is absent, empty, or has no valid terms → the view's default sort + string is used (parsed by the same machinery). +- Invalid values never raise. + +The warning surfaces drift (e.g. a #65 "view all" link with a stale/typo'd key) +without breaking a user-facing, hand-editable URL. It rides the existing +messages→toast path: `render_page()` serializes `get_messages(request)` into the +`#django-messages` JSON block (`common/layout.py`) and `toast.js` renders it — +works on a full-page GET, which is what #65 links are. No new plumbing. + +`sorting.py` itself stays HTTP-free: it *reports* unknown keys; the view emits +the `messages.warning`. + +Pagination already preserves the param: `_page_url` (in +`common/components/primitives.py`) copies `request.GET` and only replaces `page`. + +### `games/sorting.py` + +Named string roles (PEP 695 transparent aliases — `requires-python >=3.13`). +They read like TS `type X = string`: no runtime cost, no wrapping ceremony, but +each `str` in a signature now says *which* string it is: + +```python +from django.db.models import Aggregate + +type SortKey = str # public column key in a *_SORTS map and in a URL term ("playtime", "name") +type SortString = str # comma-list of signed SortKeys: the URL ?sort= value and *_DEFAULT_SORT ("-date,created") +type AnnotationName = str # an alias added via .annotate(), then referenced by SortSpec.expression +type OrderField = str # SortSpec.expression: a real model field path OR an AnnotationName + +# alias name -> the ORM aggregate that computes it, applied via queryset.annotate() +# e.g. {"total_playtime": Sum("sessions__duration_total")} +type Annotations = dict[AnnotationName, Aggregate] + +@dataclass(frozen=True) +class SortSpec: + expression: OrderField # unsigned; a real column path or an AnnotationName + annotate: Annotations | None = None +``` + +All current sorts use `Aggregate` subclasses (`Sum`/`Max`/`Min`); if a +non-aggregate annotation (`F`, `ExpressionWrapper`) is ever needed, widen +`Annotations`' value to `Combinable` then. + +Direction is never stored on the spec — it comes from the sign in the URL term. +Cross-relation sorts use **annotated aggregates** (`Sum`/`Max`/`Min`), which +group by the model PK and therefore produce no duplicate rows. Bare +`order_by("relation__field")` is never used for to-many relations. + +#### Per-model maps + default sort strings + +All three maps are typed `SortMap` (`dict[SortKey, SortSpec]`); each +`*_DEFAULT_SORT` is a `SortString`. + +**`GAME_SORTS`** — `GAME_DEFAULT_SORT = "-created"`: + +| key | expression | annotate | +|---|---|---| +| name | `name` | — | +| sort_name | `sort_name` | — | +| year | `year_released` | — | +| status | `status` | — | +| wikidata | `wikidata` | — | +| created | `created_at` | — | +| playtime | `total_playtime` | `Sum("sessions__duration_total")` | +| finished | `last_finished` | `Max("playevents__ended")` | + +**`SESSION_SORTS`** — `SESSION_DEFAULT_SORT = "-date,created"` +(reproduces today's `order_by("-timestamp_start", "created_at")`): + +| key | expression | annotate | +|---|---|---| +| name | `game__sort_name` | — (to-one; safe) | +| date | `timestamp_start` | — | +| duration | `duration_total` | — | +| device | `device__name` | — (to-one; safe) | +| created | `created_at` | — | + +**`PURCHASE_SORTS`** — `PURCHASE_DEFAULT_SORT = "-purchased,-created"` +(reproduces today's `order_by("-date_purchased", "-created_at")`): + +| key | expression | annotate | +|---|---|---| +| name | `first_game_name` | `Min("games__name")` | +| type | `type` | — | +| price | `converted_price` | — | +| infinite | `infinite` | — | +| purchased | `date_purchased` | — | +| refunded | `date_refunded` | — | +| created | `created_at` | — | +| finished | `last_finished` | `Max("games__playevents__ended")` | + +> `game__sort_name` / `device__name` are to-one relations — no duplication, so +> no annotation needed. +> The `finished` purchase key gives the stats "view all" finish-date parity. +> Model field names confirmed against `games/models.py`: `infinite`, +> `converted_price`, `date_purchased`, `date_refunded`, `type`; session +> `duration_total` is a `GeneratedField` (orderable). + +#### Term parsing + +```python +class SortTerm(NamedTuple): + key: SortKey + descending: bool # True = "-key" (desc), False = bare key (asc) + +type SortMap = dict[SortKey, SortSpec] + +class ParsedSort(NamedTuple): + terms: list[SortTerm] + unknown: list[SortKey] # keys not in the map — the view turns these into warnings + +def parse_sort_terms(raw: SortString, sort_map: SortMap) -> ParsedSort: + terms, unknown = [], [] + for token in raw.split(","): + token = token.strip() + if not token: + continue + descending = token.startswith("-") + key = token.lstrip("-") + if key in sort_map: + terms.append(SortTerm(key, descending)) + else: + unknown.append(key) + return ParsedSort(terms, unknown) +``` + +### Apply helper + +```python +class SortResult(NamedTuple): + queryset: QuerySet + terms: list[SortTerm] # the order actually applied — #73's header UI consumes this + unknown: list[SortKey] # rejected keys — the view turns these into warning toasts + +def apply_sort( + queryset: QuerySet, find: FindFilter, sort_map: SortMap, default_sort: SortString +) -> SortResult: + terms, unknown = parse_sort_terms(find.sort or "", sort_map) + if not terms: + # default_sort is trusted developer config — ignore any "unknown" from it + terms, _ = parse_sort_terms(default_sort, sort_map) + annotations: Annotations = {} + order_by: list[OrderField] = [] + for term in terms: + spec = sort_map[term.key] + if spec.annotate: + annotations.update(spec.annotate) + order_by.append(("-" if term.descending else "") + spec.expression) + if annotations: + queryset = queryset.annotate(**annotations) + return SortResult(queryset.order_by(*order_by), terms, unknown) +``` + +The full term list is applied (server-side multi-column). `SortResult.terms` is +what the future header UI (#73) consumes; `SortResult.unknown` is what the view +turns into warning toasts. + +### FindFilter parsing + +```python +def parse_find_filter(request: HttpRequest) -> FindFilter: + return FindFilter(sort=request.GET.get("sort") or None) # FindFilter.sort holds a SortString +``` + +`FindFilter.direction` is **not used** by #68 — direction lives in the sign of +each `sort` term, not a separate field. Nothing serializes it today (the +`FilterPreset.find_filter` `JSONField` is currently unpopulated; `FindFilter` +has no JSON round-trip), so #68 leaves the field untouched rather than churn it. +Whether to remove it or formally wire it is decided in **#74**. Page / per_page +likewise stay with `paginate()`; not wired into `FindFilter` now (YAGNI, #74). + +### View wiring (×3) + +In each list view, remove the hardcoded `.order_by(...)` from the base queryset +and, after filtering and before `paginate()`: + +```python +find = parse_find_filter(request) +sort = apply_sort(games, find, GAME_SORTS, GAME_DEFAULT_SORT) # sort: SortResult +games = sort.queryset +for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") +``` + +(`session.py` / `purchase.py` analogous with their maps + default sort strings.) + +## Testing + +`tests/test_sorting.py`: + +- Default order unchanged for each model when no `sort` param is present + (regression guard against the removed hardcoded `order_by` — the default sort + string must reproduce the old order exactly). +- Each key in each map sorts ascending (bare) and descending (`-` prefix). +- Multi-column term list applies in order (e.g. `?sort=status,name`). +- Unknown keys are reported in `SortResult.unknown`; the view emits a + `messages.warning` per unknown key (assert on `get_messages`). A valid key + emits none. +- An all-invalid/empty `sort` falls back to the default sort string. +- A mixed `?sort=-playtime,bogus` both sorts by `playtime` *and* warns on `bogus`. +- Annotated sorts (game playtime, game/purchase finish date, purchase name) + return no duplicate rows (`count` equals unsorted `count`). +- `parse_sort_terms` unit tests: signs, whitespace, empty tokens, unknown keys. +- Smoke: `?sort=` and `?sort=-key,key` URLs return 200 for each list view. + +## Coordination with #65 + +#65 (stats "view all" links, still unmerged) generates URLs that pass `sort=`. +Those links **must use the same public sort keys** defined in this spec's maps +(`playtime`, `finished`, etc.). When #65 and #68 both land, verify each "view +all" link's `sort=` matches a key in the target model's `*_SORTS` map and +reproduces the stats list's order. Call this out in the #68 PR description. + +## Related follow-ups (filed during design review) + +- **#73** — clickable sortable column headers + multi-column UI (consumes + `SortResult.terms`). +- **#74** — make `FindFilter` the single request parser (sort + pagination + + free-text); fixes the `paginate()` 10-vs-25 / `limit`-vs-`per_page` mismatch. +- **#75** — purchase list free-text search parity. +- **#76** — extract a shared `list_view` helper across the three list views. +- **#77** — saved filter presets should persist/restore sort (`FilterPreset.find_filter`). + +## Out of scope (follow-up issue) + +Clickable sortable column headers (asc/desc indicator, toggle on click) and the +multi-column UI affordance (shift-click to add a column → multi-term `?sort=` +URL). The backend `apply_sort` already returns `SortResult.terms` and applies +the full term list, so this is UI-only work. Tracked in **#73**. diff --git a/games/sorting.py b/games/sorting.py new file mode 100644 index 0000000..6f53845 --- /dev/null +++ b/games/sorting.py @@ -0,0 +1,140 @@ +"""Structured sorting for list views (Stash-inspired, paired with games/filters.py). + +A list view maps a public sort key to a SortSpec; the URL ?sort= param is a +signed comma-list of those keys (e.g. "-playtime,name"). See +docs/superpowers/specs/2026-06-21-list-view-sort-param-design.md. +""" + +from dataclasses import dataclass +from typing import NamedTuple + +from django.db.models import Aggregate, Max, Min, QuerySet, Sum +from django.http import HttpRequest + +from games.filters import FindFilter + +type SortKey = ( + str # public column key in a *_SORTS map and in a URL term ("playtime", "name") +) +type SortString = str # comma-list of signed SortKeys: the URL ?sort= value and *_DEFAULT_SORT ("-date,created") +type AnnotationName = ( + str # an alias added via .annotate(), then referenced by SortSpec.expression +) +type OrderField = ( + str # SortSpec.expression: a real model field path OR an AnnotationName +) + +# alias name -> the ORM aggregate that computes it, applied via queryset.annotate() +# e.g. {"total_playtime": Sum("sessions__duration_total")} +type Annotations = dict[AnnotationName, Aggregate] + + +@dataclass(frozen=True) +class SortSpec: + expression: OrderField # unsigned; a real column path or an AnnotationName + annotate: Annotations | None = None + + +class SortTerm(NamedTuple): + key: SortKey + descending: bool # True = "-key" (desc), False = bare key (asc) + + +type SortMap = dict[SortKey, SortSpec] + + +class ParsedSort(NamedTuple): + terms: list[SortTerm] + unknown: list[SortKey] # keys not in the map — the view turns these into warnings + + +def parse_sort_terms(raw: SortString, sort_map: SortMap) -> ParsedSort: + terms: list[SortTerm] = [] + unknown: list[SortKey] = [] + for token in raw.split(","): + token = token.strip() + if not token: + continue + descending = token.startswith("-") + key = token.lstrip("-") + if key in sort_map: + terms.append(SortTerm(key, descending)) + else: + unknown.append(key) + return ParsedSort(terms, unknown) + + +# ── Per-model sort maps ───────────────────────────────────────────────────── +# Cross-relation sorts use annotated aggregates (group by PK → no row dup). +# To-one relations (game__sort_name, device__name) are ordered directly. + +GAME_SORTS: SortMap = { + "name": SortSpec("name"), + "sort_name": SortSpec("sort_name"), + "year": SortSpec("year_released"), + "status": SortSpec("status"), + "wikidata": SortSpec("wikidata"), + "created": SortSpec("created_at"), + "playtime": SortSpec( + "total_playtime", {"total_playtime": Sum("sessions__duration_total")} + ), + "finished": SortSpec("last_finished", {"last_finished": Max("playevents__ended")}), +} +GAME_DEFAULT_SORT: SortString = "-created" + +SESSION_SORTS: SortMap = { + "name": SortSpec("game__sort_name"), + "date": SortSpec("timestamp_start"), + "duration": SortSpec("duration_total"), + "device": SortSpec("device__name"), + "created": SortSpec("created_at"), +} +SESSION_DEFAULT_SORT: SortString = "-date,created" + +PURCHASE_SORTS: SortMap = { + "name": SortSpec("first_game_name", {"first_game_name": Min("games__name")}), + "type": SortSpec("type"), + "price": SortSpec("converted_price"), + "infinite": SortSpec("infinite"), + "purchased": SortSpec("date_purchased"), + "refunded": SortSpec("date_refunded"), + "created": SortSpec("created_at"), + "finished": SortSpec( + "last_finished", {"last_finished": Max("games__playevents__ended")} + ), +} +PURCHASE_DEFAULT_SORT: SortString = "-purchased,-created" + + +# ── Apply ─────────────────────────────────────────────────────────────────── + + +class SortResult(NamedTuple): + queryset: QuerySet + terms: list[SortTerm] # the order actually applied — #73's header UI consumes this + unknown: list[SortKey] # rejected keys — the view turns these into warning toasts + + +def apply_sort( + queryset: QuerySet, find: FindFilter, sort_map: SortMap, default_sort: SortString +) -> SortResult: + terms, unknown = parse_sort_terms(find.sort or "", sort_map) + if not terms: + # default_sort is trusted developer config — ignore any "unknown" from it + terms, _ = parse_sort_terms(default_sort, sort_map) + annotations: Annotations = {} + order_by: list[OrderField] = [] + for term in terms: + spec = sort_map[term.key] + if spec.annotate: + annotations.update(spec.annotate) + order_by.append(("-" if term.descending else "") + spec.expression) + if annotations: + queryset = queryset.annotate(**annotations) + return SortResult(queryset.order_by(*order_by), terms, unknown) + + +def parse_find_filter(request: HttpRequest) -> FindFilter: + return FindFilter( + sort=request.GET.get("sort") or None + ) # FindFilter.sort holds a SortString diff --git a/games/views/game.py b/games/views/game.py index ab97034..1386fde 100644 --- a/games/views/game.py +++ b/games/views/game.py @@ -1,5 +1,6 @@ from typing import Any +from django.contrib import messages from django.contrib.auth.decorators import login_required from django.core.paginator import Paginator from django.db.models import Q @@ -48,6 +49,7 @@ from common.time import ( ) from common.utils import build_dynamic_filter, paginate, safe_division, truncate from games.filters import parse_game_filter +from games.sorting import GAME_DEFAULT_SORT, GAME_SORTS, apply_sort, parse_find_filter from games.forms import GameForm from games.models import Game from games.views.general import use_custom_redirect @@ -56,7 +58,7 @@ from games.views.playevent import create_playevent_tabledata @login_required def list_games(request: HttpRequest, search_string: str = "") -> HttpResponse: - games = Game.objects.order_by("-created_at") + games = Game.objects.select_related("platform") # ── Structured filter (Stash-style JSON) ── filter_json = request.GET.get("filter", "") @@ -86,6 +88,11 @@ def list_games(request: HttpRequest, search_string: str = "") -> HttpResponse: filters.append(Q(status=search_status)) games = games.filter(build_dynamic_filter(filters, "|")) + sort = apply_sort(games, parse_find_filter(request), GAME_SORTS, GAME_DEFAULT_SORT) + games = sort.queryset + for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") + games, page_obj, elided_page_range = paginate(request, games) data = { diff --git a/games/views/purchase.py b/games/views/purchase.py index 3600eb1..07c8186 100644 --- a/games/views/purchase.py +++ b/games/views/purchase.py @@ -44,6 +44,12 @@ from common.time import dateformat from common.utils import paginate from games.forms import PurchaseForm from games.models import Game, Purchase +from games.sorting import ( + PURCHASE_DEFAULT_SORT, + PURCHASE_SORTS, + apply_sort, + parse_find_filter, +) from games.views.general import use_custom_redirect @@ -119,7 +125,9 @@ def _render_purchase_row(purchase): @login_required def list_purchases(request: HttpRequest) -> HttpResponse: - purchases = Purchase.objects.order_by("-date_purchased", "-created_at") + purchases = Purchase.objects.select_related("platform").prefetch_related( + "games", "games__platform" + ) filter_json = request.GET.get("filter", "") if filter_json: @@ -129,6 +137,13 @@ def list_purchases(request: HttpRequest) -> HttpResponse: if pf is not None: purchases = purchases.filter(pf.to_q()) + sort = apply_sort( + purchases, parse_find_filter(request), PURCHASE_SORTS, PURCHASE_DEFAULT_SORT + ) + purchases = sort.queryset + for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") + purchases, page_obj, elided_page_range = paginate(request, purchases) data = { diff --git a/games/views/session.py b/games/views/session.py index a52c537..091c48a 100644 --- a/games/views/session.py +++ b/games/views/session.py @@ -1,5 +1,6 @@ from typing import Any, TypedDict +from django.contrib import messages from django.contrib.auth.decorators import login_required from django.db.models import Q from django.http import HttpRequest, HttpResponse @@ -38,6 +39,12 @@ from common.time import ( from common.utils import paginate, truncate from games.forms import SessionForm from games.models import Device, Game, Session +from games.sorting import ( + SESSION_DEFAULT_SORT, + SESSION_SORTS, + apply_sort, + parse_find_filter, +) class SessionRowData(TypedDict): @@ -119,7 +126,7 @@ def session_row(session: Session, device_list, csrf_token: str) -> Node: @login_required def list_sessions(request: HttpRequest, search_string: str = "") -> HttpResponse: - sessions = Session.objects.order_by("-timestamp_start", "created_at") + sessions = Session.objects.select_related("game", "game__platform", "device") device_list = Device.objects.order_by("name") # ── Structured filter (JSON) ── @@ -145,6 +152,12 @@ def list_sessions(request: HttpRequest, search_string: str = "") -> HttpResponse last_session = sessions.latest() except Session.DoesNotExist: last_session = None + sort = apply_sort( + sessions, parse_find_filter(request), SESSION_SORTS, SESSION_DEFAULT_SORT + ) + sessions = sort.queryset + for key in sort.unknown: + messages.warning(request, f"Unknown sort field '{key}' was ignored.") sessions, page_obj, elided_page_range = paginate(request, sessions) csrf_token = get_token(request) diff --git a/tests/test_sorting.py b/tests/test_sorting.py new file mode 100644 index 0000000..295af8b --- /dev/null +++ b/tests/test_sorting.py @@ -0,0 +1,301 @@ +"""Tests for the list-view sorting system (games/sorting.py).""" + +import re +from datetime import datetime +from zoneinfo import ZoneInfo + +import pytest +from django.conf import settings +from django.contrib.messages import get_messages +from django.test import RequestFactory +from django.urls import reverse + +from games.filters import FindFilter +from games.models import Game, Platform, Purchase, Session +from games.sorting import ( + GAME_DEFAULT_SORT, + GAME_SORTS, + PURCHASE_DEFAULT_SORT, + PURCHASE_SORTS, + SESSION_DEFAULT_SORT, + SESSION_SORTS, + SortSpec, + SortTerm, + apply_sort, + parse_find_filter, + parse_sort_terms, +) + +ZONEINFO = ZoneInfo(settings.TIME_ZONE) + +# A minimal map; parse_sort_terms only checks key membership, not spec internals. +SAMPLE_MAP = {"name": SortSpec("name"), "date": SortSpec("created_at")} + + +class TestParseSortTerms: + def test_bare_key_is_ascending(self): + parsed = parse_sort_terms("name", SAMPLE_MAP) + assert parsed.terms == [SortTerm("name", False)] + assert parsed.unknown == [] + + def test_dash_prefix_is_descending(self): + parsed = parse_sort_terms("-date", SAMPLE_MAP) + assert parsed.terms == [SortTerm("date", True)] + + def test_multi_column_preserves_order(self): + parsed = parse_sort_terms("date,-name", SAMPLE_MAP) + assert parsed.terms == [SortTerm("date", False), SortTerm("name", True)] + + def test_unknown_key_is_reported_not_raised(self): + parsed = parse_sort_terms("bogus", SAMPLE_MAP) + assert parsed.terms == [] + assert parsed.unknown == ["bogus"] + + def test_mixed_valid_and_unknown(self): + parsed = parse_sort_terms("-name,bogus", SAMPLE_MAP) + assert parsed.terms == [SortTerm("name", True)] + assert parsed.unknown == ["bogus"] + + def test_whitespace_and_empty_tokens_ignored(self): + parsed = parse_sort_terms(" name , , -date ", SAMPLE_MAP) + assert parsed.terms == [SortTerm("name", False), SortTerm("date", True)] + + def test_empty_string_yields_nothing(self): + parsed = parse_sort_terms("", SAMPLE_MAP) + assert parsed.terms == [] + assert parsed.unknown == [] + + +def _find(sort=None): + return FindFilter(sort=sort) + + +@pytest.fixture +def two_games(db): + platform = Platform.objects.create(name="P", icon="p") + alpha = Game.objects.create(name="Alpha", sort_name="Alpha", platform=platform) + beta = Game.objects.create(name="Beta", sort_name="Beta", platform=platform) + return alpha, beta + + +class TestApplySortGames: + def test_name_ascending(self, two_games): + alpha, beta = two_games + result = apply_sort( + Game.objects.all(), _find("name"), GAME_SORTS, GAME_DEFAULT_SORT + ) + assert list(result.queryset) == [alpha, beta] + assert result.terms[0].key == "name" + assert result.unknown == [] + + def test_name_descending(self, two_games): + alpha, beta = two_games + result = apply_sort( + Game.objects.all(), _find("-name"), GAME_SORTS, GAME_DEFAULT_SORT + ) + assert list(result.queryset) == [beta, alpha] + + def test_default_sort_when_absent_is_created_desc(self, two_games): + alpha, beta = two_games # beta created after alpha + result = apply_sort( + Game.objects.all(), _find(None), GAME_SORTS, GAME_DEFAULT_SORT + ) + assert list(result.queryset) == [beta, alpha] + + def test_unknown_key_reported_and_falls_back(self, two_games): + result = apply_sort( + Game.objects.all(), _find("bogus"), GAME_SORTS, GAME_DEFAULT_SORT + ) + assert result.unknown == ["bogus"] + assert result.queryset.count() == 2 # still returns rows (default order) + + def test_playtime_annotation_no_duplicate_rows(self, two_games): + alpha, _ = two_games + Session.objects.create( + game=alpha, + timestamp_start=datetime(2022, 1, 1, 10, tzinfo=ZONEINFO), + timestamp_end=datetime(2022, 1, 1, 12, tzinfo=ZONEINFO), + ) + Session.objects.create( + game=alpha, + timestamp_start=datetime(2022, 1, 2, 10, tzinfo=ZONEINFO), + timestamp_end=datetime(2022, 1, 2, 11, tzinfo=ZONEINFO), + ) + result = apply_sort( + Game.objects.all(), _find("-playtime"), GAME_SORTS, GAME_DEFAULT_SORT + ) + # two sessions on alpha must not duplicate the alpha row + assert result.queryset.count() == 2 + assert list(result.queryset)[0] == alpha # most playtime first + + +class TestParseFindFilter: + def test_reads_sort_param(self): + request = RequestFactory().get("/x", {"sort": "-playtime,name"}) + assert parse_find_filter(request).sort == "-playtime,name" + + def test_absent_sort_is_none(self): + request = RequestFactory().get("/x") + assert parse_find_filter(request).sort is None + + +class TestSortMapShapes: + def test_default_sort_keys_exist_in_maps(self): + # every key referenced by a default sort string must be defined in its map + for default, sort_map in [ + (GAME_DEFAULT_SORT, GAME_SORTS), + (SESSION_DEFAULT_SORT, SESSION_SORTS), + (PURCHASE_DEFAULT_SORT, PURCHASE_SORTS), + ]: + for token in default.split(","): + assert token.lstrip("-") in sort_map + + +@pytest.fixture +def logged_client(client, django_user_model): + user = django_user_model.objects.create_user(username="u", password="p") + client.force_login(user) + return client + + +class TestListGamesSort: + def test_sort_param_orders_rows(self, logged_client, two_games): + alpha, beta = two_games + response = logged_client.get(reverse("games:list_games"), {"sort": "-name"}) + assert response.status_code == 200 + body = response.content.decode() + assert body.index("Beta") < body.index("Alpha") + # ascending name must flip order vs the default (-created → Beta first) + ascending = logged_client.get(reverse("games:list_games"), {"sort": "name"}) + ascending_body = ascending.content.decode() + assert ascending_body.index("Alpha") < ascending_body.index("Beta") + + def test_unknown_sort_emits_warning_message(self, logged_client, two_games): + response = logged_client.get(reverse("games:list_games"), {"sort": "bogus"}) + assert response.status_code == 200 + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert any("bogus" in w for w in warnings) + + def test_valid_sort_emits_no_warning(self, logged_client, two_games): + response = logged_client.get(reverse("games:list_games"), {"sort": "name"}) + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert warnings == [] + + +class TestListSessionsSort: + def test_sort_by_duration_descending(self, logged_client, two_games): + alpha, beta = two_games + Session.objects.create( + game=alpha, + timestamp_start=datetime(2022, 1, 1, 10, tzinfo=ZONEINFO), + timestamp_end=datetime( + 2022, 1, 1, 13, tzinfo=ZONEINFO + ), # 3 h, earlier date + ) + Session.objects.create( + game=beta, + timestamp_start=datetime(2022, 1, 2, 10, tzinfo=ZONEINFO), + timestamp_end=datetime( + 2022, 1, 2, 10, 30, tzinfo=ZONEINFO + ), # 30 min, later date + ) + # default order is -date (beta first); -duration must override it (alpha's 3h first) + response = logged_client.get( + reverse("games:list_sessions"), {"sort": "-duration"} + ) + assert response.status_code == 200 + body = response.content.decode() + # Extract only the table body to avoid finding "Beta" in header's last-session button + tbody_match = re.search(r"]*>(.*?)", body, re.DOTALL) + assert tbody_match + tbody = tbody_match.group(1) + assert tbody.index("Alpha") < tbody.index( + "Beta" + ) # longer session first, despite earlier date + + def test_unknown_sort_emits_warning(self, logged_client, two_games): + response = logged_client.get(reverse("games:list_sessions"), {"sort": "nope"}) + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert any("nope" in w for w in warnings) + + +class TestListPurchasesSort: + @pytest.fixture + def two_purchases(self, db, two_games): + alpha, beta = two_games + # cheap (Alpha, price=10) purchased LATER so default -purchased order would show Alpha first + # dear (Beta, price=90) purchased EARLIER — so -price must override default order to pass + dear = Purchase.objects.create( + date_purchased=datetime(2022, 1, 1, tzinfo=ZONEINFO), + price=90, + converted_price=90, + platform=beta.platform, + ) + dear.games.add(beta) + cheap = Purchase.objects.create( + date_purchased=datetime(2022, 1, 2, tzinfo=ZONEINFO), + price=10, + converted_price=10, + platform=alpha.platform, + ) + cheap.games.add(alpha) + return cheap, dear + + def test_sort_by_price_descending(self, logged_client, two_purchases): + # default -purchased puts Alpha (later date) first; -price must override to show Beta (90) first + response = logged_client.get( + reverse("games:list_purchases"), {"sort": "-price"} + ) + assert response.status_code == 200 + body = response.content.decode() + tbody_match = re.search(r"]*>(.*?)", body, re.DOTALL) + assert tbody_match + tbody = tbody_match.group(1) + assert tbody.index("Beta") < tbody.index("Alpha") # 90 before 10 + + def test_name_aggregate_sort_no_duplicate_rows(self, logged_client, two_purchases): + # a multi-game purchase must still render exactly one row + cheap, _ = two_purchases + extra = Game.objects.create( + name="Aaa", sort_name="Aaa", platform=cheap.platform + ) + cheap.games.add(extra) + response = logged_client.get(reverse("games:list_purchases"), {"sort": "name"}) + body = response.content.decode() + assert body.count("purchase-row-") == 2 # exactly two purchase rows + + def test_unknown_sort_emits_warning(self, logged_client, two_purchases): + response = logged_client.get(reverse("games:list_purchases"), {"sort": "nope"}) + warnings = [str(m) for m in get_messages(response.wsgi_request)] + assert any("nope" in w for w in warnings) + + +class TestDefaultOrderUnchanged: + """The default sort strings must reproduce the pre-#68 hardcoded order.""" + + def test_games_default_is_created_descending(self, logged_client, two_games): + alpha, beta = two_games # beta newer + response = logged_client.get(reverse("games:list_games")) + body = response.content.decode() + tbody_match = re.search(r"]*>(.*?)", body, re.DOTALL) + assert tbody_match + tbody = tbody_match.group(1) + assert tbody.index("Beta") < tbody.index("Alpha") + + +class TestEverySortKeyReturns200: + def test_all_game_keys(self, logged_client, two_games): + for key in GAME_SORTS: + for raw in (key, f"-{key}"): + response = logged_client.get(reverse("games:list_games"), {"sort": raw}) + assert response.status_code == 200, raw + + def test_all_session_keys(self, logged_client, two_games): + for key in SESSION_SORTS: + response = logged_client.get(reverse("games:list_sessions"), {"sort": key}) + assert response.status_code == 200, key + + def test_all_purchase_keys(self, logged_client, two_games): + for key in PURCHASE_SORTS: + response = logged_client.get(reverse("games:list_purchases"), {"sort": key}) + assert response.status_code == 200, key