Jeffing doesn't update user bookmarks

boomzilla

Jeffing posts out of a topic (forking a topic in NodeBB) doesn't update user bookmarks, meaning that the last read position ends up much further down the thread than it actually is.

ben_lubar

We could rename this to "Jeffing doesn't work in forums not made by Jeff" and it'd be just as valid.

Then again, it doesn't work in Discourse either, but at least it's not this bad.

boomzilla

@ben_lubar said in Jeffing doesn't update user bookmarks:

We could rename this to "Jeffing doesn't work in forums not made by Jeff" and it'd be just as valid.

Nah. We can Jeff here.

ben_lubar

@boomzilla yesterday I wanted to jeff a flame war about Windows reboots into the Windows reboots thread and my only option was to move each post separately.

boomzilla

@ben_lubar Oh, yeah. I guess you can only move groups of posts into a new topic.

Tsaukpaetra

@ben_lubar said in Jeffing doesn't update user bookmarks:

a flame war about Windows reboots into the Windows reboots thread

Yeah, where did that go btw? I didn't get the notification about it and it certainly doesn't show in the thread,,,

Oh wait, you didn't jeff them yet? Well.... :whistles: 

anotherusername

@boomzilla can you merge two topics?

fbmac

This post is deleted!

boomzilla

@anotherusername said in Jeffing doesn't update user bookmarks:

@boomzilla can you merge two topics?

Yes, but as @ben_lubar pointed out, it's one post at a time.

aliceif

@boomzilla said in Jeffing doesn't update user bookmarks:

@anotherusername said in Jeffing doesn't update user bookmarks:

@boomzilla can you merge two topics?

Yes, but as @ben_lubar pointed out, it's one post at a time.

I pointed it out much earlier:
https://what.thedailywtf.com/topic/19248/moving-posts-a-chore

anotherusername

@boomzilla I meant merge them as one action. So then it sounds like the answer is "no".

My idea was that if you can jeff a bunch of posts to a new topic all at once, but not to an existing topic, then you could jeff them to a new topic and then just merge it into the existing topic. But it sounds like that isn't possible... sending posts to a different topic has to be done one-at-a-time unless they fix it.

Tsaukpaetra

@aliceif said in Jeffing doesn't update user bookmarks:

pointed

All these pointers!

