FIX: topic level bookmark button (#13530)

We changed (https://github.com/discourse/discourse/pull/13407) behaviour of the topic level bookmark button recently. That PR made the button be opening the edit bookmark modal when there is only one bookmark on the topic instead of just removing that bookmark as it was before.

This PR fixes the next problems that weren't taken into account in the previous PR:

1. Everything should work fine even on very big topics when a bookmarked post is unloaded from the post stream. I've added code that loads the post we need and makes everything work as expected
2. When at least one bookmark on the topic has a reminder, we should always be showing the icon with a clock on the topic level bookmark button
3. We should show correct tooltips for the topic level bookmark button

app/assets/javascripts/discourse/app/controllers/topic.js

@@ -1204,75 +1204,89 @@ export default Controller.extend(bufferedProperty("model"), {
           post.appEvents.trigger("post-stream:refresh", { id: post.id });
         },
         afterSave: (savedData) => {
+          this._addOrUpdateBookmarkedPost(post.id, savedData.reminderAt);
           post.createBookmark(savedData);
           resolve({ closedWithoutSaving: false });
         },
         afterDelete: (topicBookmarked) => {
+          this.model.set(
+            "bookmarked_posts",
+            this.model.bookmarked_posts.filter((x) => x.post_id !== post.id)
+          );
           post.deleteBookmark(topicBookmarked);
         },
       });
     });
   },
 
