diff --git a/.gitignore b/.gitignore index 1b33b3c2a..b56762ba8 100644 --- a/.gitignore +++ b/.gitignore @@ -95,6 +95,7 @@ profile_default/ sdist/ share/python-wheels/ target/ +tasks/ test-results TODO.md var/ 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/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 ): 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