server: Set reply sorting to 'oldest'

[ggtylerr]

Checklist

• All new and existing tests are passing
• (If adding features:) I have added tests to cover my changes
• (If docs changes needed:) I have updated the documentation accordingly.
• I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
• I have written proper commit message(s)
  

What changes does this Pull Request introduce?

Quick SQL patch to fix #1026

Why is this necessary?

Current reply sorting leads to confusion. See linked issue for details.

NOTE: I tested this briefly and it seems to be working (and of course unit tests pass) but I wasn't sure if the sorting option was actually going through. Hard to tell on the demo page. Will probably test tmrw over curl.

[ix5]

Hi, thank you for the quick patch! Left a few inline comments, mainly regarding the usage of comments.id for sorting which I'd prefer you switch to comments.created for consistency.

[ggtylerr]

Just confirmed the patch to be working now - you can see the results on my site. Here's the comment I showed as an example last time:

[ix5]

Thank you for addressing the feedback. Please squash your commits and give the resulting commit a more descriptive title/body, such as db: Set reply sorting to 'oldest'.

You also checked in an updated package-lock.json to your commit when you didn't mean to, please remove that as well.

[ggtylerr]

Fixed up as requested

[Kerumen]

@ix5 do you plan to merge and release this? Thanks 🙏🏻

[ix5]

Sorry for leaving this in limbo for so long.
The test suite (predictably) fails for me:

FAILED isso/tests/test_comments.py::TestComments::testFeed - AssertionError: '"1-1"' != '"1-2"'
FAILED isso/tests/test_comments.py::TestComments::testGetSortedByNewestWithNested - AssertionError: Lists differ: [2, 3, 4, 5, 6] != [6, 5, 4, 3, 2]
FAILED isso/tests/test_comments.py::TestComments::testGetSortedByUpvotesWithNested - AssertionError: Lists differ: [2, 3, 4, 5, 6] != [6, 3, 2, 4, 5]

@ggtylerr would you mind updating the tests to reflect the new sorting? Thank you!

[jelmer]

Hi @ggtylerr any news on this?

[ggtylerr]

Hi, sorry, I didn't get any notification for this when ix5 replied for some reason. I'll update it whenever I'm off my shift today.

[ggtylerr]

I fetched upstream, cleared the merge conflict and updated the tests. Everything seems to work when I run it with nektos act, although I can't run isso itself since pip isn't letting me pull misaka.

One thing to note is that testGetSortedByNewestWithNested and testGetSortedByUpvotesWithNested now have the same order, due to them testing for the order of replies and not comments + replies. I consider this fine and these two still test whether or not the reply order is modified, but it's still something to note. Might need to modify to test that, or just have a separate test.

(Also I cannot for the life of me figure out how to squash nicely w/ the merge commit, I'm sorry 😢)

[Copilot]

Pull Request Overview

This PR changes the default sorting behavior for comment replies to always use "oldest first" ordering, addressing user confusion about reply sorting inconsistencies. The change affects both the database query logic and client-side behavior.

Key changes:

• Modified SQL queries to force replies to always sort by creation time in ascending order
• Updated tests to reflect the new oldest-first reply ordering
• Added documentation noting that replies are always sorted oldest-first as of version 0.13.1

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
isso/db/comments.py	Modified SQL ORDER BY clauses to ensure replies are always sorted by creation time
isso/tests/test_comments.py	Updated test expectations to match new oldest-first reply sorting behavior
docs/docs/reference/client-config.rst	Added documentation note about forced oldest-first reply sorting
CHANGES.rst	Added changelog entry for the reply sorting fix

[jelmer]

LGTM. I'll wait a few days to give @ix5 a chance to object. I'll merge otherwise.

[Kerumen]

@jelmer Do you plan to cut a release including this change?

[jelmer]

We should do another release at some point; first we should fix the failing tests though.

[ix5]

Sorry for the late reply, updated PR now LGTM.

@ggtylerr you can do a rebase including a squash of superfluous merge/editing commits, that's the way I'd recommend at least.

[ggtylerr]

@ggtylerr you can do a rebase including a squash of superfluous merge/editing commits, that's the way I'd recommend at least.

It's uh, already merged though 😅 I probably should've said to just use the squash + merge option in GH before it got merged, but it's whatever

[ix5]

It's uh, already merged though 😅 I probably should've said to just use the squash + merge option in GH before it got merged, but it's whatever

No worries, just format your updated commit history correctly next time. git rebase -i is a big help there.