diff --git a/NEWS.md b/NEWS.md index 157185616..d4800b054 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,20 @@ width: 128px; border-radius: 128px; " /> +## v2.4.0 + +- Features + - Each top collection remembers the sort it was last browsed with. Sort + Issues by added time and Publishers alphabetically, and switching between + 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. + +- Fixes + - Saving browser settings sent an empty request that stored nothing. The + settings were persisted by the page request that followed, so nothing was + lost, but the save request itself did nothing. + ## v2.3.0 - Features diff --git a/codex/choices/browser.py b/codex/choices/browser.py index 19317bb8b..81fe70c29 100644 --- a/codex/choices/browser.py +++ b/codex/choices/browser.py @@ -708,6 +708,7 @@ def admin_default_route_for(top_collection: str) -> dict: "order_by": "sort_name", "order_reverse": False, "order_extra_keys": (), + "collection_order_memory": MappingProxyType({}), "search": "", "show": _DEFAULT_SHOW, "top_collection": "publishers", diff --git a/codex/migrations/0054_settingsbrowser_collection_order_memory.py b/codex/migrations/0054_settingsbrowser_collection_order_memory.py new file mode 100644 index 000000000..ffe35e496 --- /dev/null +++ b/codex/migrations/0054_settingsbrowser_collection_order_memory.py @@ -0,0 +1,29 @@ +"""Generated by Django 6.0.7 on 2026-08-30 12:00.""" + +from django.db import migrations, models + + +class Migration(migrations.Migration): + """ + Remember the sort each top collection was last browsed with. + + Switching top collections used to drag one global sort along with it, so + sorting issues by added time and then looking at publishers left the + publisher list in added-time order too. + + The new map is empty for every existing row, which reads as "no collection + has been customized yet" and keeps the current carry-the-sort-over + behavior until someone changes a sort. Nothing to backfill. + """ + + dependencies = [ + ("codex", "0053_reprint_issue_number"), + ] + + operations = [ + migrations.AddField( + model_name="settingsbrowser", + name="collection_order_memory", + field=models.JSONField(default=dict), + ), + ] diff --git a/codex/models/settings.py b/codex/models/settings.py index 2b39a4c1b..268a7c69a 100644 --- a/codex/models/settings.py +++ b/codex/models/settings.py @@ -350,6 +350,15 @@ class SettingsBrowser(SettingsBase): # Empty list means single-column sort (today's behavior). The # frontend table view adds entries via shift-click on a header. order_extra_keys = JSONField(default=list) + # The sort each top collection was last browsed with, so switching + # between them restores the sort that collection was left in instead + # of dragging one global sort everywhere. Keyed by + # ``BROWSER_TOP_COLLECTION_CHOICES`` key; each value is + # ``{"order_by": , "order_reverse": , "order_extra_keys": [...]}``. + # A missing key means "never customized" and the current sort carries + # over. ``search_score`` is never stored: it only exists while a + # search is active. + collection_order_memory = JSONField(default=dict) search = CharField(max_length=4095, default="", blank=True) # Display preferences @@ -387,6 +396,7 @@ class SettingsBrowser(SettingsBase): "order_by", "order_reverse", "order_extra_keys", + "collection_order_memory", "search", "custom_covers", "dynamic_covers", diff --git a/codex/serializers/browser/settings.py b/codex/serializers/browser/settings.py index 2848f661d..0e0132891 100644 --- a/codex/serializers/browser/settings.py +++ b/codex/serializers/browser/settings.py @@ -15,6 +15,7 @@ ) from codex.choices.browser import ( + BROWSER_EXTRA_SORT_UNSUPPORTED_KEYS, BROWSER_ORDER_BY_CHOICES, BROWSER_TABLE_COLUMNS, BROWSER_TABLE_COVER_SIZE_CHOICES, @@ -31,6 +32,45 @@ from codex.serializers.route import SimpleRouteSerializer from codex.serializers.settings import SettingsInputSerializer +# Sorts worth remembering per top collection. ``search_score`` only means +# anything while a search is running, so it never enters the memory. +_MEMORABLE_ORDER_BY_KEYS = frozenset(BROWSER_ORDER_BY_CHOICES.keys()) - {"search_score"} +# Extra (secondary) sort keys the order pipeline can resolve anywhere. +_MEMORABLE_EXTRA_KEYS = ( + frozenset(BROWSER_ORDER_BY_CHOICES.keys()) - BROWSER_EXTRA_SORT_UNSUPPORTED_KEYS +) + + +def _clean_remembered_extra_keys(value) -> list[dict]: + """Keep the well formed, sortable, non duplicate extra sort entries.""" + cleaned: list[dict] = [] + if not isinstance(value, list | tuple): + return cleaned + seen: set[str] = set() + for entry in value: + if not isinstance(entry, dict): + continue + key = entry.get("key") + if key not in _MEMORABLE_EXTRA_KEYS or key in seen: + continue + seen.add(key) # pyright: ignore[reportArgumentType] + cleaned.append({"key": key, "reverse": bool(entry.get("reverse", False))}) + return cleaned + + +def _clean_remembered_order(value) -> dict | None: + """Coerce one remembered sort, or None when it can't be salvaged.""" + if not isinstance(value, dict): + return None + order_by = value.get("order_by") + if order_by not in _MEMORABLE_ORDER_BY_KEYS: + return None + return { + "order_by": order_by, + "order_reverse": bool(value.get("order_reverse", False)), + "order_extra_keys": _clean_remembered_extra_keys(value.get("order_extra_keys")), + } + class BrowserSettingsShowCollectionFlagsSerializer(Serializer): """Show Collection Flags (collection vocabulary).""" @@ -127,7 +167,7 @@ class BrowserSettingsSerializer(BrowserSettingsSerializerBase): JSON_FIELDS = frozenset( BrowserSettingsSerializerBase.JSON_FIELDS - | {"table_columns", "order_extra_keys"} + | {"table_columns", "order_extra_keys", "collection_order_memory"} ) mtime = TimestampField(read_only=True) @@ -154,6 +194,14 @@ class BrowserSettingsSerializer(BrowserSettingsSerializerBase): required=False, allow_empty=True, ) + # The sort each top collection was last browsed with, keyed by + # top_collection. Cleaned in ``validate_collection_order_memory``; + # see the model field for the stored shape. + collection_order_memory = DictField( + child=DictField(), + required=False, + allow_empty=True, + ) def validate_table_columns(self, value): """ @@ -183,6 +231,33 @@ def validate_table_columns(self, value): cleaned[top_collection] = [c for c in columns if c in valid_columns] return cleaned + def validate_collection_order_memory(self, value): + """ + Drop unknown top collections and unusable remembered sorts. + + Like ``table_columns`` this round-trips out of stored settings, so a + stale client must not hard-400 the whole browse page. An entry naming + a sort that no longer exists is dropped with a warning; the collection + then just keeps whatever sort it is browsed with next. + """ + cleaned: dict[str, dict] = {} + for top_collection, order in value.items(): + if top_collection not in BROWSER_TOP_COLLECTION_CHOICES: + logger.warning( + "Dropping unknown collection_order_memory top_collection " + f"{top_collection!r}" + ) + continue + remembered_order = _clean_remembered_order(order) + if remembered_order is None: + logger.warning( + "Dropping unusable collection_order_memory order for " + f"{top_collection!r}" + ) + continue + cleaned[top_collection] = remembered_order + return cleaned + def validate_order_extra_keys(self, value): """ Reject malformed entries; coerce to the canonical shape. diff --git a/codex/user_data/restore.py b/codex/user_data/restore.py index 6a806ab3c..9ca05c9c6 100644 --- a/codex/user_data/restore.py +++ b/codex/user_data/restore.py @@ -568,6 +568,15 @@ def _resolve_filter_column(row_keys, column: str) -> str | None: return None +def _row_column(row, column: str): + """Read a column a sidecar written by an older codex may not carry.""" + try: + return row[column] + except (IndexError, KeyError): + # sqlite3.Row raises IndexError, a plain mapping raises KeyError. + return None + + 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 "" @@ -582,6 +591,9 @@ def _build_browser_defaults(row, show) -> dict[str, Any]: "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 "{}" + ), "search": row["search"] or "", "custom_covers": bool(row["custom_covers"]), "dynamic_covers": bool(row["dynamic_covers"]), diff --git a/codex/user_data/schema.sql b/codex/user_data/schema.sql index 5156c0729..72ec50aaa 100644 --- a/codex/user_data/schema.sql +++ b/codex/user_data/schema.sql @@ -86,6 +86,7 @@ CREATE TABLE IF NOT EXISTS settings_browser ( order_by TEXT NOT NULL DEFAULT '', order_reverse INTEGER NOT NULL DEFAULT 0, order_extra_keys TEXT NOT NULL DEFAULT '[]', + collection_order_memory TEXT NOT NULL DEFAULT '{}', search TEXT NOT NULL DEFAULT '', custom_covers INTEGER NOT NULL DEFAULT 1, dynamic_covers INTEGER NOT NULL DEFAULT 1, diff --git a/codex/user_data/serializers.py b/codex/user_data/serializers.py index 714d9b68d..68b2a4814 100644 --- a/codex/user_data/serializers.py +++ b/codex/user_data/serializers.py @@ -205,6 +205,9 @@ def serialize_settings_browser( "order_extra_keys": json.dumps( browser.order_extra_keys, separators=(",", ":") ), + "collection_order_memory": json.dumps( + browser.collection_order_memory, separators=(",", ":") + ), "search": browser.search, "custom_covers": int(browser.custom_covers), "dynamic_covers": int(browser.dynamic_covers), diff --git a/codex/views/browser/settings.py b/codex/views/browser/settings.py index 2417b449d..0be7236d5 100644 --- a/codex/views/browser/settings.py +++ b/codex/views/browser/settings.py @@ -27,6 +27,40 @@ # Collections whose nav owns a dedicated route (folders / story arcs); for these # the URL collection becomes the top_collection directly during validation. _OWN_ROUTE_COLLECTIONS = frozenset({FOLDER_COLLECTION, STORY_ARC_COLLECTION}) +_SEARCH_ORDER_BY = "search_score" + + +def apply_collection_order_memory( + params: MutableMapping, old_top_collection: str, new_top_collection: str +) -> None: + """ + Carry the remembered sort across a top collection change. + + Files the sort the old collection is leaving with, then restores the sort + the new collection was last browsed with. The browser store does this for + collection switches the user makes; this covers the ones the server makes + on its own when a url or a settings change forces a different top + collection, which would otherwise leak one collection's sort into another. + """ + if old_top_collection == new_top_collection: + return + memory = dict(params.get("collection_order_memory") or {}) + order_by = params.get("order_by") + if old_top_collection and order_by and order_by != _SEARCH_ORDER_BY: + memory[old_top_collection] = { + "order_by": order_by, + "order_reverse": bool(params.get("order_reverse")), + "order_extra_keys": list(params.get("order_extra_keys") or ()), + } + params["collection_order_memory"] = memory + + remembered = memory.get(new_top_collection) + if not remembered or params.get("search"): + # An active search owns the sort until it's cleared. + return + params["order_by"] = remembered["order_by"] + params["order_reverse"] = remembered["order_reverse"] + params["order_extra_keys"] = list(remembered.get("order_extra_keys") or ()) class BrowserSettingsBaseView(SettingsBaseView): @@ -145,6 +179,7 @@ def _validate_settings_get(self, validated_data, params: dict) -> dict: else Collection.ROOT ) self._validate_top_collection(params, collection, top_collection) + apply_collection_order_memory(params, top_collection, params["top_collection"]) self.set_order_by_default(params) return params diff --git a/codex/views/browser/validate.py b/codex/views/browser/validate.py index 5050379dc..138d76c07 100644 --- a/codex/views/browser/validate.py +++ b/codex/views/browser/validate.py @@ -12,6 +12,7 @@ from codex.models.collections import BrowserCollectionModel from codex.util import mapping_to_dict from codex.views.browser.filters.search.parse import SearchFilterView +from codex.views.browser.settings import apply_collection_order_memory from codex.views.const import ( COLLECTION_MODEL_MAP, COMIC_COLLECTION, @@ -77,7 +78,12 @@ def raise_redirect( route["params"].update(route_mask) settings = cast("dict[str, Any]", deepcopy(mapping_to_dict(self.params))) if settings_mask: + old_top_collection = settings.get("top_collection", "") settings.update(settings_mask) + # A redirect that moves the top collection moves its sort too. + apply_collection_order_memory( + settings, old_top_collection, settings.get("top_collection", "") + ) detail = {"route": route, "settings": settings, "reason": reason} raise SeeOtherRedirectError(detail=detail) diff --git a/codex/views/settings.py b/codex/views/settings.py index 5abc65c37..888e64ea0 100644 --- a/codex/views/settings.py +++ b/codex/views/settings.py @@ -26,7 +26,6 @@ SettingsReader, ) from codex.views.auth import AuthFilterGenericAPIView -from codex.views.const import FOLDER_COLLECTION, STORY_ARC_COLLECTION # Fallback top-collection when the BG flag row is missing, off, or holds # an invalid value. Mirrors ``SettingsBrowser.top_collection``'s model @@ -405,21 +404,6 @@ def load_params_from_settings(self, only: Sequence[str] | None = None) -> dict: # ── Save (write) ──────────────────────────────────────────────── - def _get_browser_order_defaults(self) -> dict: - if collection := self.kwargs.get("collection"): - # order_by has a dynamic collection based default - order_by = ( - "filename" - if collection == FOLDER_COLLECTION - else "story_arc_number" - if collection == STORY_ARC_COLLECTION - else "sort_name" - ) - order_defaults = {"order_by": order_by} - else: - order_defaults = {} - return order_defaults - @staticmethod def _save_browser_show(instance: SettingsBrowser, show_data: dict) -> bool: """ diff --git a/frontend/src/api/v4/browser.js b/frontend/src/api/v4/browser.js index fe47f3f1e..200fad001 100644 --- a/frontend/src/api/v4/browser.js +++ b/frontend/src/api/v4/browser.js @@ -149,7 +149,10 @@ export const getSettings = (data) => { export const updateSettings = (settings) => { const params = serializeParams(settings, undefined, false); - return HTTP.patch(_collectionSettingsBase(settings?.collection), { params }); + // The settings go in the request body, not a `params` wrapper: the + // endpoint validates the body's top-level keys, so a wrapped object + // validates as empty and saves nothing. + return HTTP.patch(_collectionSettingsBase(settings?.collection), params); }; export const resetSettings = (settings) => diff --git a/frontend/src/stores/browser.js b/frontend/src/stores/browser.js index d3f0cfae8..eb4f83e0f 100644 --- a/frontend/src/stores/browser.js +++ b/frontend/src/stores/browser.js @@ -231,6 +231,7 @@ export const useBrowserStore = defineStore("browser", { orderBy: BROWSER_DEFAULTS.orderBy, orderReverse: BROWSER_DEFAULTS.orderReverse, orderExtraKeys: BROWSER_DEFAULTS.orderExtraKeys ?? [], + collectionOrderMemory: BROWSER_DEFAULTS.collectionOrderMemory ?? {}, search: BROWSER_DEFAULTS.search, show: BROWSER_DEFAULTS.show, topCollection: BROWSER_DEFAULTS.topCollection, @@ -496,14 +497,94 @@ export const useBrowserStore = defineStore("browser", { return this.settings.show[topCollection]; } }, + /* + * COLLECTION ORDER MEMORY + * + * Each top collection remembers the sort it was last browsed with, so + * switching between them (or leaving and returning from a search) + * restores that sort instead of dragging one global sort everywhere. + * A collection with nothing filed keeps whatever sort arrives with it. + */ + _stashCollectionOrder(data, topCollection) { + /* + * File the sort ``topCollection`` is being left in. ``search_score`` + * is never filed: it only means anything while that search runs. + */ + const { orderBy, orderReverse, orderExtraKeys } = this.settings; + if (!topCollection || !orderBy || orderBy === "search_score") { + return; + } + data.collectionOrderMemory = { + ...(data.collectionOrderMemory ?? this.settings.collectionOrderMemory), + [topCollection]: { + orderBy, + orderReverse, + orderExtraKeys: [...(orderExtraKeys ?? [])], + }, + }; + }, + _applyOrder(data, order) { + /* + * Write a sort into a settings payload without overruling one the + * payload already asks for: a sort the user just picked outranks + * anything filed away earlier. + */ + if (!Object.hasOwn(data, "orderBy")) { + data.orderBy = order.orderBy; + } + if (!Object.hasOwn(data, "orderReverse")) { + data.orderReverse = order.orderReverse; + } + if (!Object.hasOwn(data, "orderExtraKeys")) { + data.orderExtraKeys = [...(order.orderExtraKeys ?? [])]; + } + }, + _restoreCollectionOrder(data, topCollection) { + // Hand back the sort topCollection was last browsed with, if any. + const memory = + data.collectionOrderMemory ?? this.settings.collectionOrderMemory; + const order = memory?.[topCollection]; + if (!order) { + return false; + } + this._applyOrder(data, order); + return true; + }, + _restoreSearchOrder(data) { + /* + * Hand the sort back when a search ends: the one this collection was + * last browsed with, or its plain default when nothing was filed. + */ + if (this._restoreCollectionOrder(data, this.settings.topCollection)) { + return; + } + this._applyOrder(data, { + orderBy: + this.settings.topCollection === "folders" ? "filename" : "sort_name", + orderReverse: false, + orderExtraKeys: [], + }); + }, + _applyCollectionOrderMemory(data, isCollectionSwitch) { + /* + * Swap the remembered sorts over a user's top collection change. + * Server-sent payloads (settings load, saved view, redirect) carry + * their own sort and are left alone; the backend files those. + */ + if (!isCollectionSwitch) { + return; + } + this._stashCollectionOrder(data, this.settings.topCollection); + if (!this.settings.search && !data.search) { + // A search owns the sort until it's cleared. + this._restoreCollectionOrder(data, data.topCollection); + } + }, _validateSearch(data) { if (!this.settings.search && !data.search) { // if cleared search check for bad order_by if (this.settings.orderBy === "search_score") { - data.orderBy = - this.settings.topCollection === "folders" - ? "filename" - : "sort_name"; + this._restoreSearchOrder(data); } return; } else if (this.settings.search) { @@ -514,6 +595,7 @@ export const useBrowserStore = defineStore("browser", { // Otherwise we'd strand the user at e.g. the series root with no // parent breadcrumbs / up-arrows. if (Object.hasOwn(data, "search") && !data.search) { + this._restoreSearchOrder(data); const { collection, pks } = liveBrowseParams(); if (!pks && collection === this.lowestShownCollection) { return { params: { collection: "root", pks: "", page: "1" } }; @@ -522,7 +604,9 @@ export const useBrowserStore = defineStore("browser", { // Still searching (or nothing to undo): don't redirect. return; } - // If first search redirect to lowest collection and change order + // If first search redirect to lowest collection and change order. + // File the sort first so clearing the search can hand it back. + this._stashCollectionOrder(data, this.settings.topCollection); data.orderBy = "search_score"; data.orderReverse = true; const collection = liveBrowseParams().collection; @@ -672,7 +756,17 @@ export const useBrowserStore = defineStore("browser", { this.startSearchHideTimeout(); }, _validateAndSaveSettings(data) { + /* + * Decide before the validators run: they inject an ``orderBy`` of + * their own, which would otherwise read as a payload that already + * carries a sort. + */ + const isCollectionSwitch = + Boolean(data?.topCollection) && + data.topCollection !== this.settings.topCollection && + !Object.hasOwn(data, "orderBy"); let redirect = this._validateSearch(data); + this._applyCollectionOrderMemory(data, isCollectionSwitch); redirect = this._validateTopCollection(data, redirect); if (dequal(redirect?.params, liveBrowseParams())) { // not triggered if page is numeric, which is intended. @@ -716,6 +810,10 @@ export const useBrowserStore = defineStore("browser", { state.settings.search = data.search; state.settings.orderBy = data.orderBy; state.settings.orderReverse = data.orderReverse; + // Assigned, not merged: a reset empties the memory, and + // ``_addSettings``' merge could never remove an entry. + state.settings.collectionOrderMemory = + data.collectionOrderMemory ?? {}; } state.browserPageLoaded = true; }); @@ -983,6 +1081,9 @@ export const useBrowserStore = defineStore("browser", { return; } this._validateAndSaveSettings(settings); + // Claim the page so the route watcher reloads with these settings + // instead of re-fetching the stored ones over them. + this.browserPageLoaded = true; // ignore redirect router.push(toBrowseRoute(route)).catch(console.error); }, diff --git a/frontend/tests/unit/browser-store-order-memory.test.js b/frontend/tests/unit/browser-store-order-memory.test.js new file mode 100644 index 000000000..d4993bf3a --- /dev/null +++ b/frontend/tests/unit/browser-store-order-memory.test.js @@ -0,0 +1,202 @@ +/* + * Tests for the per-top-collection sort memory in ``stores/browser.js`` + * (issue #415). + * + * Each top collection remembers the sort it was last browsed with, so + * switching between them — or leaving and returning from a search — + * hands that sort back instead of dragging one global sort everywhere. + */ +import { createPinia, setActivePinia } from "pinia"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// ``liveBrowseParams()`` (module scope in browser.js) reads +// ``router.currentRoute.value``; mock the router so the store can be +// driven without a real one. +vi.mock("@/plugins/router", () => ({ + default: { currentRoute: { value: { params: {}, query: {} } } }, +})); + +import { useBrowserStore } from "@/stores/browser"; + +const ADDED_TIME = { + orderBy: "created_at", + orderReverse: true, + orderExtraKeys: [], +}; + +const makeStore = (settings = {}) => { + const store = useBrowserStore(); + store.settings.topCollection = "publishers"; + store.settings.orderBy = "sort_name"; + store.settings.orderReverse = false; + store.settings.orderExtraKeys = []; + store.settings.search = ""; + store.settings.collectionOrderMemory = {}; + store.settings.show = { + publishers: true, + imprints: true, + series: true, + volumes: false, + }; + Object.assign(store.settings, settings); + return store; +}; + +beforeEach(() => { + setActivePinia(createPinia()); +}); + +describe("collection order memory — switching top collections", () => { + it("files the sort the old collection is left in", () => { + const store = makeStore({ orderBy: "created_at", orderReverse: true }); + const data = { topCollection: "comics" }; + + store._validateAndSaveSettings(data); + + expect(data.collectionOrderMemory.publishers).toStrictEqual(ADDED_TIME); + }); + + it("hands back the sort the new collection was last browsed with", () => { + const store = makeStore({ + collectionOrderMemory: { comics: ADDED_TIME }, + }); + const data = { topCollection: "comics" }; + + store._validateAndSaveSettings(data); + + expect(data.orderBy).toBe("created_at"); + expect(data.orderReverse).toBe(true); + expect(data.orderExtraKeys).toStrictEqual([]); + }); + + it("keeps the current sort for a collection with nothing filed", () => { + const store = makeStore({ orderBy: "created_at", orderReverse: true }); + const data = { topCollection: "comics" }; + + store._validateAndSaveSettings(data); + + expect(data.orderBy).toBeUndefined(); + expect(store.settings.orderBy).toBe("created_at"); + }); + + it("round-trips a sort back to the collection it came from", () => { + const store = makeStore({ orderBy: "created_at", orderReverse: true }); + + // Leave issues for publishers... + store.settings.topCollection = "comics"; + const toPublishers = { topCollection: "publishers" }; + store._validateAndSaveSettings(toPublishers); + store.settings.orderBy = "sort_name"; + store.settings.orderReverse = false; + + // ...then come back. + const toComics = { topCollection: "comics" }; + store._validateAndSaveSettings(toComics); + + expect(toComics.orderBy).toBe("created_at"); + expect(toComics.orderReverse).toBe(true); + }); + + it("leaves a payload that carries its own sort alone", () => { + // Settings loads, saved views and redirects all arrive this way. + const store = makeStore({ + collectionOrderMemory: { comics: ADDED_TIME }, + }); + const data = { + topCollection: "comics", + orderBy: "sort_name", + orderReverse: false, + }; + + store._validateAndSaveSettings(data); + + expect(data.orderBy).toBe("sort_name"); + expect(data.collectionOrderMemory).toBeUndefined(); + }); + + it("ignores a settings change that isn't a collection switch", () => { + const store = makeStore(); + const data = { orderBy: "created_at", orderReverse: true }; + + store._validateAndSaveSettings(data); + + expect(data.collectionOrderMemory).toBeUndefined(); + }); + + it("does not disturb an active search", () => { + const store = makeStore({ + search: "batman", + orderBy: "search_score", + orderReverse: true, + collectionOrderMemory: { comics: ADDED_TIME }, + }); + const data = { topCollection: "comics" }; + + store._validateAndSaveSettings(data); + + expect(data.orderBy).toBeUndefined(); + // Relevance ordering is never filed against the collection either. + expect(data.collectionOrderMemory?.publishers).toBeUndefined(); + }); +}); + +describe("collection order memory — around a search", () => { + it("files the sort a search displaces and hands it back on clear", () => { + const store = makeStore({ orderBy: "created_at", orderReverse: true }); + + const searching = { search: "batman" }; + store._validateSearch(searching); + expect(searching.collectionOrderMemory.publishers).toStrictEqual( + ADDED_TIME, + ); + expect(searching.orderBy).toBe("search_score"); + + // The store now looks like it does mid-search. + store.settings.search = "batman"; + store.settings.orderBy = "search_score"; + store.settings.orderReverse = true; + store.settings.collectionOrderMemory = searching.collectionOrderMemory; + + const clearing = { search: "" }; + store._validateSearch(clearing); + + expect(clearing.orderBy).toBe("created_at"); + expect(clearing.orderReverse).toBe(true); + }); + + it("falls back to the collection default when nothing was filed", () => { + const store = makeStore({ search: "batman", orderBy: "search_score" }); + + const clearing = { search: "" }; + store._validateSearch(clearing); + + expect(clearing.orderBy).toBe("sort_name"); + expect(clearing.orderReverse).toBe(false); + }); + + it("falls back to filename in folders", () => { + const store = makeStore({ + topCollection: "folders", + search: "batman", + orderBy: "search_score", + }); + + const clearing = { search: "" }; + store._validateSearch(clearing); + + expect(clearing.orderBy).toBe("filename"); + }); + + it("cleans up a stale relevance sort left behind by a cleared search", () => { + const store = makeStore({ + search: "", + orderBy: "search_score", + collectionOrderMemory: { publishers: ADDED_TIME }, + }); + + const data = {}; + store._validateSearch(data); + + expect(data.orderBy).toBe("created_at"); + }); +}); diff --git a/tests/test_collection_order_memory.py b/tests/test_collection_order_memory.py new file mode 100644 index 000000000..bed215df8 --- /dev/null +++ b/tests/test_collection_order_memory.py @@ -0,0 +1,326 @@ +"""Tests for the per-top-collection sort memory (issue #415).""" + +import json +from typing import Final, override + +from django.contrib.auth.models import User +from django.test import Client, TestCase + +from codex.models.settings import SettingsBrowser +from codex.serializers.browser.settings import BrowserSettingsSerializer +from codex.startup import init_admin_flags +from codex.views.browser.settings import apply_collection_order_memory + +_TEST_PASSWORD: Final = "test-pw-hush-S106" # noqa: S105 +_HTTP_OK: Final = 200 +_HTTP_CREATED: Final = 201 +_HTTP_BAD_REQUEST: Final = 400 +_SETTINGS_URL: Final = "/api/v4/browse/publishers/settings" +_FOLDERS_SETTINGS_URL: Final = "/api/v4/browse/folders/settings" +_SAVED_URL: Final = "/api/v4/browse/publishers/saved-settings" + +_ADDED_TIME_DESC: Final = { + "orderBy": "created_at", + "orderReverse": True, + "orderExtraKeys": [], +} + + +def _v4(response): + """Unwrap the v4 ``{data, meta, errors}`` envelope and return ``data``.""" + body = response.json() + if isinstance(body, dict) and "data" in body and "meta" in body: + return body["data"] + return body + + +class CollectionOrderMemoryModelTestCase(TestCase): + """The field is wired into the generic settings machinery.""" + + def test_default_is_empty_dict(self): + field = SettingsBrowser._meta.get_field("collection_order_memory") + assert field.default is dict + assert field.default() == {} + + def test_direct_keys_includes_field(self): + # DIRECT_KEYS membership is what carries the field through load, + # save, reset and saved-view cloning. + assert "collection_order_memory" in SettingsBrowser.DIRECT_KEYS + + +class CollectionOrderMemorySerializerTestCase(TestCase): + """The validator cleans leniently instead of rejecting.""" + + @staticmethod + def _validated(memory): + serializer = BrowserSettingsSerializer(data={"collectionOrderMemory": memory}) + assert serializer.is_valid(), serializer.errors + return serializer.validated_data["collection_order_memory"] + + def test_valid_memory_round_trips_as_snake_case(self): + cleaned = self._validated( + {"comics": {**_ADDED_TIME_DESC, "orderExtraKeys": [{"key": "year"}]}} + ) + assert cleaned == { + "comics": { + "order_by": "created_at", + "order_reverse": True, + "order_extra_keys": [{"key": "year", "reverse": False}], + } + } + + def test_unknown_top_collection_dropped(self): + cleaned = self._validated( + {"nope": _ADDED_TIME_DESC, "comics": _ADDED_TIME_DESC} + ) + assert set(cleaned) == {"comics"} + + def test_unknown_order_by_dropped(self): + assert self._validated({"comics": {"orderBy": "not_a_sort"}}) == {} + + def test_search_score_never_remembered(self): + # Relevance ordering only means anything while its search runs. + assert self._validated({"comics": {"orderBy": "search_score"}}) == {} + + def test_extra_keys_cleaned_without_raising(self): + cleaned = self._validated( + { + "comics": { + "orderBy": "sort_name", + "orderExtraKeys": [ + "garbage", + {"key": "story_arc_number"}, + {"key": "year", "reverse": True}, + {"key": "year"}, + ], + } + } + ) + # Unsortable-as-an-extra and duplicate entries go; the first + # ``year`` survives with its own reverse flag. + assert cleaned["comics"]["order_extra_keys"] == [ + {"key": "year", "reverse": True} + ] + + def test_non_dict_order_is_rejected(self): + # Type garbage still 400s at the field layer, like table_columns. + serializer = BrowserSettingsSerializer( + data={"collectionOrderMemory": {"comics": 5}} + ) + assert not serializer.is_valid() + + +class CollectionOrderMemoryHelperTestCase(TestCase): + """The server-side stash/restore used when the server moves the top.""" + + @staticmethod + def _params(**overrides): + params = { + "top_collection": "publishers", + "order_by": "sort_name", + "order_reverse": False, + "order_extra_keys": [], + "collection_order_memory": {}, + "search": "", + } + params.update(overrides) + return params + + def test_stashes_the_departing_sort(self): + params = self._params(order_by="created_at", order_reverse=True) + apply_collection_order_memory(params, "publishers", "folders") + assert params["collection_order_memory"]["publishers"] == { + "order_by": "created_at", + "order_reverse": True, + "order_extra_keys": [], + } + + def test_restores_the_arriving_sort(self): + params = self._params( + collection_order_memory={ + "folders": { + "order_by": "filename", + "order_reverse": True, + "order_extra_keys": [], + } + } + ) + apply_collection_order_memory(params, "publishers", "folders") + assert params["order_by"] == "filename" + assert params["order_reverse"] is True + + def test_unremembered_collection_keeps_the_current_sort(self): + params = self._params(order_by="created_at") + apply_collection_order_memory(params, "publishers", "comics") + assert params["order_by"] == "created_at" + + def test_no_change_is_a_noop(self): + params = self._params() + apply_collection_order_memory(params, "publishers", "publishers") + assert params["collection_order_memory"] == {} + + def test_active_search_keeps_its_sort(self): + params = self._params( + order_by="search_score", + search="batman", + collection_order_memory={ + "folders": { + "order_by": "filename", + "order_reverse": False, + "order_extra_keys": [], + } + }, + ) + apply_collection_order_memory(params, "publishers", "folders") + assert params["order_by"] == "search_score" + # ...and relevance ordering is not filed against publishers. + assert "publishers" not in params["collection_order_memory"] + + def test_unset_sort_is_not_remembered(self): + params = self._params(order_by="") + apply_collection_order_memory(params, "publishers", "folders") + assert params["collection_order_memory"] == {} + + +class CollectionOrderMemoryRoundTripTestCase(TestCase): + """End-to-end through the settings HTTP endpoint.""" + + @override + def setUp(self) -> None: + init_admin_flags() + self.user = User.objects.create_user( # pyright: ignore[reportUninitializedInstanceVariable] + username="order_memory_test", password=_TEST_PASSWORD + ) + self.client = Client() + self.client.force_login(self.user) + + def _patch(self, payload: dict): + return self.client.patch( + _SETTINGS_URL, + data=json.dumps(payload), + content_type="application/json", + ) + + def _get(self, url: str = _SETTINGS_URL) -> dict: + response = self.client.get(url) + assert response.status_code == _HTTP_OK, response.content + return _v4(response) + + def test_default_get_is_empty(self): + assert self._get()["collectionOrderMemory"] == {} + + def test_patch_persists_to_the_row(self): + memory = {"comics": _ADDED_TIME_DESC} + response = self._patch({"collectionOrderMemory": memory}) + assert response.status_code == _HTTP_OK, response.content + assert self._get()["collectionOrderMemory"] == memory + row = SettingsBrowser.objects.get(user=self.user, name="") + assert row.collection_order_memory == { + "comics": { + "order_by": "created_at", + "order_reverse": True, + "order_extra_keys": [], + } + } + + def test_patch_drops_unknown_top_collection(self): + response = self._patch( + {"collectionOrderMemory": {"nope": _ADDED_TIME_DESC}}, + ) + assert response.status_code == _HTTP_OK, response.content + assert self._get()["collectionOrderMemory"] == {} + + def test_settings_get_accepts_a_memory_query_string(self): + """A URL-encoded JSON map in the query string must not 400.""" + memory = {"comics": _ADDED_TIME_DESC} + url = f"{_SETTINGS_URL}?collectionOrderMemory={json.dumps(memory)}" + response = self.client.get(url) + assert response.status_code == _HTTP_OK, response.content + + def test_browse_page_query_string_persists_the_memory(self): + """ + The browse page is what actually stores an echoed memory map. + + The browser sends its whole settings object as query params on + every page fetch, and that request persists them; the store + never PATCHes the map on its own. + """ + memory = {"comics": _ADDED_TIME_DESC} + url = f"/api/v4/browse/publishers?page=1&collectionOrderMemory={json.dumps(memory)}" + response = self.client.get(url) + assert response.status_code == _HTTP_OK, response.content + row = SettingsBrowser.objects.get(user=self.user, name="") + assert row.collection_order_memory == { + "comics": { + "order_by": "created_at", + "order_reverse": True, + "order_extra_keys": [], + } + } + + def test_delete_resets_the_memory(self): + self._patch({"collectionOrderMemory": {"comics": _ADDED_TIME_DESC}}) + response = self.client.delete(_SETTINGS_URL) + assert response.status_code == _HTTP_OK, response.content + assert self._get()["collectionOrderMemory"] == {} + + def test_saved_view_round_trips_the_memory(self): + memory = {"comics": _ADDED_TIME_DESC} + assert self._patch({"collectionOrderMemory": memory}).status_code == _HTTP_OK + + save_resp = self.client.post( + _SAVED_URL, + data=json.dumps({"name": "AddedTime"}), + content_type="application/json", + ) + assert save_resp.status_code == _HTTP_CREATED, save_resp.content + listed = next( + entry + for entry in _v4(self.client.get(_SAVED_URL))["savedSettings"] + if entry["name"] == "AddedTime" + ) + + # Reset so the loaded values can only come from the saved row. + self.client.delete(_SETTINGS_URL) + load_resp = self.client.get(f"{_SAVED_URL}/{listed['pk']}") + assert load_resp.status_code == _HTTP_OK, load_resp.content + assert _v4(load_resp)["settings"]["collectionOrderMemory"] == memory + + def test_url_forced_top_collection_swaps_the_sort(self): + """ + A url that forces a different top collection swaps sorts too. + + Entering the folders url while the stored top collection is + publishers is a collection switch the browser store never sees, so + the settings GET has to file the publisher sort and hand back the + folder one itself. + """ + folder_order = { + "orderBy": "filename", + "orderReverse": True, + "orderExtraKeys": [], + } + assert ( + self._patch( + { + "orderBy": "created_at", + "orderReverse": True, + "collectionOrderMemory": {"folders": folder_order}, + } + ).status_code + == _HTTP_OK + ) + + # The client names the collection in the path and the query, and + # the settings GET reads the query one when it validates the top. + body = self._get(f"{_FOLDERS_SETTINGS_URL}?collection=folders") + + assert body["topCollection"] == "folders" + assert body["orderBy"] == "filename" + assert body["orderReverse"] is True + # The sort publishers was left in is filed on the way past. + assert body["collectionOrderMemory"]["publishers"] == { + "orderBy": "created_at", + "orderReverse": True, + "orderExtraKeys": [], + } diff --git a/tests/test_user_data_restore.py b/tests/test_user_data_restore.py index c46633133..069cdf795 100644 --- a/tests/test_user_data_restore.py +++ b/tests/test_user_data_restore.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import shutil import sqlite3 from pathlib import Path @@ -338,6 +339,28 @@ def test_browser_defaults_pass_current_keys_through(self) -> None: assert defaults["order_by"] == "community_rating" assert defaults["table_columns"] == {"community_rating": True} + def test_browser_defaults_restore_collection_order_memory(self) -> None: + """The per-collection sort memory round-trips out of the sidecar.""" + from codex.user_data.restore import _build_browser_defaults + + memory = { + "comics": { + "order_by": "created_at", + "order_reverse": True, + "order_extra_keys": [], + } + } + row = self._browser_row(collection_order_memory=json.dumps(memory)) + defaults = _build_browser_defaults(row, show=None) + assert defaults["collection_order_memory"] == memory + + 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 + + defaults = _build_browser_defaults(self._browser_row(), show=None) + assert defaults["collection_order_memory"] == {} + def test_filter_restore_reads_legacy_column(self) -> None: """A legacy critical_rating filter column lands on community_rating.""" from codex.models.settings import (