From f4878e9bf197e8748954afb7a3188194ab5b1673 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Sat, 6 Jun 2026 15:55:06 +0500 Subject: [PATCH] fix: order responses numerically by PK instead of lexicographically Comment.get_list() previously sorted by `sort_key`, a CharField that holds the comment's primary key rendered as a string. String sort gave the order "1", "10", "11", ..., "2", "3", ..., which looks to users as if response sorting is not being applied at all. Switch to ordering at the database layer by integer PK (monotonic with creation time): `.order_by("pk")` for ascending (Oldest first) and `.order_by("-pk")` for descending (Newest first). This also fixes a latent bug where, with `sort=None` and `resp_limit` set, pagination sliced into an empty `result` list and returned no rows. Fixes https://github.com/openedx/frontend-app-discussions/issues/823 Co-Authored-By: Claude Opus 4.7 --- forum/backends/mysql/models.py | 43 +++++++++++++++++----------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/forum/backends/mysql/models.py b/forum/backends/mysql/models.py index 211d1ff5..02aa97c8 100644 --- a/forum/backends/mysql/models.py +++ b/forum/backends/mysql/models.py @@ -403,28 +403,29 @@ def get_list(**kwargs: Any) -> list[dict[str, Any]]: sort = kwargs.pop("sort", None) resp_skip = kwargs.pop("resp_skip", 0) resp_limit = kwargs.pop("resp_limit", None) - comments = Comment.objects.filter(**kwargs) - result = [] - if sort: - if sort == 1: - result = sorted( - comments, key=lambda x: (x.sort_key is None, x.sort_key or "") - ) - elif sort == -1: - result = sorted( - comments, - key=lambda x: (x.sort_key is None, x.sort_key or ""), - reverse=True, - ) - - paginated_comments = result or list(comments) - - # Apply pagination if resp_limit is provided + comments_qs = Comment.objects.filter(**kwargs) + + # Order by primary key, which is monotonically increasing with creation + # time. The previous implementation sorted by `sort_key`, a CharField + # containing the PK rendered as a string ("10" < "2" lexicographically), + # which produced an interleaved order that looked like sorting was not + # being applied at all. + # See https://github.com/openedx/frontend-app-discussions/issues/823 + if sort == -1: + comments_qs = comments_qs.order_by("-pk") + else: + # Treat sort == 1 and the unsorted/None case the same way: ascending + # by PK. The legacy "no sort" path silently returned no rows when + # `resp_limit` was set (because pagination indexed into an empty + # `result` list); using a deterministic order fixes that too. + comments_qs = comments_qs.order_by("pk") + if resp_limit is not None: - resp_end = resp_skip + resp_limit - paginated_comments = result[resp_skip:resp_end] - elif resp_skip: # If resp_limit is None but resp_skip is provided - paginated_comments = result[resp_skip:] + paginated_comments = list(comments_qs[resp_skip:resp_skip + resp_limit]) + elif resp_skip: + paginated_comments = list(comments_qs[resp_skip:]) + else: + paginated_comments = list(comments_qs) return [content.to_dict() for content in paginated_comments]