FEATURE: Make topic-level bookmarks different from OP post bookmarks (PR #14272)

This commit introduces the concept of topic-level bookmarks as a separate concept from what they previously were, which was a bookmark on the OP of the topic. This is done by passing post ID of -1 to a bookmark when it is created. This necessitates changing the bookmark creation checks and unique index to not just be on the post ID, but on the user ID, post ID, and topic ID as well.

This requires some duplication on the topic model in JavaScript to have the same methods as the post model, which are slightly different to one another, for interacting with bookmarks.

GitHub

Need to move this magic number into some other place in JS

Need test

Need test

Ember CLI failures are unrelated

I wonder if we just pass nil to post_id here? require topic_id / post_id here.

from the JSON API we should simply omit sending the post_id. is there a away we can pull this off?

Hmm I think you have a point. Let me try that way out, it will be clearer than a magic number.

This is ugly but I am not really sure how else to do this…

Couldn’t figure out a way to use Post.secured still and be able to do this so I just extracted it here

hmmm so this will change to the “last read” post later?

Yes those changes will be in the next PR, this is mainly so the user bookmark list doesn’t blow up. However that does bring up the question: When showing and searching topic-level bookmarks in the user bookmark list, should we be using the first post content, or the last unread post content? If the latter searching in https://github.com/discourse/discourse/pull/14272/files/45019bea922e24db615180c419e92ce8af0889a8#diff-0f7c6a210ef52b10b632c103844a35108a837501df63a47d2f7e077cf95b878f is a bit more complex

      post_id = params[:post_id].to_i

I don’t think we need the lonely operator here since we’ve already checked that the param is not blank.

:+1: that was leftover from some reshuffling

Am I right to say that if post_id is present, topic_id will be nil and vice versa? If that is the case, shouldn’t we maintain two index of user_id, topic_id and user_id, post_id instead?

Will bookmark be blank if we’re already checked that bookmarked is true?

we would need to make them filtered for them to be unique , but yeah that can work.

one index for user_id,topic_id where post_id is null … unique one index for user_id,post_id where topic_id is null … unique

I guess one big question is if we are relying on de-normalization here for any of the queries eg bookmark post X in topic Y, then move post X to topic M…

Good point!

No, topic ID will always be present, only post_id is nullable. So this index should work for:

user_id: 1, topic_id: 99, post_id: 123

and

user_id: 1, topic_id: 99, post_id: 124

and

user_id: 1, topic_id: 99, post_id: NULL

Right?

Same here, we’ve already checked that the topic is bookmarked.

Same here as well, this attribute is not included unless bookmarked