isso icon indicating copy to clipboard operation
isso copied to clipboard

Add sorting option for comments

Open pkvach opened this issue 2 years ago • 1 comments

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

Checklist

  • [x] All new and existing tests are passing
  • [x] (If adding features:) I have added tests to cover my changes
  • [x] (If docs changes needed:) I have updated the documentation accordingly.
  • [x] I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • [x] 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 https://github.com/isso-comments/isso/issues/4

pkvach avatar Apr 02 '24 18:04 pkvach

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.

pkvach avatar Apr 28 '24 08:04 pkvach

This is good to go, if you could rebase and resolve the merge conflicts I would prefer to merge this before https://github.com/isso-comments/isso/pull/993 (since both PRs touch some of the same files)

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

ix5 avatar May 05 '24 21:05 ix5

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

pkvach avatar May 06 '24 06:05 pkvach

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]

ix5 avatar May 06 '24 20:05 ix5

@ix5 Do you plan to release this?

Kerumen avatar Aug 29 '24 17:08 Kerumen

@ix5 Do you plan to release this?

We should cut a release soon. I'd like to see https://github.com/isso-comments/isso/pull/1035 merged before I tag 0.13.1.dev1 and call for testing.

ix5 avatar Sep 09 '24 09:09 ix5