From cf4cedd55162a656636c8bae7bb7ce2e44b0a261 Mon Sep 17 00:00:00 2001 From: Pavel Kvach Date: Tue, 2 Apr 2024 21:14:50 +0300 Subject: [PATCH] Add sorting option for comments Add sorting and pagination support for comments, including new API query parameters for sort order and offset. Adjust comment fetching logic accordingly. Closes https://github.com/isso-comments/isso/issues/4 --- CHANGES.rst | 2 + docs/docs/reference/client-config.rst | 16 ++++ isso/db/comments.py | 19 ++-- isso/js/app/api.js | 12 +-- isso/js/app/default_config.js | 1 + isso/js/app/isso.js | 47 +++++----- isso/js/embed.js | 20 ++-- isso/js/tests/unit/comment.test.js | 2 +- isso/tests/test_comments.py | 128 ++++++++++++++++++++++++++ isso/views/comments.py | 36 +++++++- 10 files changed, 229 insertions(+), 54 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index b6954ca14..f85a6470a 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,12 +12,14 @@ New Features - Add search for comments by URL in the admin interface (`#1000`_, pkvach) - Add CSS variables for better organization and flexibility (`#1001`_, pkvach) - Add support for comment search by Thread URL in admin interface (`#1020`_, pkvach) +- Add sorting option for comments (`#1005`_, pkvach) .. _#966: https://github.com/posativ/isso/pull/966 .. _#998: https://github.com/isso-comments/isso/pull/998 .. _#1000: https://github.com/isso-comments/isso/pull/1000 .. _#1001: https://github.com/isso-comments/isso/pull/1001 .. _#1020: https://github.com/isso-comments/isso/pull/1020 +.. _#1005: https://github.com/isso-comments/isso/pull/1005 Breaking Changes ^^^^^^^^^^^^^^^^ diff --git a/docs/docs/reference/client-config.rst b/docs/docs/reference/client-config.rst index 40a1e2679..b940adf7a 100644 --- a/docs/docs/reference/client-config.rst +++ b/docs/docs/reference/client-config.rst @@ -13,6 +13,7 @@ preferably in the script tag which embeds the JS: data-isso-max-comments-top="10" data-isso-max-comments-nested="5" data-isso-reveal-on-click="5" + data-isso-sorting="newest" data-isso-avatar="true" data-isso-avatar-bg="#f0f0f0" data-isso-avatar-fg="#9abf88 #5698c4 #e279a3 #9163b6 ..." @@ -255,6 +256,21 @@ data-isso-reply-notifications-default-enabled .. versionadded:: 0.13 +.. _data-isso-sorting: + +data-isso-sorting + Sort thread comments by specified sorting method. + + Possible sorting methods: + + - ``newest``: Bring newest comments to the top + - ``oldest``: Bring oldest comments to the top + - ``upvotes``: Bring most liked comments to the top + + Default sorting is ``oldest``. + + .. versionadded:: 0.13.1 + Deprecated Client Settings -------------------------- diff --git a/isso/db/comments.py b/isso/db/comments.py index c41c77fe7..48cbbfe69 100644 --- a/isso/db/comments.py +++ b/isso/db/comments.py @@ -238,11 +238,11 @@ def fetchall(self, mode=5, after=0, parent='any', order_by='id', yield dict(zip(fields_comments + fields_threads, item)) def fetch(self, uri, mode=5, after=0, parent='any', - order_by='id', asc=1, limit=None): + order_by='id', asc=1, limit=None, offset=0): """ Return comments for :param:`uri` with :param:`mode`. """ - sql = ['SELECT comments.* FROM comments INNER JOIN threads ON', + sql = ['SELECT comments.*, likes - dislikes AS karma FROM comments INNER JOIN threads ON', ' threads.uri=? AND comments.tid=threads.id AND (? | comments.mode) = ?', ' AND comments.created>?'] @@ -256,14 +256,18 @@ def fetch(self, uri, mode=5, after=0, parent='any', sql_args.append(parent) # custom sanitization - if order_by not in ['id', 'created', 'modified', 'likes', 'dislikes']: + if order_by not in ['id', 'created', 'modified', 'likes', 'dislikes', 'karma']: order_by = 'id' sql.append('ORDER BY ') sql.append(order_by) if not asc: sql.append(' DESC') - if limit: + if offset and limit: + sql.append('LIMIT ?,?') + sql_args.append(offset) + sql_args.append(limit) + elif limit: sql.append('LIMIT ?') sql_args.append(limit) @@ -350,7 +354,7 @@ def vote(self, upvote, id, remote_addr): return {'likes': likes + 1, 'dislikes': dislikes} return {'likes': likes, 'dislikes': dislikes + 1} - def reply_count(self, url, mode=5, after=0): + def reply_count(self, url, mode=5): """ Return comment count for main thread and all reply threads for one url. """ @@ -358,11 +362,10 @@ def reply_count(self, url, mode=5, after=0): sql = ['SELECT comments.parent,count(*)', 'FROM comments INNER JOIN threads ON', ' threads.uri=? AND comments.tid=threads.id AND', - ' (? | comments.mode = ?) AND', - ' comments.created > ?', + ' (? | comments.mode = ?)', 'GROUP BY comments.parent'] - return dict(self.db.execute(sql, [url, mode, mode, after]).fetchall()) + return dict(self.db.execute(sql, [url, mode, mode]).fetchall()) def count(self, *urls): """ diff --git a/isso/js/app/api.js b/isso/js/app/api.js index 90efa4926..540b67125 100644 --- a/isso/js/app/api.js +++ b/isso/js/app/api.js @@ -138,17 +138,13 @@ var view = function(id, plain) { return deferred.promise; }; -var fetch = function(tid, limit, nested_limit, parent, lastcreated) { - if (typeof(limit) === 'undefined') { limit = "inf"; } - if (typeof(nested_limit) === 'undefined') { nested_limit = "inf"; } - if (typeof(parent) === 'undefined') { parent = null; } +var fetch = function({ tid, limit = "inf", nested_limit = "inf", parent = null, sort = "", offset = 0 }) { + var query_dict = { uri: tid || location(), sort, parent, offset }; - var query_dict = {uri: tid || location(), after: lastcreated, parent: parent}; - - if(limit !== "inf") { + if (limit !== "inf") { query_dict['limit'] = limit; } - if(nested_limit !== "inf"){ + if (nested_limit !== "inf") { query_dict['nested_limit'] = nested_limit; } diff --git a/isso/js/app/default_config.js b/isso/js/app/default_config.js index 68dd9fecc..6a872aedf 100644 --- a/isso/js/app/default_config.js +++ b/isso/js/app/default_config.js @@ -13,6 +13,7 @@ var default_config = { "max-comments-top": "inf", "max-comments-nested": 5, "reveal-on-click": 5, + "sorting": "oldest", "gravatar": false, "avatar": true, "avatar-bg": "#f0f0f0", diff --git a/isso/js/app/isso.js b/isso/js/app/isso.js index fbf958a3d..83ac00f9a 100644 --- a/isso/js/app/isso.js +++ b/isso/js/app/isso.js @@ -114,7 +114,7 @@ var Postbox = function(parent) { notification: $("[name=notification]", el).checked() ? 1 : 0, }).then(function(comment) { $(".isso-textarea", el).value = ""; - insert(comment, true); + insert({ comment, scrollIntoView: true, offset: 0 }); if (parent !== null) { el.onsuccess(); @@ -125,7 +125,7 @@ var Postbox = function(parent) { return el; }; -var insert_loader = function(comment, lastcreated) { +var insert_loader = function(comment, offset) { var entrypoint; if (comment.id === null) { entrypoint = $("#isso-root"); @@ -140,34 +140,37 @@ var insert_loader = function(comment, lastcreated) { $("a.isso-load-hidden", el).on("click", function() { el.remove(); - api.fetch($("#isso-thread").getAttribute("data-isso-id"), - config["reveal-on-click"], config["max-comments-nested"], - comment.id, - lastcreated).then( + + api.fetch({ + tid: $("#isso-thread").getAttribute("data-isso-id"), + limit: config["reveal-on-click"], + nested_limit: config["max-comments-nested"], + parent: comment.id, + sort: config["sorting"], + offset: offset + }).then( function(rv) { if (rv.total_replies === 0) { return; } - var lastcreated = 0; rv.replies.forEach(function(commentObject) { - insert(commentObject, false); - if(commentObject.created > lastcreated) { - lastcreated = commentObject.created; - } + insert({ comment: commentObject, scrollIntoView: false, offset: 0 }); + }); if(rv.hidden_replies > 0) { - insert_loader(rv, lastcreated); + insert_loader(rv, offset + rv.replies.length); } }, function(err) { console.log(err); - }); + } + ); }); }; -var insert = function(comment, scrollIntoView) { +var insert = function({ comment, scrollIntoView, offset }) { var el = $.htmlify(template.render("comment", {"comment": comment})); // update datetime every 60 seconds @@ -381,19 +384,13 @@ var insert = function(comment, scrollIntoView) { show($("a.isso-reply", footer).detach()); } - if(comment.hasOwnProperty('replies')) { - var lastcreated = 0; - comment.replies.forEach(function(replyObject) { - insert(replyObject, false); - if(replyObject.created > lastcreated) { - lastcreated = replyObject.created; - } - + if (comment.hasOwnProperty('replies')) { + comment.replies.forEach(function (replyObject) { + insert({ comment: replyObject, scrollIntoView: false, offset: offset + 1 }); }); - if(comment.hidden_replies > 0) { - insert_loader(comment, lastcreated); + if (comment.hidden_replies > 0) { + insert_loader(comment, offset + comment.replies.length); } - } }; diff --git a/isso/js/embed.js b/isso/js/embed.js index 1c1cfb8e3..dd7b06afb 100644 --- a/isso/js/embed.js +++ b/isso/js/embed.js @@ -122,9 +122,15 @@ function fetchComments() { } isso_root.textContent = ''; - api.fetch(isso_thread.getAttribute("data-isso-id") || location.pathname, - config["max-comments-top"], - config["max-comments-nested"]).then( + + api.fetch({ + tid: isso_thread.getAttribute("data-isso-id") || location.pathname, + limit: config["max-comments-top"], + nested_limit: config["max-comments-nested"], + parent: null, + sort: config["sorting"], + offset: 0 + }).then( function (rv) { if (rv.total_replies === 0) { @@ -132,18 +138,14 @@ function fetchComments() { return; } - var lastcreated = 0; var count = rv.total_replies; rv.replies.forEach(function(comment) { - isso.insert(comment, false); - if (comment.created > lastcreated) { - lastcreated = comment.created; - } + isso.insert({ comment: comment, scrollIntoView: false, offset: 0 }); }); heading.textContent = i18n.pluralize("num-comments", count); if (rv.hidden_replies > 0) { - isso.insert_loader(rv, lastcreated); + isso.insert_loader(rv, rv.replies.length); } if (window.location.hash.length > 0 && diff --git a/isso/js/tests/unit/comment.test.js b/isso/js/tests/unit/comment.test.js index 2e8de7e79..4c90629e1 100644 --- a/isso/js/tests/unit/comment.test.js +++ b/isso/js/tests/unit/comment.test.js @@ -58,7 +58,7 @@ test('Rendered comment should match snapshot', () => { var isso_thread = $('#isso-thread'); isso_thread.append('
'); - isso.insert(comment, false); + isso.insert({ comment, scrollIntoView: false, offset: 0 }); // Will create a `.snap` file in `./__snapshots__/`. // Don't forget to check in those files when changing anything! diff --git a/isso/tests/test_comments.py b/isso/tests/test_comments.py index 8f6b4579c..a3989c98f 100644 --- a/isso/tests/test_comments.py +++ b/isso/tests/test_comments.py @@ -247,6 +247,134 @@ def testGetLimitedNested(self): self.assertEqual(len(rv['replies']), 10) self.assertEqual(rv['total_replies'], 20) + def testGetWithOffset(self): + for i in range(5): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + + r = self.get('/?uri=test&limit=3&offset=2') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + self.assertEqual( + [comment['id'] for comment in rv['replies']], + [3, 4, 5] + ) + + def testGetWithOffsetIgnoredWithoutLimit(self): + for i in range(5): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + + r = self.get('/?uri=test&offset=2') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + self.assertEqual( + [comment['id'] for comment in rv['replies']], + [1, 2, 3, 4, 5] + ) + + def testGetNestedWithOffset(self): + + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + for i in range(5): + self.post('/new?uri=test', + data=json.dumps({'text': '...', 'parent': 1})) + + r = self.get('/?uri=test&parent=1&limit=3&offset=2') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + self.assertEqual( + [comment['id'] for comment in rv['replies']], + [4, 5, 6] + ) + + def testGetSortedByOldest(self): + for i in range(5): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + + r = self.get('/?uri=test&sort=oldest') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + # assert order of comments is oldest first + self.assertEqual( + [comment['id'] for comment in rv['replies']], + [1, 2, 3, 4, 5] + ) + + def testGetSortedByNewest(self): + for i in range(5): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + + r = self.get('/?uri=test&sort=newest') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + # assert order of comments is newest first + self.assertEqual( + [comment['id'] for comment in rv['replies']], + [5, 4, 3, 2, 1] + ) + + def testGetSortedByUpvotes(self): + for i in range(5): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + + # update the likes for some comments + self.app.db.execute( + 'UPDATE comments SET likes = id WHERE id IN (2, 4)' + ) + + r = self.get('/?uri=test&sort=upvotes') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + # assert order of comments is by upvotes + self.assertEqual( + [comment['id'] for comment in rv['replies']], + [4, 2, 1, 3, 5] + ) + + def testGetSortedByNewestWithNested(self): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + for i in range(5): + self.post('/new?uri=test', + data=json.dumps({'text': '...', 'parent': 1})) + + r = self.get('/?uri=test&sort=newest') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + self.assertEqual(len(rv['replies']), 1) + # assert order of nested comments is newest first + self.assertEqual( + [comment['id'] for comment in rv['replies'][0]['replies']], + [6, 5, 4, 3, 2] + ) + + def testGetSortedByUpvotesWithNested(self): + self.post('/new?uri=test', data=json.dumps({'text': '...'})) + for i in range(5): + self.post('/new?uri=test', + data=json.dumps({'text': '...', 'parent': 1})) + + # update the likes for some comments + self.app.db.execute( + 'UPDATE comments SET likes = id WHERE id IN (3, 6)' + ) + + r = self.get('/?uri=test&sort=upvotes') + self.assertEqual(r.status_code, 200) + + rv = loads(r.data) + self.assertEqual(len(rv['replies']), 1) + # assert order of nested comments is newest first + self.assertEqual( + [comment['id'] for comment in rv['replies'][0]['replies']], + [6, 3, 2, 4, 5] + ) + def testUpdate(self): self.post('/new?uri=%2Fpath%2F', diff --git a/isso/views/comments.py b/isso/views/comments.py index 6f3545566..83e1b2241 100644 --- a/isso/views/comments.py +++ b/isso/views/comments.py @@ -809,7 +809,7 @@ def moderate(self, environ, request, id, action, key): @api {get} / Get comments @apiGroup Thread @apiName fetch - @apiVersion 0.12.6 + @apiVersion 0.13.1 @apiDescription Queries the publicly visible comments of a thread. @apiQuery {String} uri @@ -823,6 +823,10 @@ def moderate(self, environ, request, id, action, key): The maximum number of returned nested comments per comment. Omit for unlimited results. @apiQuery {Number} [after] Includes only comments were added after the provided UNIX timestamp. + @apiQuery {String} [sort] + The sorting order of the comments. Possible values are `newest`, `oldest`, `upvotes`. if omitted, default sort order will be `oldest`. + @apiQuery {Number} [offset] + Offset the returned comments by this number. Used for pagination. Works only in combination with `limit`. @apiSuccess {Number} id Id of the comment `replies` is the list of replies of. `null` for the list of top-level comments. @@ -899,6 +903,23 @@ def fetch(self, environ, request, uri): 'after': request.args.get('after', 0) } + # map sort query parameter + valid_sort_options = ['newest', 'oldest', 'upvotes'] + sort = request.args.get('sort', 'oldest') + + if sort not in valid_sort_options: + return BadRequest("Invalid sort option. Must be one of: 'newest', 'oldest', 'upvotes'") + + if sort == 'newest': + args['order_by'] = 'created' + args['asc'] = 0 + elif sort == 'oldest': + args['order_by'] = 'created' + args['asc'] = 1 + elif sort == 'upvotes': + args['order_by'] = 'karma' + args['asc'] = 0 + try: args['limit'] = int(request.args.get('limit')) except TypeError: @@ -906,6 +927,13 @@ def fetch(self, environ, request, uri): except ValueError: return BadRequest("limit should be integer") + try: + args['offset'] = int(request.args.get('offset', 0)) + if args['offset'] < 0: + return BadRequest("offset should not be negative") + except (ValueError, TypeError): + return BadRequest("offset should be integer") + if request.args.get('parent') is not None: try: args['parent'] = int(request.args.get('parent')) @@ -918,7 +946,7 @@ def fetch(self, environ, request, uri): plain = request.args.get('plain', '0') == '0' - reply_counts = self.comments.reply_count(uri, after=args['after']) + reply_counts = self.comments.reply_count(uri) if args['limit'] == 0: root_list = [] @@ -941,7 +969,7 @@ def fetch(self, environ, request, uri): rv = { 'id': root_id, 'total_replies': total_replies, - 'hidden_replies': reply_counts[root_id] - len(root_list), + 'hidden_replies': reply_counts[root_id] - len(root_list) - args['offset'], 'replies': self._process_fetched_list(root_list, plain), 'config': self.public_conf } @@ -954,6 +982,8 @@ def fetch(self, environ, request, uri): if nested_limit > 0: args['parent'] = comment['id'] args['limit'] = nested_limit + # Reset offset to 0 for nested comments to ensure correct pagination + args['offset'] = 0 replies = list(self.comments.fetch(**args)) else: replies = []