Merge pull request #12186 from Harshul23/12153/hotfix/series-position-sorting

Fix series sorting for numeric, non-numeric, and range positions

Author: Harshul23

openlibrary/core/lists/model.py

@@ -2,6 +2,7 @@
 
 import contextlib
 import logging
+import re
 import typing
 from collections.abc import Iterable
 from functools import cached_property
@@ -21,10 +22,17 @@
     Thing,
     ThingKey,
     ThingReferenceDict,
+    WorkSeriesEdgeDB,
     does_seed_have_metadata,
     update_list_seed_metadata,
 )
-from openlibrary.plugins.upstream.models import Author, Changeset, Edition, User, Work
+from openlibrary.plugins.upstream.models import (
+    Author,
+    Changeset,
+    Edition,
+    User,
+    Work,
+)
 from openlibrary.plugins.worksearch.search import get_solr
 from openlibrary.plugins.worksearch.subjects import get_subject
 from openlibrary.utils.solr import Solr
@@ -656,6 +664,24 @@ class SeriesDict(ListDict):
     pass
 
 
+def get_work_sort_key(
+    tpl: tuple[Work, WorkSeriesEdgeDB],
+) -> tuple[str, int, float, str]:
+    work, edge = tpl
+    position = edge.get('position')
+    pos_str = str(position or "").strip()
+
+    with contextlib.suppress(ValueError):
+        return ("A: Numeric", 1, float(pos_str), work.key)
+
+    if match := re.fullmatch(r'(\d+)\s*-\s*(\d+)', pos_str):
+        lower = int(match.group(1))
+        upper = int(match.group(2))
+        return ("C: Range", upper - lower, float(lower), work.key)
+
+    return ("B: Non-numeric", 0, 0.0, work.key)
+
+
 class Series(List):
     SEED_LIMIT = 100
 
@@ -681,15 +707,7 @@ def get_seeds(self, sort=False, resolve_redirects=False) -> list['Seed']:
             (work, edge) for work in works if (edge := work.find_series_edge(self.key))
         ]
 
-        def get_work_position(position: str | None) -> float:
-            position_float = 9999.0
-            with contextlib.suppress(ValueError):
-                position_float = float(position or 9999)
-            return position_float
-
-        sorted_edges = sorted(
-            series_edges, key=lambda tpl: get_work_position(tpl[1].get('position'))
-        )
+        sorted_edges = sorted(series_edges, key=get_work_sort_key)
 
         seeds: list[Seed] = []
         for work, edge in sorted_edges:

openlibrary/tests/core/lists/test_model.py

@@ -1,3 +1,5 @@
+import web
+
 import openlibrary.core.lists.model as list_model
 from openlibrary.mocks.mock_infobase import MockSite
 
@@ -27,3 +29,105 @@ def save_doc(self, site, type, key, **fields):
         d = {"key": key, "type": {"key": type}}
         d.update(fields)
         site.save(d)
+
+
+class MockWork:
+    def __init__(self, key, position):
+        self.key = key
+        self._position = position
+
+    def find_series_edge(self, series_key):
+        return {"position": self._position}
+
+
+class MockSeriesSite:
+    def __init__(self, works):
+        self._works = works
+
+    def things(self, query):
+        return [work.key for work in self._works]
+
+    def get_many(self, keys):
+        return self._works
+
+
+class TestSeries:
+    def test_series_sorting(self, monkeypatch):
+        works = [
+            MockWork("/works/OL5W", "2"),
+            MockWork("/works/OL1W", "22-23"),
+            MockWork("/works/OL2W", "1"),
+            MockWork("/works/OL3W", "3"),
+            MockWork("/works/OL4W", "24-25"),
+            MockWork("/works/OL8W", "Non-numeric"),
+            MockWork("/works/OL7W", "4-5"),
+            MockWork("/works/OL6W", "1-3"),
+            MockWork("/works/OL9W", "1-9"),
+            MockWork("/works/OL10W", None),
+            MockWork("/works/OL11W", "3"),
+        ]
+
+        mock_site = MockSeriesSite(works)
+        monkeypatch.setattr(web, "ctx", web.storage(site=mock_site))
+
+        series = list_model.Series(mock_site, "/series/OL1L")
+        seeds = series.get_seeds()
+
+        result_keys = [seed.key for seed in seeds]
+        expected_keys = [
+            "/works/OL2W",  # 1
+            "/works/OL5W",  # 2
+            "/works/OL11W",  # 3 (tie-break by work key)
+            "/works/OL3W",  # 3
+            "/works/OL10W",  # None -> non-numeric bucket
+            "/works/OL8W",  # Non-numeric
+            "/works/OL7W",  # 4-5   (range size 1, lower 4)
+            "/works/OL1W",  # 22-23 (range size 1, lower 22)
+            "/works/OL4W",  # 24-25 (range size 1, lower 24)
+            "/works/OL6W",  # 1-3   (range size 2, lower 1)
+            "/works/OL9W",  # 1-9   (range size 8, lower 1)
+        ]
+
+        assert result_keys == expected_keys
+
+
+class TestWorkSortKey:
+    def test_numeric_position(self):
+        class DummyWork:
+            def __init__(self, key):
+                self.key = key
+
+        work = DummyWork("/works/OL1W")
+
+        key = list_model.get_work_sort_key((work, {"position": "2"}))
+        assert key == ("A: Numeric", 1, 2.0, "/works/OL1W")
+
+    def test_range_position(self):
+        class DummyWork:
+            def __init__(self, key):
+                self.key = key
+
+        work = DummyWork("/works/OL1W")
+
+        key = list_model.get_work_sort_key((work, {"position": "1-3"}))
+        assert key == ("C: Range", 2, 1.0, "/works/OL1W")
+
+    def test_non_numeric_position(self):
+        class DummyWork:
+            def __init__(self, key):
+                self.key = key
+
+        work = DummyWork("/works/OL1W")
+
+        key = list_model.get_work_sort_key((work, {"position": "abc"}))
+        assert key == ("B: Non-numeric", 0, 0.0, "/works/OL1W")
+
+    def test_none_position(self):
+        class DummyWork:
+            def __init__(self, key):
+                self.key = key
+
+        work = DummyWork("/works/OL1W")
+
+        key = list_model.get_work_sort_key((work, {"position": None}))
+        assert key == ("B: Non-numeric", 0, 0.0, "/works/OL1W")