And here I thought pointer logic was a Bad Idea (No idea why, it's just written there with no explanation...)

boomzilla

I think I have this mostly working...except it only updates one user. For some reason:

// tid = 5;
Topics.getTopicBookmarks( tid, next );

Only returns a single bookmark. But:

> db.objects.find( {_key:'tid:5:bookmarks'})
{ "_id" : ObjectId("570d1227eed5e44e0f7e8c0a"), "_key" : "tid:5:bookmarks", "value" : "38", "score" : 1 }
{ "_id" : ObjectId("570cef2beed5e44e0f7e8b41"), "_key" : "tid:5:bookmarks", "value" : "2", "score" : 33 }
{ "_id" : ObjectId("5707aa88c4ba72ece8a19f96"), "_key" : "tid:5:bookmarks", "value" : "1", "score" : 39 }

RaceProUK

@boomzilla Best guess: that Topics object is holding onto a user context

ben_lubar

@RaceProUK impossibru.

Here's the function with the closest name to the name @boomzilla posted:

NodeBB/src/topics.js at f55b26282a731e04b58034350303e4773d2d0c9a · NodeBB/NodeBB

Node.js based forum software built for the modern web - NodeBB/NodeBB

boomzilla

@RaceProUK It doesn't seem to be:

Topics.getTopicBookmarks = function( tid, callback ){
	db.getObjectsFields(['tid:' + tid + ':bookmarks'], ['value','score'], callback );
};

In fact, I wrote that function as part of this change. Of course, there are no comments, but the name and the code of getObjectsFields and the return value (an array) tell me that it's supposed to return multiple. The user id is stored as the value in the object, which getObjectsFields (src/database/mongo/hash.js) doesn't do anything special with (aside from fetching it because I said to).

Aside: It took me quite a while to figure out that "normal" node callbacks and the stuff you pass around in async have different signatures (i.e., how results and err and the callback show up or don't).

ben_lubar

@boomzilla but it's not an object - it's a sorted set.

ben_lubar

@boomzilla https://github.com/NodeBB/NodeBB/blob/v1.0.2/src/database/mongo/sorted.js#L121

boomzilla

@ben_lubar So...any idea what start and stop mean there?

ben_lubar

@boomzilla if you want all of them, set start to 0 and stop to -1.

boomzilla

Whew...check it out:

Apr 13, 2016

When forking a topic, updates user bookmarks in the topic to keep the… · boomzillawtf/NodeBB@c7a3f66

… last read position from inadvertently being too far down the topic due to post indices decreasing because some posts were moved to a new topic.

Now with more tests!

Tsaukpaetra

So here's a question: How does Jeffing affect links to specific posts?
I'm asking for a friend, because they seem to think that since posts are numbered only by their position in the thread (when linked), this will similarly irreversibly corrupt the linked post's link destination.

boomzilla

@Tsaukpaetra I think that it will certainly break quote links. I'm not sure about the reply link, but those might stay intact. Looking at the template, they seem to be using pid, which is a global post id.

boomzilla

@boomzilla said in Jeffing doesn't update user bookmarks:

Whew...check it out:

Hey noders (nodies?)! Am I starting to get the hang of this node stuff or am I still the node version of this guy? Because that's kind of how I feel. (@accalia @RaceProUK @Yamikuronue @ben_lubar ... and anyone else willing to admit noderdom)

ben_lubar

@boomzilla said in Jeffing doesn't update user bookmarks:

Hey noders

don't ask me! I'm a !

boomzilla

@ben_lubar said in Jeffing doesn't update user bookmarks:

don't ask me! I'm a !

Whatever with that, .

Yamikuronue

@boomzilla

it('should have 12 replies', function(done) {
 +			assert.equal( 12, replies.length );
 +			done();
 +		});

In this test, you're literally testing your precondition. Don't do that. Test the method under test.

The actual code uses the style I hate that Node standardized on, CPS, so I'll let one of the others chime in instead.

boomzilla

@Yamikuronue said in Jeffing doesn't update user bookmarks:

In this test, you're literally testing your precondition. Don't do that. Test the method under test.

Yes. Why not? I like to do this sort of thing because it makes it easier to spot why something is failing (when it does).

@Yamikuronue said in Jeffing doesn't update user bookmarks:

The actual code uses the style I hate that Node standardized on, CPS, so I'll let one of the others chime in instead.

Right. I mean...is there any other (reasonable) way to do it? I was largely asking if I was doing that bit correctly, because it still seems crazy and confusing to me.

Yamikuronue

@boomzilla said in Jeffing doesn't update user bookmarks:

is there any other (reasonable) way to do it?

Yeah, using Promises. I write all my code that way nowadays.

@boomzilla said in Jeffing doesn't update user bookmarks:

Why not?

Because there should already be a whole test suite to test replying to things. So you know replies work. Which would just make this test confusing as I try to read your mind about what you're actually testing and what was a precondition.

I suppose if you wanted to put an assertion at the end of your precondition block as a guard condition, that would be reasonable.

boomzilla

@Yamikuronue said in Jeffing doesn't update user bookmarks:

Yeah, using Promises. I write all my code that way nowadays.

Oh, right. I...recognize that name. Though in this case, since nodebb doesn't seem to use them, I guess I shouldn't be using them either.

@Yamikuronue said in Jeffing doesn't update user bookmarks:

Which would just make this test confusing as I try to read your mind about what you're actually testing and what was a precondition.

Hmm...I guess I can see that point of view.

Yamikuronue

@boomzilla said in Jeffing doesn't update user bookmarks:

, since nodebb doesn't seem to use them,

Yeah, you'd have to add an external dependency to get them in the versions of Node that they're building on. I have the benefit of being able to use ES6 which comes with Promise support natively.

anotherusername

@Tsaukpaetra said in Jeffing doesn't update user bookmarks:

irreversibly corrupt the linked post's link destination

yep.

@boomzilla said in Jeffing doesn't update user bookmarks:

I'm not sure about the reply link, but those might stay intact

Looking at the raw:

@boomzilla said in [Jeffing doesn&apos;t update user bookmarks](/topic/19651/jeffing-doesn-t-update-user-bookmarks/21):

...that /21 suggests to me that they'll all most certainly break.

Now, those  links at the tops of the posts... those might not break.

anotherusername

It'd be best to figure out some way to use the PID as the target... preferably thread-agnostic so that links to a post will still work if it's been jeffed to a different thread.

And it'd be nice if we could globally replace all of the existing links to posts with the new link-to-post syntax that won't break...

Tsaukpaetra

@anotherusername said in Jeffing doesn't update user bookmarks:

use the PID as the target

That sounds like an excellent use-case for anchors... Wait....

boomzilla

@Tsaukpaetra said in Jeffing doesn't update user bookmarks:

That sounds like an excellent use-case for anchors... Wait....

Sort of. It's more complicated than that because you can't guarantee that everything will be loaded on the page. Especially around here with the number of long threads.

It will probably require a new route or url pattern or whatever the kids are calling that these days.

Tsaukpaetra

@boomzilla said in Jeffing doesn't update user bookmarks:

Sort of. It's more complicated than that because you can't guarantee that everything will be loaded on the page. Especially around here with the number of long threads.
It will probably require a new route or url pattern or whatever the kids are calling that these days.

Yeah. My guess is that whatever method loading the page is doing may end up round-tripping to the server again if that particular post-id isn't found on the expected destination page.

It just gets more complicated from there...

julianlam

When a topic is forked, user bookmarks should be updated · Issue #4553 · NodeBB/NodeBB

I found a fork by @boomzillawtf @barisusakli perhaps a review is in order...

boomzilla

@julianlam Huh...thanks for resurrecting that. I guess I forgot about it while trying to ignore topics.

boomzilla

This post is deleted!

NedFodder

EDIT: Nevermind, I re-read the OP and it's the same thing I was talking about.

boomzilla

Accepted and merged!

When forking a topic, updates user bookmarks in the topic to keep the… by boomzillawtf · Pull Request #4554 · NodeBB/NodeBB

… last read position from inadvertently being too far down the topic due to post indices decreasing because some posts were moved to a new topic. Issue #4553