DEV: Change to use nil post ID instead of -1 post ID

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.

app/assets/javascripts/discourse/app/components/bookmark.js

@@ -184,6 +184,7 @@ export default Component.extend({
       reminder_at: reminderAtISO,
       name: this.model.name,
       post_id: this.model.postId,
+      topic_id: this.model.topicId,
       id: this.model.id,
       auto_delete_preference: this.autoDeletePreference,
     };
@@ -212,6 +213,7 @@ export default Component.extend({
       autoDeletePreference: this.autoDeletePreference,
       id: this.model.id || response.id,
       name: this.model.name,
+      postId: this.model.postId,
     });
   },
 

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

@@ -719,11 +719,11 @@ export default Controller.extend(bufferedProperty("model"), {
       }
     },
 
-    toggleBookmark(post) {
+    toggleBookmark(target) {
       if (!this.currentUser) {
         return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
-      } else if (post) {
-        return this._togglePostBookmark(post);
+      } else if (target) {
+        return this._toggleBookmark(target);
       } else {
         return this._toggleTopicBookmark(this.model).then((changedIds) => {
           if (!changedIds) {
@@ -1189,17 +1189,23 @@ export default Controller.extend(bufferedProperty("model"), {
     }
   },
 
-  _togglePostBookmark(post) {
+  // target can be a topic, for topic-level bookmarks, or a post
+  _toggleBookmark(target) {
     return new Promise((resolve) => {
+      const bookmarkingTopic = target instanceof Topic;
+      const postId = bookmarkingTopic ? null : target.id;
+      const topicId = bookmarkingTopic ? target.id : target.topic_id;
+
       let modalController = showModal("bookmark", {
         model: {
-          postId: post.id,
-          id: post.bookmark_id,
-          reminderAt: post.bookmark_reminder_at,
-          autoDeletePreference: post.bookmark_auto_delete_preference,
-          name: post.bookmark_name,
+          postId,
+          topicId,
+          id: target.bookmark_id,
+          reminderAt: target.bookmark_reminder_at,
+          autoDeletePreference: target.bookmark_auto_delete_preference,
+          name: target.bookmark_name,
         },
-        title: post.bookmark_id
+        title: target.bookmark_id
           ? "post.bookmarks.edit"
           : "post.bookmarks.create",
         modalClass: "bookmark-with-reminder",
@@ -1207,36 +1213,46 @@ export default Controller.extend(bufferedProperty("model"), {
       modalController.setProperties({
         onCloseWithoutSaving: () => {
           resolve({ closedWithoutSaving: true });
-          post.appEvents.trigger("post-stream:refresh", { id: post.id });
+
+          if (bookmarkingTopic) {
+            target.appEvents.trigger("post-stream:refresh", {
+              id: postId,
+            });
+          }
         },
         afterSave: (savedData) => {
-          this._addOrUpdateBookmarkedPost(post.id, savedData.reminderAt);
-          post.createBookmark(savedData);
+          this._addOrUpdateBookmarkedPost(
+            savedData.postId,
+            savedData.reminderAt
+          );
+          target.createBookmark(savedData);
           resolve({ closedWithoutSaving: false });
         },
         afterDelete: (topicBookmarked) => {
           this.model.set(
-            "bookmarked_posts",
-            this.model.bookmarked_posts.filter((x) => x.post_id !== post.id)
+            "bookmarked_items",
+            this.model.bookmarked_items.filter(
+              (bookmarkedItem) => bookmarkedItem.post_id !== postId
+            )
           );
-          post.deleteBookmark(topicBookmarked);
+          target.deleteBookmark(topicBookmarked);
         },
       });
     });
   },
 
   _addOrUpdateBookmarkedPost(postId, reminderAt) {
-    if (!this.model.bookmarked_posts) {
-      this.model.set("bookmarked_posts", []);
+    if (!this.model.bookmarked_items) {
+      this.model.set("bookmarked_items", []);
     }
 
-    let bookmarkedPost = this.model.bookmarked_posts.findBy("post_id", postId);
-    if (!bookmarkedPost) {
-      bookmarkedPost = { post_id: postId };
-      this.model.bookmarked_posts.pushObject(bookmarkedPost);
+    let bookmarkedItem = this.model.bookmarked_items.findBy("post_id", postId);
+    if (!bookmarkedItem) {
+      bookmarkedItem = { post_id: postId };
+      this.model.bookmarked_items.pushObject(bookmarkedItem);
     }
 
-    bookmarkedPost.reminder_at = reminderAt;
+    bookmarkedItem.reminder_at = reminderAt;
   },
 
   _toggleTopicBookmark() {
@@ -1244,29 +1260,43 @@ export default Controller.extend(bufferedProperty("model"), {
       return Promise.resolve();
     }
     this.model.set("bookmarking", true);
-    const bookmarkedPostsCount = this.model.bookmarked_posts
-      ? this.model.bookmarked_posts.length
+    const bookmarkedCount = this.model.bookmarked_items
+      ? this.model.bookmarked_items.length
       : 0;
 
-    const bookmarkPost = async (post) => {
-      const opts = await this._togglePostBookmark(post);
+    const bookmarkTarget = async (target) => {
+      const opts = await this._toggleBookmark(target);
       this.model.set("bookmarking", false);
       if (opts.closedWithoutSaving) {
         return;
       }
-      this.model.afterPostBookmarked(post);
-      return [post.id];
+
+      if (target instanceof Post) {
+        this.model.afterPostBookmarked(target);
+      }
+
+      return [target.id];
     };
 
     const toggleBookmarkOnServer = async () => {
-      if (bookmarkedPostsCount === 0) {
-        const firstPost = await this.model.firstPost();
-        return bookmarkPost(firstPost);
-      } else if (bookmarkedPostsCount === 1) {
-        const postId = this.model.bookmarked_posts[0].post_id;
-        const post = await this.model.postById(postId);
-        return bookmarkPost(post);
+      // bookmark the topic if there are no bookmarked posts
+      if (bookmarkedCount === 0) {
+        return bookmarkTarget(this.model);
+
+        // if there is only one bookmarked item for the topic we want to
+        // get it in the stream and edit that one (could be a post or the topic-level bookmark)
+      } else if (bookmarkedCount === 1) {
+        const postId = this.model.bookmarked_items[0].post_id;
+
+        if (!postId) {
+          return bookmarkTarget(this.model);
+        } else {
+          const post = await this.model.postById(postId);
+          return bookmarkTarget(post);
+        }
       } else {
+        // otherwise we want to clear all the bookmarks out if there is more
+        // than one bookmarked item in the topic
         return this.model
           .deleteBookmarks()
           .then(() => this.model.clearBookmarks())
@@ -1276,7 +1306,7 @@ export default Controller.extend(bufferedProperty("model"), {
     };
 
     return new Promise((resolve) => {
-      if (bookmarkedPostsCount > 1) {
+      if (bookmarkedCount > 1) {
         bootbox.confirm(
           I18n.t("bookmarks.confirm_clear"),
           I18n.t("no_value"),

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

@@ -67,8 +67,10 @@ export default {
       dependentKeys: ["topic.bookmarked", "topic.bookmarksWereChanged"],
       id: "bookmark",
       icon() {
-        const bookmarkedPosts = this.topic.bookmarked_posts;
-        if (bookmarkedPosts && bookmarkedPosts.find((x) => x.reminder_at)) {
+        const bookmarkedItems = this.topic.bookmarked_items || [];
+        if (
+          bookmarkedItems.find((bookmarkedItem) => bookmarkedItem.reminder_at)
+        ) {
           return "discourse-bookmark-clock";
         }
         return "bookmark";
@@ -81,14 +83,14 @@ export default {
       },
       label() {
         if (!this.topic.isPrivateMessage || this.site.mobileView) {
-          const bookmarkedPosts = this.topic.bookmarked_posts;
-          const bookmarkedPostsCount = bookmarkedPosts
-            ? bookmarkedPosts.length
+          const bookmarkedItems = this.topic.bookmarked_items;
+          const bookmarkedItemsCount = bookmarkedItems
+            ? bookmarkedItems.length
             : 0;
 
-          if (bookmarkedPostsCount === 0) {
+          if (bookmarkedItemsCount === 0) {
             return "bookmarked.title";
-          } else if (bookmarkedPostsCount === 1) {
+          } else if (bookmarkedItemsCount === 1) {
             return "bookmarked.edit_bookmark";
           } else {
             return "bookmarked.clear_bookmarks";
@@ -96,12 +98,14 @@ export default {
         }
       },
       translatedTitle() {
-        const bookmarkedPosts = this.topic.bookmarked_posts;
-        if (!bookmarkedPosts || bookmarkedPosts.length === 0) {
+        const bookmarkedItems = this.topic.bookmarked_items;
+        if (!bookmarkedItems || bookmarkedItems.length === 0) {
           return I18n.t("bookmarked.help.bookmark");
-        } else if (bookmarkedPosts.length === 1) {
+        } else if (bookmarkedItems.length === 1) {
           return I18n.t("bookmarked.help.edit_bookmark");
-        } else if (bookmarkedPosts.find((x) => x.reminder_at)) {
+        } else if (
+          bookmarkedItems.find((bookmarkedItem) => bookmarkedItem.reminder_at)
+        ) {
           return I18n.t("bookmarked.help.unbookmark_with_reminder");
         } else {
           return I18n.t("bookmarked.help.unbookmark");

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

@@ -376,17 +376,51 @@ const Topic = RestModel.extend({
     return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" });
   },
 
+  createBookmark(data) {
+    this.setProperties({
+      bookmarked: true,
+      bookmark_reminder_at: data.reminderAt,
+      bookmark_reminder_type: data.reminderType,
+      bookmark_auto_delete_preference: data.autoDeletePreference,
+      bookmark_name: data.name,
+      bookmark_id: data.id,
+    });
+    this.incrementProperty("bookmarksWereChanged");
+    this.appEvents.trigger("page:bookmark-post-toggled", this.firstPost());
+    this.appEvents.trigger("post-stream:refresh", { id: this.firstPost().id });
+  },
+
+  deleteBookmark(bookmarked) {
+    this.set("bookmarked", bookmarked);
+    this.clearBookmark();
+    this.incrementProperty("bookmarksWereChanged");
+  },
+
+  clearBookmark() {
+    this.setProperties({
+      bookmark_reminder_at: null,
+      bookmark_reminder_type: null,
+      bookmark_name: null,
+      bookmark_id: null,
+      bookmarked: false,
+      bookmark_auto_delete_preference: null,
+    });
+    this.incrementProperty("bookmarksWereChanged");
+  },
+
   clearBookmarks() {
     this.toggleProperty("bookmarked");
 
-    const postIds = this.bookmarked_posts.mapBy("post_id");
+    const postIds = this.bookmarked_items.mapBy("post_id");
     postIds.forEach((postId) => {
       const loadedPost = this.postStream.findLoadedPost(postId);
       if (loadedPost) {
         loadedPost.clearBookmark();
       }
     });
-    this.set("bookmarked_posts", []);
+
+    this.clearBookmark();
+    this.set("bookmarked_items", []);
 
     return postIds;
   },

app/assets/javascripts/discourse/tests/acceptance/topic-timeline-test.js

@@ -266,7 +266,7 @@ acceptance("Topic Timeline", function (needs) {
         ],
         chunk_size: 20,
         bookmarked: false,
-        bookmarked_posts: null,
+        bookmarked_items: null,
         topic_timer: null,
         message_bus_last_id: 5,
         participant_count: 1,

app/controllers/bookmarks_controller.rb

@@ -4,7 +4,14 @@ class BookmarksController < ApplicationController
   requires_login
 
   def create
-    params.require(:post_id)
+    if params[:post_id].blank?
+      if params[:topic_id].blank?
+        raise Discourse::InvalidParameters.new("Either a post_id or a topic_id is required.")
+      end
+      topic_id = params[:topic_id].to_i
+    else
+      post_id = params[:post_id]&.to_i
+    end
 
     RateLimiter.new(
       current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i
@@ -12,7 +19,8 @@ class BookmarksController < ApplicationController
 
     bookmark_manager = BookmarkManager.new(current_user)
     bookmark = bookmark_manager.create(
-      post_id: params[:post_id],
+      topic_id: topic_id,
+      post_id: post_id,
       name: params[:name],
       reminder_type: params[:reminder_type],
       reminder_at: params[:reminder_at],

app/models/bookmark.rb

@@ -39,6 +39,7 @@ class Bookmark < ActiveRecord::Base
   # we don't care whether the post or topic is deleted,
   # they hold important information about the bookmark
   def post
+    return if for_topic?
     Post.unscoped { super }
   end
 
@@ -47,7 +48,7 @@ class Bookmark < ActiveRecord::Base
   end
 
   def unique_per_post_for_user
-    existing_bookmark = Bookmark.find_by(user_id: user_id, post_id: post_id)
+    existing_bookmark = Bookmark.find_by(user_id: user_id, post_id: post_id, topic_id: topic_id)
     return if existing_bookmark.blank? || existing_bookmark.id == id
     self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
   end
@@ -76,6 +77,14 @@ class Bookmark < ActiveRecord::Base
     )
   end
 
+  def for_topic?
+    self.post_id.blank?
+  end
+
+  def for_post?
+    !self.for_topic?
+  end
+
   def no_reminder?
     self.reminder_at.blank? && self.reminder_type.blank?
   end
@@ -156,11 +165,11 @@ end
 #
 # Indexes
 #
-#  index_bookmarks_on_post_id              (post_id)
-#  index_bookmarks_on_reminder_at          (reminder_at)
-#  index_bookmarks_on_reminder_set_at      (reminder_set_at)
-#  index_bookmarks_on_reminder_type        (reminder_type)
-#  index_bookmarks_on_topic_id             (topic_id)
-#  index_bookmarks_on_user_id              (user_id)
-#  index_bookmarks_on_user_id_and_post_id  (user_id,post_id) UNIQUE
+#  index_bookmarks_on_post_id                           (post_id)
+#  index_bookmarks_on_reminder_at                       (reminder_at)
+#  index_bookmarks_on_reminder_set_at                   (reminder_set_at)
+#  index_bookmarks_on_reminder_type                     (reminder_type)
+#  index_bookmarks_on_topic_id                          (topic_id)
+#  index_bookmarks_on_user_id                           (user_id)
+#  index_bookmarks_on_user_id_and_post_id_and_topic_id  (user_id,post_id,topic_id) UNIQUE
 #

app/serializers/topic_view_serializer.rb

@@ -62,7 +62,12 @@ class TopicViewSerializer < ApplicationSerializer
     :is_warning,
     :chunk_size,
     :bookmarked,
-    :bookmarked_posts,
+    :bookmark_reminder_at,
+    :bookmark_id,
+    :bookmark_reminder_type,
+    :bookmark_name,
+    :bookmark_auto_delete_preference,
+    :bookmarked_items,
     :message_archived,
     :topic_timer,
     :unicode_title,
@@ -190,11 +195,53 @@ class TopicViewSerializer < ApplicationSerializer
   end
 
   def bookmarked
-    object.has_bookmarks?
+    @bookmarked ||= object.has_bookmarks?
   end
 
-  def bookmarked_posts
-    object.bookmarked_posts
+  def bookmark_id
+    return if !bookmarked
+    object.bookmark&.id
+  end
+
+  def include_bookmark_reminder_at?
+    bookmarked
+  end
+
+  def include_bookmark_reminder_type?
+    bookmarked
+  end
+
+  def include_bookmark_name?
+    bookmarked
+  end
+
+  def include_bookmark_auto_delete_preference?
+    bookmarked
+  end
+
+  def include_bookmark_id?
+    bookmarked
+  end
+
+  def bookmark_reminder_at
+    object.bookmark&.reminder_at
+  end
+
+  def bookmark_reminder_type
+    return if object.bookmark.blank?
+    Bookmark.reminder_types[object.bookmark.reminder_type].to_s
+  end
+
+  def bookmark_name
+    object.bookmark&.name
+  end
+
+  def bookmark_auto_delete_preference
+    object.bookmark&.auto_delete_preference
+  end
+
+  def bookmarked_items
+    object.bookmarked_items
   end
 
   def topic_timer

app/serializers/user_bookmark_serializer.rb

@@ -35,7 +35,15 @@ class UserBookmarkSerializer < ApplicationSerializer
   end
 
   def post
-    @post ||= object.post || Post.unscoped.find(object.post_id)
+    if for_topic?
+      @post ||= topic.first_post
+    else
+      @post ||= object.post || Post.unscoped.find(object.post_id)
+    end
+  end
+
+  def for_topic?
+    object.for_topic?
   end
 
   def closed

app/serializers/web_hook_topic_view_serializer.rb

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

config/locales/client.en.yml

@@ -298,7 +298,7 @@ en:
       edit_bookmark: "Edit Bookmark"
       clear_bookmarks: "Clear Bookmarks"
       help:
-        bookmark: "Click to bookmark the first post on this topic"
+        bookmark: "Click to bookmark 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."

db/migrate/20210907022247_make_post_id_optional_bookmarks.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class MakePostIdOptionalBookmarks < ActiveRecord::Migration[6.1]
+  def up
+    remove_index :bookmarks, [:user_id, :post_id], unique: true
+    change_column_null :bookmarks, :post_id, true
+    add_index :bookmarks, [:user_id, :post_id, :topic_id], unique: true
+  end
+
+  def down
+    Bookmark.where(post_id: -1).delete_all
+    Bookmark.where(post_id: nil).delete_all
+    remove_index :bookmarks, [:user_id, :post_id, :topic_id], unique: true
+    change_column_null :bookmarks, :post_id, false
+    add_index :bookmarks, [:user_id, :post_id], unique: true
+  end
+end

lib/bookmark_manager.rb

@@ -7,22 +7,27 @@ class BookmarkManager
     @user = user
   end
 
-  def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil, options: {})
-    post = Post.find_by(id: post_id)
+  def create(post_id:, topic_id: nil, name: nil, reminder_type: nil, reminder_at: nil, options: {})
+    if post_id.blank? && topic_id.present?
+      bookmark_for_topic = true
+      topic = Topic.find_by(id: topic_id)
+    else
+      post = Post.find_by(id: post_id)
+      topic = post&.topic
+    end
     reminder_type = parse_reminder_type(reminder_type)
 
-    # no bookmarking deleted posts or topics
-    raise Discourse::InvalidAccess if post.blank? || post.topic.blank?
+    raise Discourse::InvalidAccess if (!bookmark_for_topic && post.blank?) || topic.blank?
 
-    if !Guardian.new(@user).can_see_post?(post) || !Guardian.new(@user).can_see_topic?(post.topic)
+    if (!bookmark_for_topic && !Guardian.new(@user).can_see_post?(post)) || !Guardian.new(@user).can_see_topic?(topic)
       raise Discourse::InvalidAccess
     end
 
     bookmark = Bookmark.create(
       {
         user_id: @user.id,
-        topic: post.topic,
-        post: post,
+        topic: topic,
+        post_id: post_id,
         name: name,
         reminder_type: reminder_type,
         reminder_at: reminder_at,
@@ -34,7 +39,7 @@ class BookmarkManager
       return add_errors_from(bookmark)
     end
 
-    update_topic_user_bookmarked(post.topic)
+    update_topic_user_bookmarked(topic)
 
     bookmark
   end

lib/bookmark_query.rb

@@ -35,13 +35,22 @@ class BookmarkQuery
     pms = Topic.private_messages_for_user(@user)
     results = results.merge(topics.or(pms))
 
-    results = results.merge(Post.secured(@guardian))
+    results = results.where(
+      "posts.post_type IN (?) OR bookmarks.post_id IS NULL", Topic.visible_post_types(@guardian&.user)
+    )
 
     if @params[:q].present?
       term = @params[:q]
       bookmark_ts_query = Search.ts_query(term: term)
       results = results
-        .joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id")
+        .joins(<<~SQL)
+          LEFT JOIN post_search_data ON post_search_data.post_id = CASE
+            WHEN bookmarks.post_id IS NULL THEN (
+              SELECT posts.id FROM posts WHERE post_number = 1 AND topic_id = bookmarks.topic_id
+            )
+            WHEN bookmarks.post_id IS NOT NULL THEN bookmarks.post_id
+            END
+        SQL
         .where(
           "bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data",
           q: "%#{term}%"

lib/bookmark_reminder_notification_handler.rb

@@ -4,7 +4,7 @@ class BookmarkReminderNotificationHandler
   def self.send_notification(bookmark)
     return if bookmark.blank?
     Bookmark.transaction do
-      if bookmark.post.blank? || bookmark.post.deleted_at.present?
+      if bookmark.post&.deleted_at.present?
         clear_reminder(bookmark)
       elsif bookmark.topic
         create_notification(bookmark)
@@ -31,7 +31,7 @@ class BookmarkReminderNotificationHandler
     user.notifications.create!(
       notification_type: Notification.types[:bookmark_reminder],
       topic_id: bookmark.topic_id,
-      post_number: bookmark.post.post_number,
+      post_number: bookmark.for_topic? ? 1 : bookmark.post.post_number,
       data: {
         topic_title: bookmark.topic.title,
         display_username: user.username,

lib/topic_view.rb

@@ -395,16 +395,22 @@ class TopicView
     @topic.bookmarks.exists?(user_id: @user.id)
   end
 
-  def bookmarked_posts
+  def bookmarked_items
     return nil unless has_bookmarks?
     @topic.bookmarks.where(user: @user).pluck(:post_id, :reminder_at).map do |post_id, reminder_at|
       {
         post_id: post_id,
-        reminder_at: reminder_at
+        reminder_at: reminder_at,
+        topic_level: post_id.blank?
       }
     end
   end
 
+  def bookmark
+    return if !has_bookmarks?
+    @bookmark ||= @topic.bookmarks.where(user_id: @user.id, post_id: nil).first
+  end
+
   MAX_PARTICIPANTS = 24
 
   def post_counts_by_user

spec/components/topic_view_spec.rb

@@ -408,19 +408,31 @@ describe TopicView do
       end
     end
 
-    context "#bookmarked_posts" do
+    context "#bookmarked_items" 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) }
+      let!(:bookmark3) { Fabricate(:bookmark_next_business_day_reminder, post: nil, user: user, topic: topic) }
 
       it "gets the first post bookmark reminder at for the user" do
         topic_view = TopicView.new(topic.id, user)
 
-        first, second = topic_view.bookmarked_posts
+        first, second = topic_view.bookmarked_items
         expect(first[:post_id]).to eq(bookmark1.post_id)
         expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
+        expect(first[:topic_level]).to eq(false)
         expect(second[:post_id]).to eq(bookmark2.post_id)
         expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at)
+        expect(second[:topic_level]).to eq(false)
+      end
+
+      it "gets the topic level bookmark reminder at for the user" do
+        topic_view = TopicView.new(topic.id, user)
+
+        topic_level_item = topic_view.bookmarked_items.last
+        expect(topic_level_item[:post_id]).to eq(nil)
+        expect(topic_level_item[:reminder_at]).to eq(bookmark3.reminder_at)
+        expect(topic_level_item[:topic_level]).to eq(true)
       end
 
       context "when the topic is deleted" do
@@ -429,7 +441,7 @@ describe TopicView do
           PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
           topic.reload
 
-          first, second = topic_view.bookmarked_posts
+          first, second = topic_view.bookmarked_items
           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)

spec/fabricators/bookmark_fabricator.rb

@@ -3,7 +3,7 @@
 Fabricator(:bookmark) do
   user
   post { Fabricate(:post) }
-  topic { |attrs| attrs[:post].topic }
+  topic { |attrs| attrs[:post].present? ? attrs[:post].topic : Fabricate(:topic) }
   name "This looked interesting"
   reminder_type { Bookmark.reminder_types[:tomorrow] }
   reminder_at { 1.day.from_now.iso8601 }

spec/lib/bookmark_manager_spec.rb

@@ -41,6 +41,32 @@ RSpec.describe BookmarkManager do
       expect(tu.bookmarked).to eq(true)
     end
 
+    context "when creating a topic-level bookmark" do
+      it "allows passing nil as a post ID" do
+        subject.create(post_id: nil, topic_id: post.topic.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+        expect(
+          Bookmark.exists?(user: user, topic: post.topic, post_id: nil)
+        ).to eq(true)
+      end
+
+      it "when topic is deleted it raises invalid access from guardian check" do
+        post.topic.trash!
+        expect { subject.create(post_id: nil, topic_id: post.topic.id, name: name) }.to raise_error(
+          Discourse::InvalidAccess
+        )
+      end
+
+      it "updates the topic user bookmarked column to true" do
+        subject.create(post_id: nil, topic_id: post.topic.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+        tu = TopicUser.find_by(user: user)
+        expect(tu.bookmarked).to eq(true)
+        tu.update(bookmarked: false)
+        subject.create(post_id: Fabricate(:post, topic: post.topic).id)
+        tu.reload
+        expect(tu.bookmarked).to eq(true)
+      end
+    end
+
     context "when a reminder time + type is provided" do
       it "saves the values correctly" do
         subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)

spec/lib/bookmark_query_spec.rb

@@ -62,6 +62,28 @@ RSpec.describe BookmarkQuery do
         bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
         expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
       end
+
+      context "for bookmarks that are topic level" do
+        before do
+          @bookmark3.update(post_id: nil)
+          @bookmark4.update(post_id: nil)
+        end
+
+        it "can search by bookmark name" do
+          bookmarks = bookmark_query(params: { q: 'check' }).list_all
+          expect(bookmarks.map(&:id)).to eq([@bookmark3.id])
+        end
+
+        it "can search by post content" do
+          bookmarks = bookmark_query(params: { q: 'content' }).list_all
+          expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
+        end
+
+        it "can search by topic title" do
+          bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
+          expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
+        end
+      end
     end
 
     context "for a whispered post" do
@@ -173,6 +195,7 @@ RSpec.describe BookmarkQuery do
     let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago, reminder_type: nil, reminder_at: nil) }
     let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_type: nil, reminder_at: nil) }
     let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_type: nil, reminder_at: nil) }
+    let!(:bookmark6) { Fabricate(:bookmark, user: user, updated_at: 7.days.ago, post: nil, reminder_type: nil, reminder_at: nil) }
 
     it "order defaults to updated_at DESC" do
       expect(bookmark_query.list_all.map(&:id)).to eq([
@@ -180,7 +203,8 @@ RSpec.describe BookmarkQuery do
         bookmark2.id,
         bookmark5.id,
         bookmark4.id,
-        bookmark3.id
+        bookmark3.id,
+        bookmark6.id
       ])
     end
 
@@ -195,7 +219,8 @@ RSpec.describe BookmarkQuery do
         bookmark5.id,
         bookmark1.id,
         bookmark2.id,
-        bookmark3.id
+        bookmark3.id,
+        bookmark6.id
       ])
 
     end
@@ -220,7 +245,8 @@ RSpec.describe BookmarkQuery do
         bookmark4.id,
         bookmark1.id,
         bookmark2.id,
-        bookmark5.id
+        bookmark5.id,
+        bookmark6.id
       ])
     end
   end

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -28,6 +28,19 @@ RSpec.describe BookmarkReminderNotificationHandler do
       expect(data["bookmark_name"]).to eq(bookmark.name)
     end
 
+    it "creates a bookmark reminder notification for a topic-level bookmark" do
+      bookmark.update(post_id: nil)
+      subject.send_notification(bookmark)
+      notif = bookmark.user.notifications.last
+      expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
+      expect(notif.topic_id).to eq(bookmark.topic_id)
+      expect(notif.post_number).to eq(1)
+      data = JSON.parse(notif.data)
+      expect(data["topic_title"]).to eq(bookmark.topic.title)
+      expect(data["display_username"]).to eq(bookmark.user.username)
+      expect(data["bookmark_name"]).to eq(bookmark.name)
+    end
+
     it "clears the reminder" do
       subject.send_notification(bookmark)
       bookmark.reload

spec/requests/bookmarks_controller_spec.rb

@@ -12,6 +12,33 @@ describe BookmarksController do
   end
 
   describe "#create" do
+    it "creates a bookmark for a post" do
+      post "/bookmarks.json", params: {
+        post_id: bookmark_post.id,
+        reminder_type: "tomorrow",
+        reminder_at: (Time.zone.now + 1.day).iso8601
+      }
+
+      expect(response.status).to eq(200)
+      expect(Bookmark.exists?(user: bookmark_user, post: bookmark_post)).to eq(true)
+    end
+
+    it "creates a bookmark for the topic" do
+      post "/bookmarks.json", params: {
+        post_id: nil,
+        topic_id: bookmark_post.topic.id,
+        reminder_type: "tomorrow",
+        reminder_at: (Time.zone.now + 1.day).iso8601
+      }
+
+      expect(response.status).to eq(200)
+      expect(
+        Bookmark.exists?(
+          user: bookmark_user, post_id: nil, topic_id: bookmark_post.topic.id
+        )
+      ).to eq(true)
+    end
+
     it "rate limits creates" do
       SiteSetting.max_bookmarks_per_day = 1
       RateLimiter.enable