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

Add sorting option for comments #1005

Merged
merged 1 commit into from
May 6, 2024

Conversation

pkvach
Copy link
Contributor

@pkvach pkvach commented Apr 2, 2024

Add sorting and pagination support for comments, including new API query parameters for sort order and offset. Adjust comment fetching logic accordingly.

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?

  • Added sorting and pagination support for comments in both backend and frontend.
  • Introduced new API query parameters for sorting (sort) and pagination (offset).
  • Updated the client configuration documentation to include the new data-isso-sorting option.
  • Enhanced the comments fetching logic to support sorting by newest, oldest, and upvotes, as well as pagination through offset and limit.
  • Added unit tests to verify the functionality of sorting and pagination.

Why is this necessary?

Closes #4

@pkvach pkvach force-pushed the feat/add-sorting-for-comments branch from 63d893b to ec0283b Compare April 2, 2024 18:35
@pkvach pkvach force-pushed the feat/add-sorting-for-comments branch from ec0283b to d2ed180 Compare April 19, 2024 04:57
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.

Nice work, you're being very diligent while trying to keep the diff to a minimum and not break existing things.

A few comments inline, mostly related to a (server-side) default for sort and JS readability.

As for the after parameter, with the switch to offset it isn't needed by the frontend anymore. Did you keep it for backward compatibility? I could imagine a website admin only wanting to display recent comments (after = today-7days) but they'd have to monkey-patch or recompile the embed JS to do that.

I know this is much to ask, but could you please add Jest JS tests as well? Successively loading hidden comments is not tested at all at the moment (most of the still-few tests were written in a mad frenzy by me while trying to learn JS and Jest, so this is coming to bite us back).

docs/docs/reference/client-config.rst Outdated Show resolved Hide resolved
isso/js/app/isso.js Outdated Show resolved Hide resolved
isso/js/embed.js Outdated Show resolved Hide resolved
isso/views/comments.py Outdated Show resolved Hide resolved
isso/views/comments.py Outdated Show resolved Hide resolved
isso/js/app/api.js Outdated Show resolved Hide resolved
docs/docs/reference/client-config.rst Outdated Show resolved Hide resolved
isso/views/comments.py Outdated Show resolved Hide resolved
isso/db/comments.py Show resolved Hide resolved
@pkvach
Copy link
Contributor Author

pkvach commented Apr 28, 2024

Nice work, you're being very diligent while trying to keep the diff to a minimum and not break existing things.

Thank you for your feedback.

As for the after parameter, with the switch to offset it isn't needed by the frontend anymore. Did you keep it for backward compatibility? I could imagine a website admin only wanting to display recent comments (after = today-7days) but they'd have to monkey-patch or recompile the embed JS to do that.

Yes, that's correct: after is no longer used in frontend requests. Maybe someone uses it, so I left it for backward compatibility.

I know this is much to ask, but could you please add Jest JS tests as well? Successively loading hidden comments is not tested at all at the moment (most of the still-few tests were written in a mad frenzy by me while trying to learn JS and Jest, so this is coming to bite us back).

Sure, I will try to add Jest tests.
The only thing I've noticed is that they can be unstable when running tests: they fail with a timeout error in different tests.

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.

Thank you for incorporating the feedback.

The only thing I've noticed is that they can be unstable when running tests: they fail with a timeout error in different tests.

I've pushed a PR that should hopefully address this: #1021

isso/views/comments.py Outdated Show resolved Hide resolved
docs/docs/reference/client-config.rst Outdated Show resolved Hide resolved
@ix5
Copy link
Member

ix5 commented May 5, 2024

This is good to go, if you could rebase and resolve the merge conflicts I would prefer to merge this before #993 (since both PRs touch some of the same files)

Edit: (Jest tests can be added in another PR)

@pkvach pkvach force-pushed the feat/add-sorting-for-comments branch from a8e40dc to cf4cedd Compare May 6, 2024 06:25
Add sorting and pagination support for comments, including new API query parameters for sort order and offset. Adjust comment fetching logic accordingly.

Closes isso-comments#4
@pkvach pkvach force-pushed the feat/add-sorting-for-comments branch from cf4cedd to 00fc383 Compare May 6, 2024 06:30
@pkvach
Copy link
Contributor Author

pkvach commented May 6, 2024

@ix5
Thank you for the review.
I have fixed the last comments about the API docs and prepared the changes for merge.

@ix5 ix5 merged commit ca91601 into isso-comments:master May 6, 2024
20 checks passed
@ix5
Copy link
Member

ix5 commented May 6, 2024

Tested locally as well with the demo page and I couldn't find any obvious flaws.

Merged, thanks again for applying the feedback.


For local testing/validating, I found this easy to quickly manipulate demo data with votes:

diff --git a/isso/db/__init__.py b/isso/db/__init__.py
index e7209fe..d1ebb52 100644
--- a/isso/db/__init__.py
+++ b/isso/db/__init__.py
@@ -57,6 +57,7 @@ class SQLite3:
             sql = ' '.join(sql)
 
         with sqlite3.connect(self.path) as con:
+            con.set_trace_callback(logger.info)
             return con.execute(sql, args)
 
     @property
diff --git a/isso/demo/index.html b/isso/demo/index.html
index 1577253..61c804c 100644
--- a/isso/demo/index.html
+++ b/isso/demo/index.html
@@ -10,6 +10,10 @@
    <h2><a href=".">Isso Demo</a></h2>
 
    <script src="../js/embed.dev.js"
+           data-isso-sorting="upvotes" # or "newest" ...
+           data-isso-max-comments-top="1"
+           data-isso-max-comments-nested="2"
+           data-isso-reveal-on-click="2"
            data-isso="../"
            ></script>
    <!-- Uncomment to only test count functionality: -->
diff --git a/isso/views/comments.py b/isso/views/comments.py
index 9352dd0..933720d 100644
--- a/isso/views/comments.py
+++ b/isso/views/comments.py
@@ -409,6 +409,10 @@ class API(object):
 
         Recipe source: https://stackoverflow.com/a/22936947/636849
         """
+
+        import random
+        return ".".join(4 * (str(random.randint(0, 255)),))
+
         remote_addr = request.remote_addr
         if self.trusted_proxies:
             route = request.access_route + [remote_addr]

@pkvach pkvach deleted the feat/add-sorting-for-comments branch May 7, 2024 05:29
@Kerumen
Copy link

Kerumen commented Aug 29, 2024

@ix5 Do you plan to release this?

@ix5
Copy link
Member

ix5 commented Sep 9, 2024

@ix5 Do you plan to release this?

We should cut a release soon. I'd like to see #1035 merged before I tag 0.13.1.dev1 and call for testing.

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] Sort by newest/highest votes
3 participants