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 a sorted set for replies-to-this-post #5050

Merged
merged 10 commits into from Oct 31, 2016

Conversation

@BenLubar
Copy link
Contributor

commented Sep 24, 2016

No description provided.

@barisusakli

This comment has been minimized.

Copy link
Member

commented Sep 25, 2016

This is similar to https://github.com/NodeBB/nodebb-plugin-threaded-replies, we even used the same sorted set name

I am not against merging this into core although we need to add support for it in the default themes. There needs to be some UI to expand the replies on each post.

Also I don't think we should remove/add on post delete, it makes more sense to just remove the post from pid:<postData.toPid>:replies when the post is purged.

Another thing the plugin handles is post move, when a post is moved it is also removed from the replies sorted set.

@BenLubar

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2016

There needs to be some UI to expand the replies on each post.

I was thinking something like "2 replies" right next to where the "in reply to" shows up (at least in Persona - I haven't used the other themes), and if you click on it, you get a dialog similar to the "fork topic" dialog but with a list of links that let you go to the post and its replies. (But I want feedback on that idea before I implement it.)

Another thing the plugin handles is post move, when a post is moved it is also removed from the replies sorted set.

Replies still work when made in a different topic (example), so I'd rather keep them in the sorted set for moves.

Also I don't think we should remove/add on post delete, it makes more sense to just remove the post from pid:<postData.toPid>:replies when the post is purged.

It appears that replies still show up on deleted posts, so that's a good point. https://community.nodebb.org/post/37043

@BenLubar BenLubar force-pushed the BenLubar:replies-to-post branch from 3b2c852 to 0ed6801 Sep 25, 2016

@barisusakli

This comment has been minimized.

Copy link
Member

commented Sep 25, 2016

I think for the UI it would be better if we just have a clickable "X replies" box that expands/collapses the replies inline under the post.

Post A
   + 2 Replies

When clicked

Post A
   - 2 replies
    Post B
    Post C

Pretty sure @psychobunny will have some ideas as well.

Delete posts are just hidden but still visible to admins/mods so we need them in the sets, they should be removed when purged.

@BenLubar

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2016

Delete posts are just hidden but still visible to admins/mods so we need them in the sets, they should be removed when purged.

Yeah, I was thinking the "in reply to" thing was hidden on deleted posts, but it's visible, so there's no extra information being revealed on deleted posts. I updated the PR with the change from deletion to purging.

@barisusakli
Copy link
Member

left a comment

remove countReplies and store the reply count in the post object itself, this way there is no need to count replies for each post on topic load.

if (!postData.toPid) {
return next(null);
}
db.sortedSetAdd('pid:' + postData.toPid + ':replies', timestamp, postData.pid, next);

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

Can you change this to

async.parallel([
    async.apply(db.sortedSetAdd , 'pid:' + postData.toPid + ':replies', timestamp, postData.pid),
    async.apply(db.incrObjectField, 'post:' + postData.toPid, 'replies')
]. next);
if (!parseInt(toPid, 10)) {
return next(null);
}
db.sortedSetRemove('pid:' + toPid + ':replies', pid, next);

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

Change this to

async.parallel([
    async.apply(db.sortedSetRemove , 'pid:' + toPid + ':replies', pid),
    async.apply(db.decrObjectField, 'post:' + toPid, 'replies')
]. next);
@@ -66,6 +66,9 @@ module.exports = function(Topics) {
voteData: function(next) {
favourites.getVoteStatusByPostIDs(pids, uid, next);
},
replies: function(next) {
posts.countReplies(pids, next);

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

remove this since replies will be stored in the post object

@@ -124,6 +127,7 @@ module.exports = function(Topics) {
postObj.upvoted = results.voteData.upvotes[i];
postObj.downvoted = results.voteData.downvotes[i];
postObj.votes = postObj.votes || 0;
postObj.replies = results.replies[i] || 0;

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

Change this to postObj.replies = postObj.replies || 0;

@@ -10,7 +10,7 @@ var db = require('./database'),
schemaDate, thisSchemaDate,

// IMPORTANT: REMEMBER TO UPDATE VALUE OF latestSchema
latestSchema = Date.UTC(2016, 8, 22);
latestSchema = Date.UTC(2016, 8, 24);

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

Update upgrade date

if (!parseInt(post.toPid, 10)) {
return next(null);
}
db.sortedSetAdd('pid:' + postData.toPid + ':replies', postData.timestamp, postData.pid, next);

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

Change this to

async.parallel([
    async.apply(db.sortedSetAdd , 'pid:' + postData.toPid + ':replies', postData.timestamp, postData.pid),
    async.apply(db.incrObjectField, 'post:' + postData.toPid, 'replies')
]. next);
@@ -258,4 +258,29 @@ var plugins = require('./plugins');
});
};

Posts.countReplies = function(pid, callback) {

This comment has been minimized.

Copy link
@barisusakli

barisusakli Oct 14, 2016

Member

Can remove this function

@barisusakli barisusakli self-assigned this Oct 14, 2016

@barisusakli barisusakli added this to the 1.3.0 milestone Oct 14, 2016

@BenLubar BenLubar force-pushed the BenLubar:replies-to-post branch from 0ed6801 to bbfc52b Oct 14, 2016

BenLubar added a commit to BenLubar/NodeBB that referenced this pull request Oct 14, 2016

@BenLubar BenLubar force-pushed the BenLubar:replies-to-post branch from bbfc52b to a4ea453 Oct 14, 2016

BenLubar added a commit to BenLubar/NodeBB that referenced this pull request Oct 14, 2016

@BenLubar BenLubar force-pushed the BenLubar:replies-to-post branch from a4ea453 to 69ee813 Oct 27, 2016

@BenLubar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

@barisusakli

Ok, while testing this today (I'm planning to make the UI today), I got this error:

27/10 18:01 [31] - error: [upgrade] Errors were encountered while updating the NodeBB schema: E11000 duplicate key error index: 0.objects.$_key_1_value_-1 dup key: { : "post:26454", : null }

As far as I know, the only queries that ran that even touched something with that _key were:

posts.getPostsFields(ids, ['pid', 'toPid', 'timestamp'], function (err, data) {

which should be read-only and

async.apply(db.incrObjectField, 'post:' + postData.toPid, 'replies')

which shouldn't be inserting any new records into the database. In MongoDB console, db.objects.findOne({_key: "post:26454"}) shows this:

{
        "_id" : ObjectId("56de5945f848f35aaeb980ad"),
        "_key" : "post:26454",
        "pid" : 26454,
        "uid" : 12,
        "tid" : 698,
        "content" : "@Alex Papadimoulis said:<blockquote>As far as the logo -- the reason it's red is because it's supposted to the result of a Rubber Stamp. Like, at a code review, someone would stamp a big WTF on your code. Get it?\r\n<p>BTW, would anyone ever buy those ruber stamps? I got a quote a while back for like $12/ea ... with shipping and whatnot probably could get them to people for $15 - $20 ea. seems a bit steep.</p>\r\n<p>&nbsp;</blockquote></p><p>Yeah. Makes sense. Nice idea.</p>Custom-made stamps are expensive, because they can't be mass-produced: you don't buy 1,000 stamps; you buy 2 or 3, and they last for a pretty long time. It involves a bit of manual labour and that's more costly than running the process through a big machine that spits out 10,000 stamps an hour.<br><br>Multi-colour staps are even more expensive -- basically they're several stamps layed out in one stamp... thing.<br><br>Red Bic pens, however, cost nothing at all. :)<br>",
        "timestamp" : 1147238664436,
        "votes" : 0,
        "editor" : "",
        "edited" : 0,
        "deleted" : 0,
        "ip" : "[redacted]",
        "_imported_pid" : 144112,
        "_imported_uid" : 16,
        "_imported_tid" : 5606,
        "_imported_content" : "[quote user=\"Alex Papadimoulis\"]As far as the logo -- the reason it's red is because it's supposted to the result of a Rubber Stamp. Like, at a code review, someone would stamp a big WTF on your code. Get it?\r\n<p>BTW, would anyone ever buy those ruber stamps? I got a quote a while back for like $12/ea ... with shipping and whatnot probably could get them to people for $15 - $20 ea. seems a bit steep.</p>\r\n<p>&nbsp;[/quote]</p><p>Yeah. Makes sense. Nice idea.</p>Custom-made stamps are expensive, because they can't be mass-produced: you don't buy 1,000 stamps; you buy 2 or 3, and they last for a pretty long time. It involves a bit of manual labour and that's more costly than running the process through a big machine that spits out 10,000 stamps an hour.<br><br>Multi-colour staps are even more expensive -- basically they're several stamps layed out in one stamp... thing.<br><br>Red Bic pens, however, cost nothing at all. :)<br>",
        "_imported_cid" : 13,
        "_imported_ip" : "[redacted]",
        "_imported_guest" : "",
        "_imported_toPid" : 71994,
        "_imported_user_slug" : "",
        "_imported_user_path" : "/users/dhromed",
        "_imported_topic_slug" : "",
        "_imported_topic_path" : "",
        "_imported_category_path" : "/c/general",
        "_imported_category_slug" : "general",
        "_imported_path" : ""
}

I'm restoring the staging server's database from a backup, but I have no idea how that error message could even correspond to the queries being run.

@coveralls

This comment has been minimized.

Copy link

commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.003%) to 59.981% when pulling 69ee813 on BenLubar:replies-to-post into f97f45a on NodeBB:master.

@barisusakli

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

@BenLubar you can fix it by changing async.each to async.eachSeries

@barisusakli

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

It sounds like we need to do this check in incrObjectField as well https://github.com/NodeBB/NodeBB/blob/master/src/database/mongo/sorted.js#L21-L23

BenLubar added 3 commits Oct 27, 2016
@BenLubar BenLubar referenced this pull request Oct 27, 2016
@coveralls

This comment has been minimized.

Copy link

commented Oct 28, 2016

Coverage Status

Coverage decreased (-0.002%) to 59.982% when pulling 8121188 on BenLubar:replies-to-post into f97f45a on NodeBB:master.

@coveralls

This comment has been minimized.

Copy link

commented Oct 28, 2016

Coverage Status

Coverage decreased (-0.002%) to 59.982% when pulling 8121188 on BenLubar:replies-to-post into f97f45a on NodeBB:master.

@BenLubar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2016

@psychobunny

This comment has been minimized.

Copy link
Member

commented Oct 28, 2016

Niiice. Needs a touch more love, let me know if I can help

@BenLubar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2016

@psychobunny if you have any ideas, go ahead!

@barisusakli

This comment has been minimized.

Copy link
Member

commented Oct 28, 2016

This looks good, my only complaint is the icon is hardcoded to fa-plus, themes should be able to use something else if they want.

@barisusakli

This comment has been minimized.

Copy link
Member

commented Oct 28, 2016

Something else I noticed, the OP also gets a pid:<toPid>:replies, do we need this? Since all posts are already displayed under the OP, i think it doesn't make sense to have it.

@BenLubar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2016

A post can be a reply directly to the OP or a reply to the topic in general or a reply to another post in the topic (or even in a different topic if one of the posts was moved).

@coveralls

This comment has been minimized.

Copy link

commented Oct 28, 2016

Coverage Status

Coverage decreased (-0.002%) to 59.982% when pulling d74e2d0 on BenLubar:replies-to-post into f97f45a on NodeBB:master.

@BenLubar BenLubar force-pushed the BenLubar:replies-to-post branch from 85631bc to 661bdc8 Oct 28, 2016

@coveralls

This comment has been minimized.

Copy link

commented Oct 28, 2016

Coverage Status

Coverage decreased (-1.2%) to 58.749% when pulling 661bdc8 on BenLubar:replies-to-post into 2da3251 on NodeBB:master.

@coveralls

This comment has been minimized.

Copy link

commented Oct 28, 2016

Coverage Status

Coverage increased (+0.3%) to 60.286% when pulling 661bdc8 on BenLubar:replies-to-post into 2da3251 on NodeBB:master.

BenLubar added a commit to boomzillawtf/tdwtf that referenced this pull request Oct 30, 2016
Spooky Halloween NodeBB Update!
Highlights
----------

- Got tired of waiting, so some pull requests are locally merged as part of the Docker container build process:
  - Replies-to-this-post support <NodeBB/NodeBB#5050> <NodeBB/nodebb-theme-persona#329>
  - Ban UI fixes <NodeBB/NodeBB#5169>
    - Permanent bans assigned by global moderators no longer expire immediately (does not apply retroactively)
    - Bans with no reason given no longer have "undefined" as their reason (applies retroactively)
    - 0 is now a selectable value for "ban duration, select 0 for permanent"
  - Fixed jpegs being rotated incorrectly in thumbnails <NodeBB/nodebb-plugin-imagemagick#6>
- Accounts flagged as probable spammers by StopForumSpam are put into the registration queue.
- watchdog.bash can now kill a process if it fails to start up for a long period of time.

Change lists
------------

- NodeBB/NodeBB@d002c3e...1f10e0b
- NodeBB/nodebb-widget-essentials@8acf99a...c2aa33b
- julianlam/nodebb-plugin-sso-github@3fa44c6...f77da26
BenLubar added a commit to boomzillawtf/tdwtf that referenced this pull request Oct 30, 2016
Spooky Halloween NodeBB Update!
Highlights
----------

- Got tired of waiting, so some pull requests are locally merged as part of the Docker container build process:
  - Replies-to-this-post support <NodeBB/NodeBB#5050> <NodeBB/nodebb-theme-persona#329>
  - Ban UI fixes <NodeBB/NodeBB#5169>
    - Permanent bans assigned by global moderators no longer expire immediately (does not apply retroactively)
    - Bans with no reason given no longer have "undefined" as their reason (applies retroactively)
    - 0 is now a selectable value for "ban duration, select 0 for permanent"
  - Fixed jpegs being rotated incorrectly in thumbnails <NodeBB/nodebb-plugin-imagemagick#6>
- Accounts flagged as probable spammers by StopForumSpam are put into the registration queue.
- watchdog.bash can now kill a process if it fails to start up for a long period of time.

Change lists
------------

- NodeBB/NodeBB@d002c3e...1f10e0b
- NodeBB/nodebb-widget-essentials@8acf99a...c2aa33b
- julianlam/nodebb-plugin-sso-github@3fa44c6...f77da26

@barisusakli barisusakli merged commit 0c08e44 into NodeBB:master Oct 31, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 60.286%
Details
licence/cla Contributor License Agreement is signed.
Details
@barisusakli barisusakli referenced this pull request Oct 31, 2016
@barisusakli

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

@BenLubar I made some changes to the UI after the merge. Instead of using a different template I use the same post template from the topic.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.