+  _addOrUpdateBookmarkedPost(postId, reminderAt) {
+    if (!this.model.bookmarked_posts) {
+      this.model.set("bookmarked_posts", []);
+    }
+
+    let bookmarkedPost = this.model.bookmarked_posts.findBy("post_id", postId);
+    if (!bookmarkedPost) {
+      bookmarkedPost = { post_id: postId };
+      this.model.bookmarked_posts.pushObject(bookmarkedPost);
+    }
+
+    bookmarkedPost.reminder_at = reminderAt;
+  },
+
   _toggleTopicBookmark() {
     if (this.model.bookmarking) {
       return Promise.resolve();
     }
     this.model.set("bookmarking", true);
-    const alreadyBookmarkedPosts = this.model.bookmarkedPosts;
+    const bookmarkedPostsCount = this.model.bookmarked_posts
+      ? this.model.bookmarked_posts.length
+      : 0;
 
-    return this.model.firstPost().then((firstPost) => {
+    const bookmarkPost = async (post) => {
-      const bookmarkPost = async (post) => {
+      const opts = await this._togglePostBookmark(post);
-        const opts = await this._togglePostBookmark(post);
+      this.model.set("bookmarking", false);
-        this.model.set("bookmarking", false);
+      if (opts.closedWithoutSaving) {
-        if (opts.closedWithoutSaving) {
+        return;
-          return;
+      }
-        }
+      this.model.afterPostBookmarked(post);
-        this.model.afterPostBookmarked(post);
+      return [post.id];
-        return [post.id];
+    };
-      };
 
-      const toggleBookmarkOnServer = () => {
+    const toggleBookmarkOnServer = async () => {
-        if (alreadyBookmarkedPosts.length === 0) {
+      if (bookmarkedPostsCount === 0) {
-          return bookmarkPost(firstPost);
+        const firstPost = await this.model.firstPost();
-        } else if (alreadyBookmarkedPosts.length === 1) {
+        return bookmarkPost(firstPost);
-          const post = alreadyBookmarkedPosts[0];
+      } else if (bookmarkedPostsCount === 1) {
-          return bookmarkPost(post);
+        const postId = this.model.bookmarked_posts[0].post_id;
-        } else {
+        const post = await this.model.postById(postId);
-          return this.model
+        return bookmarkPost(post);
-            .deleteBookmark()
+      } else {
-            .then(() => {
+        return this.model
-              this.model.toggleProperty("bookmarked");
+          .deleteBookmarks()
-              this.model.set("bookmark_reminder_at", null);
+          .then(() => this.model.clearBookmarks())
-              alreadyBookmarkedPosts.forEach((post) => {
+          .catch(popupAjaxError)
-                post.clearBookmark();
+          .finally(() => this.model.set("bookmarking", false));
-              });
+      }
-              return alreadyBookmarkedPosts.mapBy("id");
+    };
-            })
-            .catch(popupAjaxError)
-            .finally(() => this.model.set("bookmarking", false));
-        }
-      };
 
-      return new Promise((resolve) => {
+    return new Promise((resolve) => {
-        if (alreadyBookmarkedPosts.length > 1) {
+      if (bookmarkedPostsCount > 1) {
-          bootbox.confirm(
+        bootbox.confirm(
-            I18n.t("bookmarks.confirm_clear"),
+          I18n.t("bookmarks.confirm_clear"),
-            I18n.t("no_value"),
+          I18n.t("no_value"),
-            I18n.t("yes_value"),
+          I18n.t("yes_value"),
-            (confirmed) => {
+          (confirmed) => {
-              if (confirmed) {
+            if (confirmed) {
-                toggleBookmarkOnServer().then(resolve);
+              toggleBookmarkOnServer().then(resolve);
-              } else {
+            } else {
-                this.model.set("bookmarking", false);
+              this.model.set("bookmarking", false);
-                resolve();
+              resolve();
-              }
             }
-          );
+          }
-        } else {
+        );
-          toggleBookmarkOnServer().then(resolve);
+      } else {
-        }
+        toggleBookmarkOnServer().then(resolve);
-      });
+      }
     });
   },
 

app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js

@@ -1,5 +1,4 @@
 import I18n from "I18n";
-import { formattedReminderTime } from "discourse/lib/bookmark";
 import { registerTopicFooterButton } from "discourse/lib/register-topic-footer-button";
 import showModal from "discourse/lib/show-modal";
 
@@ -12,8 +11,7 @@ const DEFER_PRIORITY = 500;
 export default {
   name: "topic-footer-buttons",
 
-  initialize(container) {
+  initialize() {
-    const currentUser = container.lookup("current-user:main");
     registerTopicFooterButton({
       id: "share-and-invite",
       icon: "link",
@@ -66,22 +64,27 @@ export default {
     });
 
     registerTopicFooterButton({
-      dependentKeys: ["topic.bookmarked", "topic.bookmarkedPosts"],
+      dependentKeys: ["topic.bookmarked", "topic.bookmarksWereChanged"],
       id: "bookmark",
       icon() {
-        if (this.get("topic.bookmark_reminder_at")) {
+        const bookmarkedPosts = this.topic.bookmarked_posts;
+        if (bookmarkedPosts && bookmarkedPosts.find((x) => x.reminder_at)) {
           return "discourse-bookmark-clock";
         }
         return "bookmark";
       },
       priority: BOOKMARK_PRIORITY,
       classNames() {
-        const bookmarked = this.get("topic.bookmarked");
+        return this.topic.bookmarked
-        return bookmarked ? ["bookmark", "bookmarked"] : ["bookmark"];
+          ? ["bookmark", "bookmarked"]
+          : ["bookmark"];
       },
       label() {
-        if (!this.get("topic.isPrivateMessage") || this.site.mobileView) {
+        if (!this.topic.isPrivateMessage || this.site.mobileView) {
-          const bookmarkedPostsCount = this.get("topic.bookmarkedPosts").length;
+          const bookmarkedPosts = this.topic.bookmarked_posts;
+          const bookmarkedPostsCount = bookmarkedPosts
+            ? bookmarkedPosts.length
+            : 0;
 
           if (bookmarkedPostsCount === 0) {
             return "bookmarked.title";
@@ -93,20 +96,16 @@ export default {
         }
       },
       translatedTitle() {
-        const bookmarked = this.get("topic.bookmarked");
+        const bookmarkedPosts = this.topic.bookmarked_posts;
-        const bookmark_reminder_at = this.get("topic.bookmark_reminder_at");
+        if (!bookmarkedPosts || bookmarkedPosts.length === 0) {
-        if (bookmarked) {
+          return I18n.t("bookmarked.help.bookmark");
-          if (bookmark_reminder_at) {
+        } else if (bookmarkedPosts.length === 1) {
-            return I18n.t("bookmarked.help.unbookmark_with_reminder", {
+          return I18n.t("bookmarked.help.edit_bookmark");
-              reminder_at: formattedReminderTime(
+        } else if (bookmarkedPosts.find((x) => x.reminder_at)) {
-                bookmark_reminder_at,
+          return I18n.t("bookmarked.help.unbookmark_with_reminder");
-                currentUser.resolvedTimezone(currentUser)
+        } else {
-              ),
-            });
-          }
           return I18n.t("bookmarked.help.unbookmark");
         }
-        return I18n.t("bookmarked.help.bookmark");
       },
       action: "toggleBookmark",
       dropdown() {

app/assets/javascripts/discourse/app/models/post.js

@@ -322,6 +322,7 @@ const Post = RestModel.extend({
   deleteBookmark(bookmarked) {
     this.set("topic.bookmarked", bookmarked);
     this.clearBookmark();
+    this.topic.incrementProperty("bookmarksWereChanged");
     this.appEvents.trigger("page:bookmark-post-toggled", this);
   },
 

app/assets/javascripts/discourse/app/models/topic.js

@@ -322,11 +322,6 @@ const Topic = RestModel.extend({
     return Site.currentProp("archetypes").findBy("id", archetype);
   },
 
-  @discourseComputed("bookmarksWereChanged")
-  bookmarkedPosts() {
-    return this.postStream.posts.filterBy("bookmarked", true);
-  },
-
   isPrivateMessage: equal("archetype", "private_message"),
   isBanner: equal("archetype", "banner"),
 
@@ -363,7 +358,6 @@ const Topic = RestModel.extend({
 
   afterPostBookmarked(post) {
     post.set("bookmarked", true);
-    this.set("bookmark_reminder_at", post.bookmark_reminder_at);
   },
 
   firstPost() {
@@ -376,22 +370,40 @@ const Topic = RestModel.extend({
 
     const postId = postStream.findPostIdForPostNumber(1);
     if (postId) {
-      // try loading from identity map first
+      return this.postById(postId);
-      firstPost = postStream.findLoadedPost(postId);
-      if (firstPost) {
-        return Promise.resolve(firstPost);
-      }
-
-      return this.postStream.loadPost(postId);
     } else {
       return this.postStream.loadPostByPostNumber(1);
     }
   },
 
-  deleteBookmark() {
+  postById(id) {
+    const loaded = this.postStream.findLoadedPost(id);
+    if (loaded) {
+      return Promise.resolve(loaded);
+    }
+
+    return this.postStream.loadPost(id);
+  },
+
+  deleteBookmarks() {
     return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" });
   },
 
+  clearBookmarks() {
+    this.toggleProperty("bookmarked");
+
+    const postIds = this.bookmarked_posts.mapBy("post_id");
+    postIds.forEach((postId) => {
+      const loadedPost = this.postStream.findLoadedPost(postId);
+      if (loadedPost) {
+        loadedPost.clearBookmark();
+      }
+    });
+    this.set("bookmarked_posts", []);
+
+    return postIds;
+  },
+
   createGroupInvite(group) {
     return ajax(`/t/${this.id}/invite-group`, {
       type: "POST",

app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js

@@ -22,6 +22,39 @@ async function openEditBookmarkModal() {
   await click(".topic-post:first-child button.bookmarked");
 }
 
+async function testTopicLevelBookmarkButtonIcon(assert, postNumber) {
+  const iconWithoutClock = "d-icon-bookmark";
+  const iconWithClock = "d-icon-discourse-bookmark-clock";
+
+  await visit("/t/internationalization-localization/280");
+  assert.ok(
+    query("#topic-footer-button-bookmark svg").classList.contains(
+      iconWithoutClock
+    ),
+    "Shows an icon without a clock when there is no a bookmark"
+  );
+
+  await openBookmarkModal(postNumber);
+  await click("#save-bookmark");
+
+  assert.ok(
+    query("#topic-footer-button-bookmark svg").classList.contains(
+      iconWithoutClock
+    ),
+    "Shows an icon without a clock when there is a bookmark without a reminder"
+  );
+
+  await openBookmarkModal(postNumber);
+  await click("#tap_tile_tomorrow");
+
+  assert.ok(
+    query("#topic-footer-button-bookmark svg").classList.contains(
+      iconWithClock
+    ),
+    "Shows an icon with a clock when there is a bookmark with a reminder"
+  );
+}
+
 acceptance("Bookmarking", function (needs) {
   needs.user();
   let steps = [];
@@ -64,6 +97,7 @@ acceptance("Bookmarking", function (needs) {
     }
     server.post("/bookmarks", handleRequest);
     server.put("/bookmarks/1", handleRequest);
+    server.put("/bookmarks/2", handleRequest);
     server.delete("/bookmarks/1", () =>
       helper.response({ success: "OK", topic_bookmarked: false })
     );
@@ -354,4 +388,14 @@ acceptance("Bookmarking", function (needs) {
       "The edit modal is opened"
     );
   });
+
+  test("The topic level bookmark button shows an icon with a clock if there is a bookmark with a reminder on the first post", async function (assert) {
+    const postNumber = 1;
+    await testTopicLevelBookmarkButtonIcon(assert, postNumber);
+  });
+
+  test("The topic level bookmark button shows an icon with a clock if there is a bookmark with a reminder on the second post", async function (assert) {
+    const postNumber = 2;
+    await testTopicLevelBookmarkButtonIcon(assert, postNumber);
+  });
 });

app/serializers/post_serializer.rb

@@ -358,8 +358,11 @@ class PostSerializer < BasicPostSerializer
   end
 
   def post_bookmark
-    return nil if @topic_view.blank?
+    if @topic_view.present?
-    @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id }
+      @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id }
+    else
+      @post_bookmark ||= object.bookmarks.find_by(user: scope.user)
+    end
   end
 
   def bookmark_reminder_at

app/serializers/topic_view_serializer.rb

@@ -62,7 +62,7 @@ class TopicViewSerializer < ApplicationSerializer
     :is_warning,
     :chunk_size,
     :bookmarked,
-    :bookmark_reminder_at,
+    :bookmarked_posts,
     :message_archived,
     :topic_timer,
     :unicode_title,
@@ -193,12 +193,8 @@ class TopicViewSerializer < ApplicationSerializer
     object.has_bookmarks?
   end
 
-  def include_bookmark_reminder_at?
+  def bookmarked_posts
-    bookmarked
+    object.bookmarked_posts
-  end
-
-  def bookmark_reminder_at
-    object.first_post_bookmark_reminder_at
   end
 
   def topic_timer

app/serializers/web_hook_topic_view_serializer.rb

@@ -22,6 +22,7 @@ class WebHookTopicViewSerializer < TopicViewSerializer
     image_url
     slow_mode_seconds
     slow_mode_enabled_until
+    bookmarked_posts
   }.each do |attr|
     define_method("include_#{attr}?") do
       false

config/locales/client.en.yml

@@ -298,8 +298,9 @@ en:
       clear_bookmarks: "Clear Bookmarks"
       help:
         bookmark: "Click to bookmark the first post on this topic"
+        edit_bookmark: "Click to edit the bookmark on this topic"
         unbookmark: "Click to remove all bookmarks in this topic"
-        unbookmark_with_reminder: "Click to remove all bookmarks and reminders in this topic. You have a reminder set %{reminder_at} for this topic."
+        unbookmark_with_reminder: "Click to remove all bookmarks and reminders in this topic."
 
     bookmarks:
       created: "You've bookmarked this post. %{name}"

lib/topic_view.rb

@@ -395,13 +395,14 @@ class TopicView
     @topic.bookmarks.exists?(user_id: @user.id)
   end
 
-  def first_post_bookmark_reminder_at
+  def bookmarked_posts
-    @first_post_bookmark_reminder_at ||= \
+    return nil unless has_bookmarks?
-      begin
+    @topic.bookmarks.where(user: @user).pluck(:post_id, :reminder_at).map do |post_id, reminder_at|
-        first_post = @topic.posts.with_deleted.find_by(post_number: 1)
+      {
-        return if !first_post
+        post_id: post_id,
-        first_post.bookmarks.where(user: @user).pluck_first(:reminder_at)
+        reminder_at: reminder_at
-      end
+      }
+    end
   end
 
   MAX_PARTICIPANTS = 24

spec/components/topic_view_spec.rb

@@ -413,9 +413,17 @@ describe TopicView do
     context "#first_post_bookmark_reminder_at" do
       let!(:user) { Fabricate(:user) }
       let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) }
+      let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, post: topic.posts[1], user: user) }
 
       it "gets the first post bookmark reminder at for the user" do
-        expect(TopicView.new(topic.id, user).first_post_bookmark_reminder_at).to eq_time(bookmark1.reminder_at)
+        topic_view = TopicView.new(topic.id, user)
+
+        bookmarked_posts = topic_view.bookmarked_posts
+        first, second = bookmarked_posts
+        expect(first[:post_id]).to eq(bookmark1.post_id)
+        expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
+        expect(second[:post_id]).to eq(bookmark2.post_id)
+        expect(second[:reminder_at]).to eq_time(bookmark1.reminder_at)
       end
 
       context "when the topic is deleted" do
@@ -423,7 +431,13 @@ describe TopicView do
           topic_view = TopicView.new(topic, user)
           PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
           topic.reload
-          expect(topic_view.first_post_bookmark_reminder_at).to eq_time(bookmark1.reminder_at)
+
+          bookmarked_posts = topic_view.bookmarked_posts
+          first, second = bookmarked_posts
+          expect(first[:post_id]).to eq(bookmark1.post_id)
+          expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
+          expect(second[:post_id]).to eq(bookmark2.post_id)
+          expect(second[:reminder_at]).to eq_time(bookmark1.reminder_at)
         end
       end
     end

spec/serializers/post_serializer_spec.rb

@@ -245,14 +245,6 @@ describe PostSerializer do
         it "returns the reminder_at for the bookmark" do
           expect(serialized.as_json[:bookmark_reminder_at]).to eq(bookmark.reminder_at.iso8601)
         end
-
-        context "if topic_view is blank" do
-          let(:topic_view) { nil }
-
-          it "the bookmarked attribute will be false" do
-            expect(serialized.as_json[:bookmarked]).to eq(false)
-          end
-        end
       end
     end
   end