FEATURE: Topic-level bookmarks (PR #14353)

Allows creating a bookmark with the for_topic flag introduced in https://github.com/discourse/discourse/commit/d1d2298a4cee0b021db66ff116a665ee8ae27111 set to true. This happens when clicking on the Bookmark button in the topic footer when no other posts are bookmarked.

I had to do some pretty heavy refactors because most of the bookmark code in the JS topics controller was centered around instances of Post JS models, but the topic level bookmark is not centered around a post. Some refactors were just for readability as well.

Also removes some missed reminderType code from the purge in https://github.com/discourse/discourse/commit/41e19adb0d7685303009650efec3ebec0319c577

GitHub

Absolutely no idea how I missed this in the reminder type PR

Trivial but there isn’t much point in making a new variable to only use it once.

The code looks good, although I did notice that from a JS point of view we are only editing existing tests and not adding any new ones. Surely this introduces some new functionality that could use at least one new test?

I’m approving but I wouldn’t mind seeing a follow up PR with more tests!

LGTM overall :+1:. I especially love that now we return from the server bookmarks instead of bokmarked_posts with the topic.

At the same time, I have some concerns about code design of some parts, they may lead to new refactoring, so please feel to ignore a part or all of them.

It’s hard to reason what’s the differences between these two functions.

  async _modifyBookmarkWithPost(bookmark) {
    const post = await this.model.postById(bookmark.post_id);
    return this._modifyBookmark(bookmark, post);
  },

  async _modifyBookmark(bookmark, post) {
    ....
  }

Looks like _modifyBookmarkWithPost modifies the bookmark + its post, but in fact this function just looks for the post and then pass that post to _modifyBookmark and it’s that function actually modifies both the bookmark and the post. So, looks like the second function should have the name _modifyBookmarkWithPost too?

The names of these function don’t explain clearly what’s the difference between them.

Possibly, we can remove the first function, and get a post in the second function, in the beginning or when we need to (we’re going to need a post only in case it is a post level bookmark as I understand). Then we can delete the post argument from the second function signature (we omit this argument anyway when we call that function directly).

The word “cache” is confusing here. When I see this in Ember code, I think of cache in Ember Data, or probably some custom cache that we may have implemented ourselves. In fact, this function just remove bookmarks from the topic model. Maybe it can be renamed to removeBookmarkFromModel or removeBookmarkFromTopicModel. Even better, looks like this function belongs to the topic model, we call that model in every line, moving it to the model will simplify the function, and it’s name:

  // .../models/topic.js

  _removeBookmark(id) {
    if (!this.bookmarks) {
      this.set("bookmarks", []);
    }
    this.set(
      "bookmarks",
      this.bookmarks.filter((bookmark) => bookmark.id !== id)
    );
    this.set("bookmarked", this.bookmarks.length);
    this.incrementProperty("bookmarksWereChanged");
  },

The first part of this comment doesn’t say anything that cannot be discovered by reading code:

   * Used by toggleBookmark() when called without a post from the
   * topic-footer-buttons bookmark button.

If someone adds a new call to this function or change something around of existing call, he\she should go and update this comment or this comment will get outdated.

Also, the second part:

* Has different behaviour
* based on the number of bookmarks in the topic and also whether
* there is a topic-level bookmark for the OP.

is trying to explain what the function does. And again it doesn’t say anything that can’t be found in code, and now we should care about keeping this comment updated.

In general, comments is a bad practice. Not only because they easily get outdated. The more important problem is that they lead to worse code. I understand that you’re trying to make this code simpler to reason about in the future. But it’s better to do it not by adding comments. It’s always better to put more efforts into:

  • choosing a good explicit descriptive name for the function and its parameters
  • improving implementation of the function, so it is self-explainable

If you feel like you need to add a comment somewhere, probably you feel like that code is going to be hard to understand without reading history or running debugger. It’s better to improve the code itself, then.

By the way, I, personally, think the name and implementation of this function are good, no need to add the comment.

(Speaking of comments in general, it’s good to add comments to API’s (users not going to read implementation at all, they will be using documentation) or complicated algorithms and fancy optimizations (such places are hard to understand by their nature)).

A lot of logic depends on count of bookmarks, probably bookmarksCount can be the property of the topic model.

This comment is just repeating what’s already clearly said in code.

This code is self explainable without the comment as well.

This can be moved closer to the place where we need it.

This place can also benefit from having the bookmarksCount property on the topic model.

Also, this place can benefit from having the bookmarksCount property on the topic model.

    const bookmark = this.model.bookmarks.findBy("id", data.id);

I understand that it was changed during refactoring, but I fell like we still need to have _tooglePostBookmark function. It is one of top-level operations that we do with bookmarks on topics (we also have _toggleTopicLevelBookmark, deleteBookmark etc.). It’s good to have in code the same actions that we have in mind when we reason about user interactions. We clearly have the togglePostBookmark action, but it disappeared in code in this PR.

Yes I am aware of the pitfalls of comments and that they should be used for why vs what. In this case I think you are right and they don’t really serve much purpose, so I will get rid of them.

Good point, I will do this.

Yes good point, will move it to the topic JS model.

@AndrewPrigorshnev thanks for your in-depth review, I will make these changes today. I am hearing you with the confusion around _modifyBookmarkWithPost and _modifyBookmark, I am going to think of a better way to name this functionality and refactor a bit so things are clearer.