fix: order responses numerically by PK instead of lexicographically#279
fix: order responses numerically by PK instead of lexicographically#279taimoor-ahmed-1 wants to merge 1 commit into
Conversation
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 openedx/frontend-app-discussions#823
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the pull request, @taimoor-ahmed-1! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Summary
Comment.get_list()sorted responses bysort_key, aCharFieldthat holds the comment's primary key rendered as a string. String comparison gives the lexicographic order\"1\", \"10\", \"11\", \"12\", \"2\", \"3\", …instead of the numeric1, 2, 3, …, 12. To users this looks as though the response sort dropdown does nothing — picking Newest first or Oldest first produces what appears to be a random shuffle.This change orders comments at the database layer by integer PK (
pkis monotonic with creation time, so PK ascending == oldest first).Fixes openedx/frontend-app-discussions#823.
Discuss thread (same user reporting): https://discuss.openedx.org/t/discuss-forum-messages-order-not-organized-as-expected-in-teak/18665
What changed
In `forum/backends/mysql/models.py`, `Comment.get_list()`:
Behaviour for the two documented inputs is preserved:
Why not fix `get_sort_key` to be int-safe instead?
`sort_key` is a denormalised `CharField` of the form `"{pk}"` or `"{parent_pk}-{pk}"`. Sorting by PK avoids the field entirely, removes a Python-level pass over every result, and pushes ordering to the database (so pagination via `[resp_skip:resp_end]` runs against an already-sorted queryset instead of a list slice).
Endorsed/answered responses
The reporter notes that endorsed/answer responses should remain on top regardless of sort. That separation is handled one layer up in edx-platform `lms/djangoapps/discussion/rest_api/api.py` for question threads (`endorsed_responses` + `non_endorsed_responses`). Not in scope for this fix.
Test plan
🤖 Generated with Claude Code