Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: Set reply sorting to 'oldest' #1035

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ggtylerr
Copy link

@ggtylerr ggtylerr commented Aug 24, 2024

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.

Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

isso/db/comments.py Outdated Show resolved Hide resolved
isso/db/comments.py Outdated Show resolved Hide resolved
isso/db/comments.py Outdated Show resolved Hide resolved
isso/db/comments.py Outdated Show resolved Hide resolved
@ggtylerr
Copy link
Author

ggtylerr commented Sep 5, 2024

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:
image

@ix5
Copy link
Member

ix5 commented Sep 9, 2024

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.

@ix5 ix5 changed the title Quick SQL patch to fix reply sorting server: Set reply sorting to 'oldest' Sep 9, 2024
@ix5 ix5 mentioned this pull request Sep 9, 2024
5 tasks
@ggtylerr
Copy link
Author

Fixed up as requested

@Kerumen
Copy link

Kerumen commented Oct 26, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Separate sorting for replies
3 participants