From e6553d0c91bed6aa7f935021bb03b6fbc28b7e81 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Sun, 30 Aug 2026 12:32:56 -0700 Subject: [PATCH 1/4] Merge the Alternate Number sort into the Alternate Series sort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One sort key now does both halves: comics group by their alternate series (ComicInfo AlternateSeries / MetronInfo reprints) and order by the parsed AlternateNumber inside the group, falling back to the comic's own series and issue so untagged comics interleave. A correlated subquery elects one reprint per comic by natural tuple order and ORDER BY is a five-field Coalesce list reading from that election — numbers compare numerically, and the shared election ordering keeps the tuple atomic (the old sort aggregated number and suffix independently and could pair parts from different reprints). An active reprints filter narrows the election instead of using a FilteredRelation. Benchmarked at parity with a join+aggregate at 120k comics; the fallback series segment uses lower(name) so article-led titles interleave with their alternate namesakes. Collection rows sort by a matching fixed-width key in the intersection RawSQL, with an in-SQL fallback to the collection's own name. The key reads only reprint columns (a per-comic fallback under GROUP BY came back from an arbitrary row) and its printf formats are %%-escaped so the DEBUG query logger doesn't consume bound params. alternate_number is removed from the order-by choices (0053 edited in place, unreleased). New 0054 remaps stored settings in order_by, order_extra_keys and collection_order_memory with dedupe; sidecar restore gets a sort-key rename map covering the same three homes. Cover-pick subqueries for the merged key normalize to sort_name like every other M2M sort. Co-Authored-By: Claude Fable 5 --- NEWS.md | 13 +- codex/choices/browser.py | 9 +- ...ssue_number_and_collection_order_memory.py | 9 +- .../0054_merge_alternate_number_sort.py | 102 +++++++++ codex/models/named.py | 2 +- codex/user_data/restore.py | 60 ++++- codex/views/browser/annotate/order.py | 44 +--- codex/views/browser/browser.py | 14 +- codex/views/browser/columns.py | 167 +++++++++++--- codex/views/browser/intersections.py | 94 ++++++-- codex/views/browser/order_by.py | 27 +-- .../browser/card/order-by-caption.vue | 11 - frontend/tests/unit/order-by-caption.test.js | 25 +-- tests/test_alternate_series_sort_remap.py | 130 +++++++++++ tests/test_browser_ordering.py | 1 - tests/test_browser_reprints_column.py | 208 ++++++++++++++---- tests/test_user_data_restore.py | 47 ++++ 17 files changed, 744 insertions(+), 219 deletions(-) create mode 100644 codex/migrations/0054_merge_alternate_number_sort.py create mode 100644 tests/test_alternate_series_sort_remap.py diff --git a/NEWS.md b/NEWS.md index c6ddd8d52..6fd5d2e31 100644 --- a/NEWS.md +++ b/NEWS.md @@ -14,12 +14,13 @@ border-radius: 128px; them keeps each one's sort instead of carrying one sort everywhere. A collection you haven't sorted yet keeps whatever sort you arrive with. - Clearing a search puts back the sort the search replaced. - - Sort by Alternate Number, the issue number a comic carries inside its - alternate series (ComicInfo AlternateSeries / AlternateNumber). Filter by - an alternate series first to pick which one to sort by. Comics with no - alternate number fall back to their own issue number. - - The Alternate Series sort is available in cover view, not just the table. - Comics with no alternate series sort by their real series name. + - Sort by Alternate Series (ComicInfo AlternateSeries / AlternateNumber). + Comics group by their alternate series and order by its issue number + inside that group. Filter by an alternate series first to sort by that one + instead of the first one alphabetically. Comics with no alternate series + sort by their real series and issue, so they interleave rather than + clumping, and an alternate series with no issue numbers keeps its comics + in their own issue order. Available in cover view as well as the table. - Read an alternate series as a reading order: pick it in the reader's reading-order menu and next/prev follow the alternate numbering. Handy for using alternate series tags as durable reading lists. diff --git a/codex/choices/browser.py b/codex/choices/browser.py index 81fe70c29..011af59dd 100644 --- a/codex/choices/browser.py +++ b/codex/choices/browser.py @@ -18,7 +18,6 @@ { "created_at": "Added Time", "age_rating": "Age Rating", - "alternate_number": "Alternate Number", "reprints": "Alternate Series", "characters": "Characters", "child_count": "Child Count", @@ -76,7 +75,6 @@ { "created_at", "age_rating", - "alternate_number", "reprints", "child_count", "community_rating", @@ -97,15 +95,12 @@ # They sort fine as the primary, but the per-extra annotation # pipeline can't safely produce a value for them on every model # / context: ``story_arc_number`` requires StoryArc-context ``pks`` -# to resolve which arc's number to pick, ``alternate_number`` likewise -# needs the ``reprints`` filter to resolve which alternate series' -# number to pick, and ``search_score``'s ``ComicFTSRank`` only -# resolves when an FTS subquery is joined. +# to resolve which arc's number to pick, and ``search_score``'s +# ``ComicFTSRank`` only resolves when an FTS subquery is joined. # Mirrored on the frontend so the table headers can grey out the # affected columns and refuse the shift-click. BROWSER_EXTRA_SORT_UNSUPPORTED_KEYS = frozenset( { - "alternate_number", "story_arc_number", "search_score", } diff --git a/codex/migrations/0053_reprint_issue_number_and_collection_order_memory.py b/codex/migrations/0053_reprint_issue_number_and_collection_order_memory.py index 1b267b874..bc01c8b67 100644 --- a/codex/migrations/0053_reprint_issue_number_and_collection_order_memory.py +++ b/codex/migrations/0053_reprint_issue_number_and_collection_order_memory.py @@ -1,10 +1,10 @@ """ Split Reprint.issue into sortable columns & remember sorts per collection. -The ``alternate_number`` order_by key sorts comics by their -ComicInfo ``AlternateNumber`` within an alternate series. ``issue`` -is a string, so sorting it directly puts "#10" before "#2"; these -derived columns mirror ``Comic.issue_number`` / ``issue_suffix``. +The Alternate Series order_by key sorts comics by their ComicInfo +``AlternateNumber`` within an alternate series. ``issue`` is a string, +so sorting it directly puts "#10" before "#2"; these derived columns +mirror ``Comic.issue_number`` / ``issue_suffix``. ``SettingsBrowser.collection_order_memory`` remembers the sort each top collection was last browsed with. Switching top collections used to drag one @@ -73,7 +73,6 @@ class Migration(migrations.Migration): choices=[ ("created_at", "Added Time"), ("age_rating", "Age Rating"), - ("alternate_number", "Alternate Number"), ("reprints", "Alternate Series"), ("characters", "Characters"), ("child_count", "Child Count"), diff --git a/codex/migrations/0054_merge_alternate_number_sort.py b/codex/migrations/0054_merge_alternate_number_sort.py new file mode 100644 index 000000000..5516c58cf --- /dev/null +++ b/codex/migrations/0054_merge_alternate_number_sort.py @@ -0,0 +1,102 @@ +""" +Merge the Alternate Number sort into Alternate Series. + +The two sorts each did half the job — ``reprints`` ordered by a display +label (so "#10" preceded "#2") and ``alternate_number`` ordered by the +parsed number but ignored which alternate series it belonged to. They +are now one key, ``reprints``. + +0053 dropped ``alternate_number`` from the field's choices, but choices +aren't enforced by SQLite and 0053 has already been applied on every +database that could hold the value, so the stored settings need this +separate pass. Left in place, a stale key reaches ORDER BY unvalidated +(settings load raw) and raises FieldError. + +The key lives in three places on SettingsBrowser — the ``order_by`` +column, the ``order_extra_keys`` list, and the per-top-collection +``collection_order_memory`` map — and saved views are more rows in the +same table, so every row is remapped regardless of name or client. +""" + +from django.db import migrations + +_OLD_KEY = "alternate_number" +_NEW_KEY = "reprints" + + +def _remap_extra_keys(entries) -> tuple[list, bool]: + """Remap the sort key in an extras list, dropping a duplicate.""" + if not isinstance(entries, list): + return entries, False + remapped: list = [] + changed = False + seen: set = set() + for entry in entries: + key = entry.get("key") if isinstance(entry, dict) else None + if key == _OLD_KEY: + entry = {**entry, "key": _NEW_KEY} # noqa: PLW2901 + key = _NEW_KEY + changed = True + if key in seen: + # The row already sorted by the surviving key. Two entries + # for one column is not a state the sort can express, so the + # first occurrence wins, as the serializer's own extras + # cleaner does. + changed = True + continue + seen.add(key) + remapped.append(entry) + return remapped, changed + + +def _remap_memory(memory) -> tuple[dict, bool]: + """Remap the sort key inside a collection_order_memory map.""" + if not isinstance(memory, dict): + return memory, False + changed = False + for remembered in memory.values(): + if not isinstance(remembered, dict): + continue + if remembered.get("order_by") == _OLD_KEY: + remembered["order_by"] = _NEW_KEY + changed = True + extras, extras_changed = _remap_extra_keys(remembered.get("order_extra_keys")) + if extras_changed: + remembered["order_extra_keys"] = extras + changed = True + return memory, changed + + +def _remap_browser_settings(apps, _schema_editor) -> None: + settings_browser = apps.get_model("codex", "SettingsBrowser") + settings_browser.objects.filter(order_by=_OLD_KEY).update(order_by=_NEW_KEY) + # JSON payloads need a python pass; JSONField key lookups are + # unsupported on SQLite, so scan and rewrite sparsely. + rows = [] + for row in settings_browser.objects.only( + "pk", "order_extra_keys", "collection_order_memory" + ): + extras, extras_changed = _remap_extra_keys(row.order_extra_keys) + memory, memory_changed = _remap_memory(row.collection_order_memory) + if extras_changed or memory_changed: + row.order_extra_keys = extras + row.collection_order_memory = memory + rows.append(row) + if rows: + settings_browser.objects.bulk_update( + rows, ["order_extra_keys", "collection_order_memory"] + ) + + +class Migration(migrations.Migration): + """Remap the retired alternate_number sort key onto reprints.""" + + dependencies = [ + ("codex", "0053_reprint_issue_number_and_collection_order_memory"), + ] + + operations = [ + # Irreversible by design: both former keys map onto ``reprints``, + # so a reverse pass can't know which rows to send back. + migrations.RunPython(_remap_browser_settings, migrations.RunPython.noop), + ] diff --git a/codex/models/named.py b/codex/models/named.py index 12bf58cf2..5aadde84c 100644 --- a/codex/models/named.py +++ b/codex/models/named.py @@ -142,7 +142,7 @@ class Reprint(BaseModel): identifier = ForeignKey(Identifier, on_delete=SET_NULL, null=True) # ``issue`` split into its sortable parts, mirroring # ``Comic.issue_number`` / ``issue_suffix``. Without them the - # ``alternate_number`` sort would order "#10" before "#2". Derived + # Alternate Series sort would order "#10" before "#2". Derived # in ``presave``, never imported directly; unindexed because they're # only read after an indexed join on pk or series_name. issue_number = CoercingDecimalField(decimal_places=2, max_digits=10, null=True) diff --git a/codex/user_data/restore.py b/codex/user_data/restore.py index 9ca05c9c6..d18d2c323 100644 --- a/codex/user_data/restore.py +++ b/codex/user_data/restore.py @@ -553,8 +553,19 @@ def _restore_settings_browser( # Filter/sort keys renamed across codex versions; sidecar backups from # older versions still carry the old name (0048: critical -> community). +# This map also resolves *filter* columns by their legacy name, so only +# renames that applied to a filter column belong here. _LEGACY_KEY_RENAMES: Final[dict[str, str]] = {"critical_rating": "community_rating"} +# Sort keys retired into another key. Sort-only, because there was never +# an ``alternate_number`` filter column for ``_resolve_filter_column`` +# to look for (0054: the Alternate Number sort merged into Alternate +# Series). +_SORT_KEY_RENAMES: Final[dict[str, str]] = { + **_LEGACY_KEY_RENAMES, + "alternate_number": "reprints", +} + def _resolve_filter_column(row_keys, column: str) -> str | None: """Sidecar column holding ``column``'s value: itself, its legacy name, or None.""" @@ -577,10 +588,49 @@ def _row_column(row, column: str): return None +def _rename_sort_key(key) -> str: + """Rename one retired sort key. Sidecar JSON can hold anything.""" + key = key if isinstance(key, str) else "" + return _SORT_KEY_RENAMES.get(key, key) + + +def _rename_extra_keys(entries) -> list: + """Rename retired sort keys in an extras list, dropping duplicates.""" + if not isinstance(entries, list): + return [] + renamed: list = [] + seen: set[str] = set() + for entry in entries: + if not isinstance(entry, dict): + continue + key = _rename_sort_key(entry.get("key")) + if key in seen: + # Both a retired key and its replacement were stored; one + # column can only carry one sort, so the first one wins. + continue + seen.add(key) + renamed.append({**entry, "key": key}) + return renamed + + +def _rename_memory_keys(memory) -> dict: + """Rename retired sort keys inside a collection_order_memory map.""" + if not isinstance(memory, dict): + return {} + for remembered in memory.values(): + if not isinstance(remembered, dict): + continue + remembered["order_by"] = _rename_sort_key(remembered.get("order_by")) + remembered["order_extra_keys"] = _rename_extra_keys( + remembered.get("order_extra_keys") + ) + return memory + + def _build_browser_defaults(row, show) -> dict[str, Any]: """Map a sidecar settings_browser row to ``update_or_create`` defaults.""" order_by = row["order_by"] or "" - order_by = _LEGACY_KEY_RENAMES.get(order_by, order_by) + order_by = _SORT_KEY_RENAMES.get(order_by, order_by) table_columns = json.loads(row["table_columns"] or "{}") for old, new in _LEGACY_KEY_RENAMES.items(): if old in table_columns: @@ -590,9 +640,11 @@ def _build_browser_defaults(row, show) -> dict[str, Any]: "top_collection": row["top_collection"] or "", "order_by": order_by, "order_reverse": bool(row["order_reverse"]), - "order_extra_keys": json.loads(row["order_extra_keys"] or "[]"), - "collection_order_memory": json.loads( - _row_column(row, "collection_order_memory") or "{}" + "order_extra_keys": _rename_extra_keys( + json.loads(row["order_extra_keys"] or "[]") + ), + "collection_order_memory": _rename_memory_keys( + json.loads(_row_column(row, "collection_order_memory") or "{}") ), "search": row["search"] or "", "custom_covers": bool(row["custom_covers"]), diff --git a/codex/views/browser/annotate/order.py b/codex/views/browser/annotate/order.py index 063ba16c4..a7076d40e 100644 --- a/codex/views/browser/annotate/order.py +++ b/codex/views/browser/annotate/order.py @@ -12,7 +12,7 @@ ) from django.db.models.aggregates import Avg, Count, Max, Min, Sum from django.db.models.fields import CharField -from django.db.models.functions import Coalesce, Reverse, Right, StrIndex +from django.db.models.functions import Reverse, Right, StrIndex from codex.choices.browser import BROWSER_EXTRA_SORT_UNSUPPORTED_KEYS from codex.models import ( @@ -84,7 +84,6 @@ _ANNOTATED_ORDER_FIELDS = frozenset( # These are annotated with their own functions { - "alternate_number", "bookmark_updated_at", "child_count", "favorite", @@ -227,46 +226,6 @@ def _alias_story_arc_number(self, qs): return qs.alias(story_arc_number=story_arc_number) - def _alias_alternate_number(self, qs): - """Alias the alternate series issue number & suffix for ordering.""" - if self.order_key != "alternate_number": - return qs - - # Unlike ``story_arc_number`` there's no alternate series browse - # collection, so the ``reprints`` filter is the only thing that can - # say *which* alternate series' number to sort by. - reprint_pks = self.params.get("filters", {}).get("reprints", ()) - # ``self.rel_prefix`` is memoized off the *view's* model, but this - # runs for the book queryset too — take the prefix from the - # queryset being annotated, as ``_alias_story_arc_number`` does. - rel_prefix = self.get_rel_prefix(qs.model) - own_number = rel_prefix + "issue_number" - own_suffix = rel_prefix + "issue_suffix" - - if reprint_pks: - rel = rel_prefix + "reprints" - condition = Q(**{f"{rel}__pk__in": reprint_pks}) - qs = qs.alias(selected_reprint=FilteredRelation(rel, condition=condition)) - # Comics carrying no alternate number fall back to their own - # issue number so a mixed listing stays readable instead of - # collapsing every untagged comic to NULL. Coalescing *inside* - # the aggregate lets each comic contribute its own effective - # value; coalescing outside would compare one series' minimum - # alternate number against another's minimum issue number. - number = self.order_agg_func( - Coalesce("selected_reprint__issue_number", own_number) - ) - suffix = self.order_agg_func( - Coalesce("selected_reprint__issue_suffix", own_suffix) - ) - else: - # No alternate series selected: degrade to the plain issue sort - # rather than ordering everything by NULL. - number = self.order_agg_func(own_number) - suffix = self.order_agg_func(own_suffix) - - return qs.alias(alternate_number=number, alternate_number_suffix=suffix) - def _annotate_page_count(self, qs): """Hoist up total page_count of children.""" # Used for sorting and progress @@ -604,7 +563,6 @@ def annotate_order_aggregates(self, qs: QuerySet, *, for_cover: bool = False): qs = self._alias_sort_names(qs) qs = self._alias_filename(qs) qs = self._alias_story_arc_number(qs) - qs = self._alias_alternate_number(qs) if not for_cover: qs = self._annotate_page_count(qs) qs = self._annotate_bookmark_updated_at(qs) diff --git a/codex/views/browser/browser.py b/codex/views/browser/browser.py index 3a0d5f57c..9fddc3fa7 100644 --- a/codex/views/browser/browser.py +++ b/codex/views/browser/browser.py @@ -192,16 +192,20 @@ def _add_table_view_sort_annotations(self, qs): return qs fk_anns = fk_name_annotations_for(sort_keys) m2m_anns = m2m_annotations_for(sort_keys) - # Every key that sorts through a fallback alias needs it annotated, - # extras included — ``_comic_extra_fields`` resolves an extra to the - # same alias the primary uses. - m2m_sort_anns = m2m_sort_annotations_for(sort_keys) + # Every key that sorts through elected-value aliases needs them + # annotated, extras included — ``_comic_extra_fields`` resolves an + # extra to the same aliases the primary uses. An active reprints + # filter names which alternate series the user is looking at, so + # the election is narrowed to it instead of picking the + # alphabetically first one. + reprint_pks = self.params.get("filters", {}).get("reprints", ()) + m2m_sort_anns = m2m_sort_annotations_for(sort_keys, reprint_pks) if fk_anns: qs = qs.annotate(**fk_anns) if m2m_anns: qs = qs.annotate(**m2m_anns) if m2m_sort_anns: - qs = qs.annotate(**m2m_sort_anns) + qs = qs.alias(**m2m_sort_anns) return qs def _add_table_view_display_annotations(self, qs): diff --git a/codex/views/browser/columns.py b/codex/views/browser/columns.py index d5a9a80de..860818e6f 100644 --- a/codex/views/browser/columns.py +++ b/codex/views/browser/columns.py @@ -14,13 +14,15 @@ BooleanField, Case, F, + IntegerField, + OuterRef, Q, + Subquery, Value, When, ) -from django.db.models.aggregates import Min from django.db.models.fields import CharField -from django.db.models.functions import Cast, Coalesce, Concat +from django.db.models.functions import Cast, Coalesce, Concat, Lower from codex.choices.browser import ( BROWSER_TABLE_COLUMNS, @@ -29,6 +31,7 @@ from codex.models.collections import Volume from codex.models.favorite import FAVORITE_MODEL_COLLECTIONS, Favorite from codex.models.functions import JsonGroupArray +from codex.models.named import Reprint # ORM paths or expressions for M2M columns. Simple ones map a single # ``related__name`` path; ``credits`` and ``identifiers`` need composite @@ -104,6 +107,111 @@ output_field=CharField(), ) +# The Alternate Series sort: a field list like the Name sort's, with +# each part preferring the alternate value. ``reprints`` is M2M — a +# comic can be in several alternate series — so each ORDER BY column +# must first collapse to one value per comic, and collapsing columns +# independently would pair one reprint's series with another's issue +# number. Instead a correlated subquery *elects* one reprint per comic +# by the natural tuple order, and every column reads its field from +# that election. All five subqueries share one election ordering, so +# any two reprint rows that tie on it carry identical values for every +# elected field — the tuple stays atomic without a composed key. +# +# ``Lower`` runs inside the election rather than collating at ORDER BY +# time: SQLite drops a column's NOCASE collation inside expressions, so +# the election would otherwise pick its winner case-sensitively while +# the ORDER BY compared case-insensitively. +# +# The election is one covering-index probe per comic (benchmarked at +# parity with a join + aggregate at 120k comics), and reprint tags are +# sparse in practice. +_ELECTION_ORDER = ( + "_series_key", + "volume_number", + "_language_key", + "_number_key", + "_suffix_key", +) +# Aliases live in the M2M sort namespace; the tuple order is ORDER BY +# order, mirroring the election ordering above. +_M2M_SORT_ANNOTATION_PREFIX = "_table_m2m_sort_" +_REPRINTS_SORT_ALIASES = tuple( + _M2M_SORT_ANNOTATION_PREFIX + "reprints_" + part + for part in ("series", "volume", "language", "number", "suffix") +) + + +def _elected_reprint_value(value: str, reprint_pks: tuple | list = ()) -> Subquery: + """Read one annotation off the outer comic's elected reprint.""" + election = Reprint.objects.filter(comic=OuterRef("pk"), series_name__gt="") + if reprint_pks: + # A reprints filter names which alternate series the user is + # looking at; elect among those instead of alphabetically. + election = election.filter(pk__in=reprint_pks) + election = election.annotate( + _series_key=Lower("series_name"), + _language_key=Lower("language"), + # An alternate series entry with no issue at all offers nothing + # to sort by; null both parts so the outer Coalesce falls back + # to the comic's own issue *jointly*, never half-and-half. + _no_issue=Case( + When(Q(issue_number__isnull=True) & Q(issue_suffix=""), then=Value(1)), + default=Value(0), + output_field=IntegerField(), + ), + _number_key=Case( + When(_no_issue=1, then=Value(None)), + default="issue_number", + ), + _suffix_key=Case( + When(_no_issue=1, then=Value(None)), + default=Lower("issue_suffix"), + output_field=CharField(), + ), + ).order_by(*_ELECTION_ORDER) + return Subquery(election.values(value)[:1]) + + +def reprints_sort_annotations(reprint_pks: tuple | list = ()) -> dict: + """ + Build the ordered ORDER BY aliases for the Alternate Series sort. + + Alternate series identity (name, volume, language — ``Reprint``'s + unique key minus the issue) leads so every issue of one alternate + series groups together, then its parsed issue number and suffix + order the group. A comic with no alternate series falls back to its + own series and issue so mixed listings interleave. The fallback + series segment is ``Lower(name)`` rather than ``sort_name`` — + alternate series names sort by their raw name, so the article-moved + ``sort_name`` would put "The Batman" and its untagged siblings at + opposite ends of the listing. + + Comic querysets only; collection rows sort by the raw-SQL mirror in + ``intersections._build_reprints_intersection_sort_sql``. + """ + expressions = ( + Coalesce( + _elected_reprint_value("_series_key", reprint_pks), + Lower("series__name"), + ), + Coalesce( + _elected_reprint_value("volume_number", reprint_pks), + F("volume__name"), + ), + _elected_reprint_value("_language_key", reprint_pks), + Coalesce( + _elected_reprint_value("_number_key", reprint_pks), + F("issue_number"), + ), + Coalesce( + _elected_reprint_value("_suffix_key", reprint_pks), + Lower("issue_suffix"), + ), + ) + return dict(zip(_REPRINTS_SORT_ALIASES, expressions, strict=True)) + + _M2M_COLUMN_PATHS = MappingProxyType( { "characters": "characters__name", @@ -279,50 +387,37 @@ def m2m_columns() -> frozenset[str]: return frozenset(_M2M_COLUMN_PATHS.keys()) -# M2M sort keys that have a meaningful scalar counterpart on Comic to -# fall back to. Sorting by the JSON aggregate alone parks every comic -# lacking the relation in one undifferentiated clump; ``reprints`` is -# an alternate *series* name, so a comic without one sorts by its real -# series instead and the listing stays readable. Other M2M columns -# (genres, tags, …) have no such counterpart and keep the plain +# M2M sort keys that sort through elected-value aliases instead of the +# plain display aggregate. ``reprints`` is the only one: an alternate +# series has a series name *and* an issue number, so sorting it by the +# display label would order "#10" before "#2", and a comic carrying no +# alternate series would park in one undifferentiated clump. Other M2M +# columns (genres, tags, …) have no such structure and keep the # aggregate sort. -_M2M_SORT_FALLBACK_PATHS = MappingProxyType({"reprints": "series__sort_name"}) -_M2M_SORT_ANNOTATION_PREFIX = "_table_m2m_sort_" -def m2m_sort_alias_for(column_key: str) -> str: - """Return the ORDER BY alias for an M2M column with a scalar fallback.""" - return _M2M_SORT_ANNOTATION_PREFIX + column_key +def m2m_sort_columns() -> frozenset[str]: + """Return M2M column keys that sort through elected-value aliases.""" + return frozenset({"reprints"}) -def m2m_sort_columns() -> frozenset[str]: - """Return M2M column keys that sort through a fallback alias.""" - return frozenset(_M2M_SORT_FALLBACK_PATHS.keys()) +def m2m_sort_order_fields(column_key: str) -> tuple[str, ...]: + """Return the ORDER BY alias list for an elected-value M2M sort key.""" + return _REPRINTS_SORT_ALIASES if column_key == "reprints" else () -def m2m_sort_annotations_for(columns: tuple[str, ...]) -> dict[str, Coalesce]: +def m2m_sort_annotations_for( + columns: tuple[str, ...], reprint_pks: tuple | list = () +) -> dict: """ - Build ``alias -> Coalesce(Min(expr), fallback)`` sort annotations. + Build the elected-value sort aliases for the requested columns. - Separate from the display aggregate: the cell still renders the - full JSON list, while ORDER BY uses the first label so a comic - with no relation can fall through to its scalar counterpart. + Separate from the display aggregate: the cell still renders the full + JSON list of labels, while ORDER BY compares the elected fields. """ - annotations: dict[str, Coalesce] = {} - for col in columns: - fallback = _M2M_SORT_FALLBACK_PATHS.get(col) - path = _M2M_COLUMN_PATHS.get(col) - if fallback is None or path is None: - continue - agg_filter = _M2M_AGGREGATE_FILTERS.get(col) - kwargs: dict = {"filter": agg_filter} if agg_filter is not None else {} - # The composed label and the fallback column are both text but - # different field classes (``CharField`` vs ``CleaningCharField``), - # which Django refuses to unify on its own. - annotations[m2m_sort_alias_for(col)] = Coalesce( - Min(path, **kwargs), F(fallback), output_field=CharField() - ) - return annotations + if "reprints" not in columns: + return {} + return reprints_sort_annotations(reprint_pks) # FK-name annotations live in their own alias namespace so they don't diff --git a/codex/views/browser/intersections.py b/codex/views/browser/intersections.py index c724d6f8e..c66b0fc34 100644 --- a/codex/views/browser/intersections.py +++ b/codex/views/browser/intersections.py @@ -37,7 +37,6 @@ ) from codex.models.named import Reprint from codex.views.browser.columns import ( - VOLUME_YEAR_RANGE, fk_name_columns, m2m_columns, ) @@ -761,36 +760,91 @@ def _build_identifiers_intersection_sort_sql( return _IntersectionSortRawSQL(sql, []) +# SQL spellings of the reprint sort key's pieces. printf renders each +# number at a fixed width so "#2" collates before "#10" as text; widths +# come from the fields (issue: Decimal(10,2) → 8 digits + "." + 2; +# volume: PositiveSmallInteger → 5 digits). NULL numbers render as +# same-width runs of spaces (0x20 < "0" → NULLs first), because +# printf renders NULL as zero rather than propagating it. The ``%`` +# signs are doubled: Django's SQLite cursor collapses ``%%`` back to +# ``%`` at execute time, and the DEBUG query logger runs ``sql % +# params`` over the raw string — a bare ``%011.2f`` would consume a +# parameter there and crash every logged query. +_SEP_SQL = "X'1F'" +_ISSUE_FMT_SQL = "%%011.2f" +_NULL_ISSUE_SQL = "'" + " " * 11 + "'" +_VOLUME_FMT_SQL = "%%05d" +_NULL_VOLUME_SQL = "'" + " " * 5 + "'" + + +def _issue_key_sql(alias: str) -> str: + """Render a table's issue number + suffix as one fixed-width segment.""" + return ( + f"CASE WHEN {alias}.issue_number IS NULL THEN {_NULL_ISSUE_SQL} " + f"ELSE printf('{_ISSUE_FMT_SQL}', {alias}.issue_number) END " + f"|| lower({alias}.issue_suffix)" + ) + + +def _collection_own_sort_sql(collection_model: type[BrowserCollectionModel]) -> str: + """Return the collection's own name as a sort string, for fallbacks.""" + table = collection_model._meta.db_table + if collection_model is Volume: + # Volume has no sort_name; ``name`` is a nullable integer, so + # render it fixed-width like the key's own volume segment. + return f"""CASE + WHEN "{table}"."name" IS NULL THEN '' + ELSE printf('{_VOLUME_FMT_SQL}', "{table}"."name") + END""" + return f'lower("{table}"."sort_name")' + + def _build_reprints_intersection_sort_sql( collection_model: type[BrowserCollectionModel], ) -> RawSQL | None: - """Reprints render the same composed label the table cell shows.""" + """Reprints sort by the same key ordering the Comic rows use.""" correlation = _comic_correlation_sql(collection_model) if correlation is None: return None - # Mirror of ``Reprint.compose_name``: only the columns the reprint - # carries contribute, and a four-digit volume number is a year. The - # year bounds are the only bound parameters any of these - # intersection subqueries take. - inner = """ + # One composed key per shared reprint, mirroring the field order of + # ``codex.views.browser.columns.reprints_sort_annotations``: + # alternate series identity (name, volume, language) then the issue + # rendered at fixed width so "#2" collates before "#10". The key + # reads ONLY reprint columns — the envelope selects ``display_name`` + # as a bare column under ``GROUP BY target_id``, so anything read + # from the joined comic row would come back from an arbitrary child. + # Placeholder reprints render '' so the envelope's + # ``display_name != ''`` test drops them, matching the election + # filter on the Comic-row annotation. + # + # Everything spliced in is a module constant derived from the field + # definitions, never user input. + inner = f""" SELECT r.id AS target_id, - r.series_name - || CASE - WHEN r.volume_number IS NULL THEN '' - WHEN r.volume_number BETWEEN %s AND %s - THEN ' (' || r.volume_number || ')' - ELSE ' v' || r.volume_number - END - || CASE WHEN r.issue = '' THEN '' ELSE ' #' || r.issue END - || CASE WHEN r.language = '' THEN '' ELSE ' (' || r.language || ')' END - AS display_name + CASE WHEN r.series_name = '' THEN '' ELSE + lower(r.series_name) + || {_SEP_SQL} + || CASE + WHEN r.volume_number IS NULL THEN {_NULL_VOLUME_SQL} + ELSE printf('{_VOLUME_FMT_SQL}', r.volume_number) + END + || {_SEP_SQL} || lower(r.language) || {_SEP_SQL} + || {_issue_key_sql("r")} + END AS display_name FROM codex_reprint r INNER JOIN codex_comic_reprints th ON th.reprint_id = r.id INNER JOIN codex_comic c ON c.id = th.comic_id - """ - sql = _wrap_intersection_sort(inner, correlation) - return _IntersectionSortRawSQL(sql, list(VOLUME_YEAR_RANGE)) + """ # noqa: S608 + envelope = _wrap_intersection_sort(inner, correlation) + # The fallback has to be spliced into the raw string: wrapping the + # RawSQL in a Django ``Coalesce`` would restore the correlated + # subquery to the GROUP BY that ``_IntersectionSortRawSQL`` exists + # to keep it out of. An empty intersection (children share no + # alternate series, or disagree) sorts by the collection's own name. + own = _collection_own_sort_sql(collection_model) + sql = f"COALESCE(NULLIF({envelope}, ''), {own})" + return _IntersectionSortRawSQL(sql, []) def _build_story_arcs_intersection_sort_sql( diff --git a/codex/views/browser/order_by.py b/codex/views/browser/order_by.py index ea3259ef8..8230ca665 100644 --- a/codex/views/browser/order_by.py +++ b/codex/views/browser/order_by.py @@ -9,8 +9,8 @@ from codex.views.browser.columns import ( m2m_alias_for, m2m_columns, - m2m_sort_alias_for, m2m_sort_columns, + m2m_sort_order_fields, ) # Order keys that don't map directly to a Comic field name need an @@ -129,19 +129,14 @@ def _comic_order_fields_head(self, order_key: str, comic_sort_names) -> list: # natural multi-field sort that matches how the compound # ``Issue`` table column is rendered. return ["issue_number", "issue_suffix"] - if order_key == "alternate_number": - # The same compound expansion over the alternate series' - # number, whose parts are annotated aliases rather than - # columns (see ``_alias_alternate_number``). ``date`` breaks - # ties between comics sharing an alternate number, matching - # the ``story_arc_number`` tail. - return ["alternate_number", "alternate_number_suffix", "date"] if order_key in m2m_sort_columns(): - # M2M sort with a scalar counterpart (``reprints`` → - # ``series__sort_name``): order on the fallback alias so - # comics carrying no alternate series interleave by their - # real series instead of clumping under an empty list. - return [m2m_sort_alias_for(order_key)] + # M2M sort through elected-value aliases (``reprints``): a + # field list like the ``sort_name`` head, each part read + # from the comic's elected alternate series with the + # comic's own series and issue as the fallback, so comics + # carrying no alternate series interleave by their real + # series instead of clumping under an empty list. + return list(m2m_sort_order_fields(order_key)) if order_key in m2m_columns(): # M2M sort: ``ORDER BY `` where the alias is the # JsonGroupArray annotation added by the table-view path. @@ -229,12 +224,6 @@ def add_order_by( # ``_age_rating_sort_value`` is the metron index (sort). # See ``BrowserAnnotateOrderView.annotate_order_value``. order_fields_head = ["_age_rating_sort_value"] - elif self.order_key == "alternate_number": - # ``order_value`` carries the aggregated alternate number - # (and the card caption renders it); the parallel suffix - # alias is the secondary, mirroring the Comic-row compound - # expansion in ``_comic_order_fields_head``. - order_fields_head = ["order_value", "alternate_number_suffix"] else: order_fields_head = ["order_value"] diff --git a/frontend/src/components/browser/card/order-by-caption.vue b/frontend/src/components/browser/card/order-by-caption.vue index ed32fcdfa..3a74830ca 100644 --- a/frontend/src/components/browser/card/order-by-caption.vue +++ b/frontend/src/components/browser/card/order-by-caption.vue @@ -74,8 +74,6 @@ export default { return prettyBytes(Number.parseInt(ov, 10)); } else if (STAR_SORT_BY.has(this.orderBy)) { return `★ ${this.formatStarRating(ov)}`; - } else if (this.orderBy === "alternate_number") { - return this.formatAlternateNumber(ov); } else if (this.orderBy === "reprints") { return this.formatReprints(ov); } @@ -106,15 +104,6 @@ export default { if (!Number.isFinite(n)) return ov; return n.toFixed(2).replace(/\.?0+$/, ""); }, - /* - * The alternate issue number is a DecimalField aggregate, so it - * arrives as "2.00". Show "#2" the way the issue column does. - */ - formatAlternateNumber(ov) { - const n = Number.parseFloat(ov); - if (!Number.isFinite(n)) return ov; - return `#${n.toFixed(2).replace(/\.?0+$/, "")}`; - }, /* * The alternate series order_value is the JSON array the table cell * renders. Collection rows sort by a fallback the caption can't diff --git a/frontend/tests/unit/order-by-caption.test.js b/frontend/tests/unit/order-by-caption.test.js index a46d47c7e..9a7d85ec4 100644 --- a/frontend/tests/unit/order-by-caption.test.js +++ b/frontend/tests/unit/order-by-caption.test.js @@ -2,11 +2,10 @@ * Tests for the browser card's order-by caption. * * Behavior locked in here: - * - "alternate_number" is a DecimalField aggregate ("2.00") and renders - * as "#2", not "#2.00". - * - "reprints" order_value is the JSON array the table cell renders; - * comic cards join the labels, collection cards show nothing because - * they sort by a fallback the caption can't represent. + * - "reprints" order_value is the JSON array the table cell renders, + * never the composed key the rows are sorted by; comic cards join + * the labels, collection cards show nothing because they sort by a + * fallback the caption can't represent. */ import { createTestingPinia } from "@pinia/testing"; import { mount } from "@vue/test-utils"; @@ -26,22 +25,6 @@ function mountCaption(orderBy, item) { } describe("order by caption", () => { - test("alternate number trims the decimal aggregate", () => { - const wrapper = mountCaption("alternate_number", { - orderValue: "2.00", - collection: "comics", - }); - expect(wrapper.text()).toBe("#2"); - }); - - test("alternate number keeps a real fraction", () => { - const wrapper = mountCaption("alternate_number", { - orderValue: "1.50", - collection: "comics", - }); - expect(wrapper.text()).toBe("#1.5"); - }); - test("alternate series joins the label list on a comic card", () => { const wrapper = mountCaption("reprints", { orderValue: JSON.stringify(["Crossover v2", "Otra Serie (es)"]), diff --git a/tests/test_alternate_series_sort_remap.py b/tests/test_alternate_series_sort_remap.py new file mode 100644 index 000000000..dd03474d5 --- /dev/null +++ b/tests/test_alternate_series_sort_remap.py @@ -0,0 +1,130 @@ +""" +0054 remaps the retired ``alternate_number`` sort key onto ``reprints``. + +The key lived in three places on ``SettingsBrowser`` — the ``order_by`` +column, the ``order_extra_keys`` list and the per-top-collection +``collection_order_memory`` map. Stored settings load without +re-validation, so anything the migration misses reaches ORDER BY and +raises. Runs the migration's helper against live models: every surface +it touches still exists post-migration. +""" + +import importlib +from typing import Final + +from django.apps import apps +from django.test import TestCase + +from codex.models.settings import SettingsBrowser, SettingsBrowserShow + +_MIGRATION = importlib.import_module( + "codex.migrations.0054_merge_alternate_number_sort" +) +_remap_browser_settings = _MIGRATION._remap_browser_settings # noqa: SLF001 + +_OLD: Final = "alternate_number" +_NEW: Final = "reprints" + + +class Migration0054RemapTestCase(TestCase): + """The retired sort key is rewritten everywhere it can be stored.""" + + @staticmethod + def _make_row(**overrides) -> SettingsBrowser: + show, _ = SettingsBrowserShow.objects.get_or_create() + fields = { + "show": show, + "order_by": "sort_name", + "order_extra_keys": [], + "collection_order_memory": {}, + } + fields.update(overrides) + return SettingsBrowser.objects.create(**fields) + + def test_order_by_column_remapped(self) -> None: + """The plain sort key moves over; other rows are left alone.""" + remapped = self._make_row(order_by=_OLD) + untouched = self._make_row(order_by="sort_name") + + _remap_browser_settings(apps, None) + + remapped.refresh_from_db() + untouched.refresh_from_db() + assert remapped.order_by == _NEW + assert untouched.order_by == "sort_name" + + def test_extra_sort_keys_remapped(self) -> None: + """A multi-sort extra on the retired key moves over, keeping its direction.""" + row = self._make_row( + order_extra_keys=[ + {"key": "sort_name", "reverse": False}, + {"key": _OLD, "reverse": True}, + ] + ) + + _remap_browser_settings(apps, None) + + row.refresh_from_db() + assert row.order_extra_keys == [ + {"key": "sort_name", "reverse": False}, + {"key": _NEW, "reverse": True}, + ] + + def test_extra_sort_keys_dedupe(self) -> None: + """A row already sorting by reprints doesn't end up with it twice.""" + # One column can only carry one sort, so the first wins. + row = self._make_row( + order_extra_keys=[ + {"key": _NEW, "reverse": False}, + {"key": _OLD, "reverse": True}, + ] + ) + + _remap_browser_settings(apps, None) + + row.refresh_from_db() + assert row.order_extra_keys == [{"key": _NEW, "reverse": False}] + + def test_collection_order_memory_remapped(self) -> None: + """The per-top-collection sort memory is rewritten too.""" + # Missed here, the dead key gets re-injected into params the + # next time the user switches back to that top collection. + row = self._make_row( + collection_order_memory={ + "comics": { + "order_by": _OLD, + "order_reverse": True, + "order_extra_keys": [{"key": _OLD, "reverse": False}], + }, + "folders": { + "order_by": "sort_name", + "order_reverse": False, + "order_extra_keys": [], + }, + } + ) + + _remap_browser_settings(apps, None) + + row.refresh_from_db() + assert row.collection_order_memory == { + "comics": { + "order_by": _NEW, + "order_reverse": True, + "order_extra_keys": [{"key": _NEW, "reverse": False}], + }, + "folders": { + "order_by": "sort_name", + "order_reverse": False, + "order_extra_keys": [], + }, + } + + def test_saved_views_remapped(self) -> None: + """Saved views are more rows in the same table and get the same pass.""" + saved = self._make_row(name="A Saved View", order_by=_OLD) + + _remap_browser_settings(apps, None) + + saved.refresh_from_db() + assert saved.order_by == _NEW diff --git a/tests/test_browser_ordering.py b/tests/test_browser_ordering.py index cd29bd16f..cadc6094e 100644 --- a/tests/test_browser_ordering.py +++ b/tests/test_browser_ordering.py @@ -38,7 +38,6 @@ ) _NEW_ORDER_BY_KEYS: Final = ( - "alternate_number", "country", "day", "file_type", diff --git a/tests/test_browser_reprints_column.py b/tests/test_browser_reprints_column.py index e2d3b014f..dfb827ba5 100644 --- a/tests/test_browser_reprints_column.py +++ b/tests/test_browser_reprints_column.py @@ -14,7 +14,7 @@ from django.contrib.auth.models import User from django.core.cache import cache -from django.test import Client, TestCase +from django.test import Client, TestCase, override_settings from codex.choices.browser import DUMMY_NULL_NAME, VUETIFY_NULL_CODE from codex.models import Comic, Imprint, Library, Publisher, Series, Volume @@ -164,10 +164,12 @@ def test_collection_row_sort_by_reprints(self) -> None: self.comic.reprints.add( Reprint.objects.create(series_name="Zulu", volume_number=2) ) - # "Zzz" has no reprints, so ascending by the column puts it - # first — the opposite of the ``sort_name`` order the sort - # falls back to when the intersection SQL isn't wired. - self._create_comic("C2", 1, series=self._create_series("Zzz")) + # Each series' alternate series sorts opposite its own name, so + # this order can only come from the intersection SQL — falling + # back to ``sort_name`` would put "Aaa" first. + self._create_comic("C2", 1, series=self._create_series("Aaa")).reprints.add( + Reprint.objects.create(series_name="Zzz") + ) self._set_view_mode_table() response = self.client.patch( @@ -178,8 +180,8 @@ def test_collection_row_sort_by_reprints(self) -> None: assert response.status_code == _HTTP_OK, response.content rows = self._browse_series_rows()["rows"] assert [(row["name"], row["reprints"]) for row in rows] == [ - ("Zzz", []), ("Ser", ["Zulu v2"]), + ("Aaa", ["Zzz"]), ], rows @@ -230,8 +232,16 @@ def test_filter_narrows_to_tagged_comics(self) -> None: assert names == ["C1"], body -class BrowserAlternateNumberSortTestCase(_ReprintsFixtureTestCase): - """Sorting by the alternate series' issue number (ComicInfo AlternateNumber).""" +class BrowserAlternateSeriesSortTestCase(_ReprintsFixtureTestCase): + """ + The Alternate Series sort: alternate series identity, then its issue. + + One key does both halves. It leads with the alternate series so every + issue of one alternate series groups together, then orders within + that group by the parsed ComicInfo AlternateNumber so "#2" precedes + "#10", and falls back to the comic's own series and issue so + untagged comics interleave instead of clumping. + """ def _tag(self, comic: Comic, issue: str, series_name: str = "Crossover") -> Reprint: """Put ``comic`` in an alternate series at ``issue``.""" @@ -253,7 +263,7 @@ def _book_names(self) -> list[str]: return [book["name"] for book in body["books"]] def test_sorts_numerically_not_lexically(self) -> None: - """#2 sorts before #10 — the whole point of the derived columns.""" + """#2 sorts before #10 within an alternate series.""" # ``self.comic`` is C1. Issue numbers are deliberately the # reverse of the alternate numbers so a fallback to the regular # issue sort can't accidentally produce the expected order. @@ -263,19 +273,19 @@ def test_sorts_numerically_not_lexically(self) -> None: reprints.append(Reprint.objects.get(issue="2")) self._set_settings( - orderBy="alternate_number", + orderBy="reprints", orderReverse=False, filters={"reprints": [reprint.pk for reprint in reprints]}, ) assert self._book_names() == ["C1", "C3", "C2"] def test_reverse_sort(self) -> None: - """Reversing the alternate number sort reverses the books.""" + """Reversing the sort reverses the books.""" first = self._tag(self.comic, "2") second = self._tag(self._create_comic("C2", 2), "10") self._set_settings( - orderBy="alternate_number", + orderBy="reprints", orderReverse=True, filters={"reprints": [first.pk, second.pk]}, ) @@ -287,51 +297,169 @@ def test_suffix_breaks_ties(self) -> None: suffixed = self._tag(self._create_comic("C2", 2), "2a") self._set_settings( - orderBy="alternate_number", + orderBy="reprints", orderReverse=False, filters={"reprints": [plain.pk, suffixed.pk]}, ) assert self._book_names() == ["C1", "C2"] - def test_untagged_comic_falls_back_to_its_issue_number(self) -> None: - """A comic with no alternate number sorts by its own issue number.""" - # C1 carries alternate number 2; C2 has no alternate series and - # issue #50. The fallback sorts C2 by 50, i.e. last. Without it - # C2's key would be NULL, which SQLite sorts *first* ascending — - # so the expected order only holds if the fallback is applied. - tagged = self._tag(self.comic, "2") + def test_number_and_suffix_come_from_the_same_reprint(self) -> None: + """A comic in one alternate series twice keys on one whole issue.""" + # C1 is in Crossover at both #10 and #2a, so its key is the + # lesser of those two *whole* issues, "2a" — after C2's plain + # "#2". Aggregating the number and the suffix separately would + # pair 2 with an empty suffix and fabricate a "#2" that C1 + # doesn't have, tying it with C2 and letting the pk tiebreaker + # put C1 first. + first = self._tag(self.comic, "10") + second = self._tag(self.comic, "2a") + third = self._tag(self._create_comic("C2", 2), "2") + + self._set_settings( + orderBy="reprints", + orderReverse=False, + filters={"reprints": [first.pk, second.pk, third.pk]}, + ) + assert self._book_names() == ["C2", "C1"] + + def test_election_is_atomic_across_alternate_series(self) -> None: + """A comic in two alternate series keys on one whole reprint.""" + # C1 is in "Aaa" at #10 and "Zzz" at #2. Its key must be + # (aaa, 10) — the elected reprint whole — never (aaa, 2), the + # min series paired with the min number from a different + # reprint. Only the fabricated pair sorts C1 ahead of C2's + # (aaa, 5). + self._tag(self.comic, "10", series_name="Aaa") + self._tag(self.comic, "2", series_name="Zzz") + self._tag(self._create_comic("C2", 2), "5", series_name="Aaa") + + self._set_settings(orderBy="reprints", orderReverse=False) + assert self._book_names() == ["C2", "C1"] + + def test_untagged_comic_falls_back_to_its_own_series_and_issue(self) -> None: + """A comic with no alternate series sorts by its real series and issue.""" + # C1's alternate series "Zulu" sorts after C2's real series + # "Ser", so the fallback has to place C2 first — the opposite of + # both the issue order (C1 #1, C2 #50) and the pk order. + tagged = self._tag(self.comic, "2", series_name="Zulu") self._create_comic("C2", 50) self._set_settings( - orderBy="alternate_number", + orderBy="reprints", orderReverse=False, filters={"reprints": [tagged.pk, VUETIFY_NULL_CODE]}, ) - assert self._book_names() == ["C1", "C2"] - - def test_without_filter_degrades_to_issue_sort(self) -> None: - """With no alternate series selected the sort is the plain issue sort.""" - self._tag(self.comic, "10") - self._create_comic("C2", 2) - self._create_comic("C3", 3) + assert self._book_names() == ["C2", "C1"] - self._set_settings(orderBy="alternate_number", orderReverse=False) - assert self._book_names() == ["C1", "C2", "C3"] + def test_fallback_uses_raw_series_name_not_sort_name(self) -> None: + """The fallback series segment is the raw name, like alternate names.""" + # Alternate series names sort raw, so the fallback compares + # ``lower(name)`` too. "cats" sits between "batman, the" (the + # article-moved sort_name) and "the batman" (the raw name): + # only the raw-name fallback puts the Cats comic first. + the_batman = self._create_series("The Batman") + self._create_comic("C2", 1, series=the_batman) + self._tag(self._create_comic("C3", 2, series=the_batman), "1", "Cats") + + self._set_settings(orderBy="reprints", orderReverse=False) + body = self._browse(f"/api/v4/browse/series/{the_batman.pk}?page=1") + names = [book["name"] for book in body["books"]] + assert names == ["C3", "C2"], body - def test_collection_rows_sort_by_child_alternate_number(self) -> None: - """Series rows aggregate their children's alternate numbers.""" - self._tag(self.comic, "10") - other_series = self._create_series("Aaa") - early = self._tag(self._create_comic("C2", 2, series=other_series), "3") + def test_alternate_series_name_folds_case(self) -> None: + """Alternate series names compare case-insensitively.""" + # Stored raw, "Zebra" (0x5A) would sort before "apple" (0x61) + # under SQLite's binary collation — which a column's NOCASE + # collation does not survive being composed into a sort key. + first = self._tag(self.comic, "1", series_name="apple") + second = self._tag(self._create_comic("C2", 2), "1", series_name="Zebra") self._set_settings( - orderBy="alternate_number", + orderBy="reprints", orderReverse=False, - filters={"reprints": [early.pk, Reprint.objects.get(issue="10").pk]}, + filters={"reprints": [first.pk, second.pk]}, ) - body = self._browse(f"/api/v4/browse/publishers/{self.publisher.pk}?page=1") - names = [collection["name"] for collection in body["collections"]] - assert names == ["Aaa", "Ser"], body + assert self._book_names() == ["C1", "C2"] + + def test_groups_by_alternate_series_before_issue(self) -> None: + """With no filter, comics group by alternate series, then by issue.""" + # Neither the issue order (C1, C2, C3) nor the pk order can + # produce this: the two Alpha issues must come out together and + # in numeric order, ahead of Zulu. + self._tag(self.comic, "1", series_name="Zulu") + self._tag(self._create_comic("C2", 2), "9", series_name="Alpha") + self._tag(self._create_comic("C3", 3), "1", series_name="Alpha") + + self._set_settings(orderBy="reprints", orderReverse=False) + assert self._book_names() == ["C3", "C2", "C1"] + + def test_alternate_series_without_an_issue_uses_the_comics_own(self) -> None: + """Comics in an alternate series with no AlternateNumber keep issue order.""" + # Both alternate series rows carry no issue at all, so the issue + # segment falls through to the comic's own — without that they + # would share one key and land in pk order. + first = Reprint.objects.create(series_name="Crossover") + self.comic.reprints.add(first) + early = self._create_comic("C2", 0) + early.reprints.add(first) + + self._set_settings(orderBy="reprints", orderReverse=False) + assert self._book_names() == ["C2", "C1"] + + def test_collection_rows_sort_by_their_shared_alternate_issue(self) -> None: + """Series rows sort by the alternate series their children share.""" + # "Ser" shares Crossover #3 and "Aaa" shares Crossover #10, so + # the numeric order is the reverse of both the alphabetical + # sort_name order and the lexical label order ("#10" < "#3"). + self._tag(self.comic, "3") + other_series = self._create_series("Aaa") + self._tag(self._create_comic("C2", 2, series=other_series), "10") + + self._set_view_mode_table() + self._set_settings(orderBy="reprints", orderReverse=False) + rows = self._browse_series_rows()["rows"] + assert [row["name"] for row in rows] == ["Ser", "Aaa"], rows + + def test_collection_sort_survives_the_debug_query_logger(self) -> None: + """The raw SQL's printf formats don't break DEBUG query logging.""" + # ``bin/dev.sh`` runs with DEBUG=1, where every query is + # re-rendered through ``sql % params`` for the log. A bare + # ``%011.2f`` in the intersection RawSQL would swallow a bound + # parameter there and 500 the browse — the raw string doubles + # its percents so the logger renders them as literals. + self._tag(self.comic, "2") + + self._set_view_mode_table() + self._set_settings(orderBy="reprints", orderReverse=False) + with override_settings(DEBUG=True): + rows = self._browse_series_rows()["rows"] + assert [row["name"] for row in rows] == ["Ser"], rows + + def test_collection_rows_without_a_shared_alternate_series_use_their_name( + self, + ) -> None: + """A collection whose children disagree sorts by its own name.""" + # "Ser"'s two children share no alternate series, so its + # intersection is empty and it sorts under "ser" — between + # "Aaa"'s alternate series "aaa" and "Zzz"'s "zzz". Without the + # fallback the empty key would clump it at one end. + self._tag(self.comic, "1", series_name="mmm") + self._create_comic("C2", 2) + self._tag( + self._create_comic("C3", 3, series=self._create_series("Aaa")), + "1", + series_name="zzz", + ) + self._tag( + self._create_comic("C4", 4, series=self._create_series("Zzz")), + "1", + series_name="aaa", + ) + + self._set_view_mode_table() + self._set_settings(orderBy="reprints", orderReverse=False) + rows = self._browse_series_rows()["rows"] + assert [row["name"] for row in rows] == ["Zzz", "Ser", "Aaa"], rows class BrowserReprintsCoverSortTestCase(_ReprintsFixtureTestCase): diff --git a/tests/test_user_data_restore.py b/tests/test_user_data_restore.py index 069cdf795..75523149a 100644 --- a/tests/test_user_data_restore.py +++ b/tests/test_user_data_restore.py @@ -354,6 +354,53 @@ def test_browser_defaults_restore_collection_order_memory(self) -> None: defaults = _build_browser_defaults(row, show=None) assert defaults["collection_order_memory"] == memory + def test_browser_defaults_remap_retired_sort_key(self) -> None: + """A sidecar's retired alternate_number sort restores as reprints.""" + # It lives in three places, and settings load without + # re-validation, so any one left behind reaches ORDER BY. + from codex.user_data.restore import _build_browser_defaults + + row = self._browser_row( + order_by="alternate_number", + order_extra_keys=json.dumps([{"key": "alternate_number", "reverse": True}]), + collection_order_memory=json.dumps( + { + "comics": { + "order_by": "alternate_number", + "order_reverse": False, + "order_extra_keys": [ + {"key": "alternate_number", "reverse": False} + ], + } + } + ), + ) + defaults = _build_browser_defaults(row, show=None) + assert defaults["order_by"] == "reprints" + assert defaults["order_extra_keys"] == [{"key": "reprints", "reverse": True}] + assert defaults["collection_order_memory"] == { + "comics": { + "order_by": "reprints", + "order_reverse": False, + "order_extra_keys": [{"key": "reprints", "reverse": False}], + } + } + + def test_browser_defaults_dedupe_retired_sort_key(self) -> None: + """A sidecar carrying both the retired key and its replacement keeps one.""" + from codex.user_data.restore import _build_browser_defaults + + row = self._browser_row( + order_extra_keys=json.dumps( + [ + {"key": "reprints", "reverse": False}, + {"key": "alternate_number", "reverse": True}, + ] + ) + ) + defaults = _build_browser_defaults(row, show=None) + assert defaults["order_extra_keys"] == [{"key": "reprints", "reverse": False}] + def test_browser_defaults_tolerate_missing_order_memory(self) -> None: """A sidecar written before the column existed restores empty.""" from codex.user_data.restore import _build_browser_defaults From c27fbd7ed5a299dbb5d773f8e1f519b88d352c0e Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Sun, 30 Aug 2026 12:33:34 -0700 Subject: [PATCH 2/4] ignore tasks --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 1b33b3c2a..811ed5928 100644 --- a/.gitignore +++ b/.gitignore @@ -101,3 +101,4 @@ var/ venv.bak/ venv/ wheels/ +tasks/ From 127e5c8c066cdb54ebb17bf3e729d62f4bef9a1b Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Sun, 30 Aug 2026 12:34:05 -0700 Subject: [PATCH 3/4] ignore tasks --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 811ed5928..b56762ba8 100644 --- a/.gitignore +++ b/.gitignore @@ -95,10 +95,10 @@ profile_default/ sdist/ share/python-wheels/ target/ +tasks/ test-results TODO.md var/ venv.bak/ venv/ wheels/ -tasks/ From e27e931dbd2e5776fe17a61e38a5682250355737 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Sun, 30 Aug 2026 12:34:10 -0700 Subject: [PATCH 4/4] prettier --- codex/librarian/scribe/importer/delete/comics.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/codex/librarian/scribe/importer/delete/comics.py b/codex/librarian/scribe/importer/delete/comics.py index 55fc99466..d3aaab31a 100644 --- a/codex/librarian/scribe/importer/delete/comics.py +++ b/codex/librarian/scribe/importer/delete/comics.py @@ -59,7 +59,9 @@ def _populate_deleted_comic_collections( """Populate changed collections for cover timestamp updater.""" comics_deleted_qs = delete_qs.only( *ALL_COMIC_COLLECTION_FIELD_NAMES - ).prefetch_related("story_arc_numbers__story_arc", *DIRECT_M2M_COLLECTION_FIELD_NAMES) + ).prefetch_related( + "story_arc_numbers__story_arc", *DIRECT_M2M_COLLECTION_FIELD_NAMES + ) for comic in comics_deleted_qs.iterator( chunk_size=IMPORTER_DELETE_MAX_CHUNK_SIZE ):