FEATURE: Improving bookmarks part 2 -- Topic Bookmarking (#8954)

### UI Changes

If `SiteSetting.enable_bookmarks_with_reminders` is enabled:

* Clicking "Bookmark" on a topic will create a new Bookmark record instead of a post + user action
* Clicking "Clear Bookmarks" on a topic will delete all the new Bookmark records on a topic
* The topic bookmark buttons control the post bookmark flags correctly and vice-versa
Disabled selecting the "reminder type" for bookmarks in the UI because the backend functionality is not done yet (of sending users notifications etc.)

### Other Changes

* Added delete bookmark route (but no UI yet)
* Added a rake task to sync the old PostAction bookmarks to the new Bookmark table, which can be run as many times as we want for a site (it will not create duplicates).

app/assets/javascripts/discourse/controllers/bookmark.js.es6

@@ -48,6 +48,7 @@ export default Controller.extend(ModalFunctionality, {
   },
 
   usingMobileDevice: reads("site.mobileView"),
+  showBookmarkReminderControls: false,
 
   @discourseComputed()
   reminderTypes: () => {

app/assets/javascripts/discourse/models/bookmark.js.es6

@@ -0,0 +1,24 @@
+import { none } from "@ember/object/computed";
+import { computed } from "@ember/object";
+import { ajax } from "discourse/lib/ajax";
+import { Promise } from "rsvp";
+import RestModel from "discourse/models/rest";
+
+const Bookmark = RestModel.extend({
+  newBookmark: none("id"),
+
+  @computed
+  get url() {
+    return Discourse.getURL(`/bookmarks/${this.id}`);
+  },
+
+  destroy() {
+    if (this.newBookmark) return Promise.resolve();
+
+    return ajax(this.url, {
+      type: "DELETE"
+    });
+  }
+});
+
+export default Bookmark;

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

@@ -353,14 +353,20 @@ const Post = RestModel.extend({
           this.appEvents.trigger("post-stream:refresh", { id: this.id });
         },
         afterSave: reminderAtISO => {
-          this.set("bookmark_reminder_at", reminderAtISO);
+          this.setProperties({
+            "topic.bookmarked": true,
+            bookmark_reminder_at: reminderAtISO
+          });
           this.appEvents.trigger("post-stream:refresh", { id: this.id });
         }
       });
     } else {
       this.set("bookmark_reminder_at", null);
       return Post.destroyBookmark(this.id)
-        .then(() => this.appEvents.trigger("page:bookmark-post-toggled", this))
+        .then(result => {
+          this.set("topic.bookmarked", result.topic_bookmarked);
+          this.appEvents.trigger("page:bookmark-post-toggled", this);
+        })
         .catch(error => {
           this.toggleProperty("bookmarked_with_reminder");
           throw new Error(error);

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

@@ -402,6 +402,9 @@ const Topic = RestModel.extend({
           this.toggleProperty("bookmarked");
           if (bookmark && firstPost) {
             firstPost.set("bookmarked", true);
+            if (this.siteSettings.enable_bookmarks_with_reminders) {
+              firstPost.set("bookmarked_with_reminder", true);
+            }
             return [firstPost.id];
           }
           if (!bookmark && posts) {
@@ -409,7 +412,14 @@ const Topic = RestModel.extend({
             posts.forEach(post => {
               if (post.get("bookmarked")) {
                 post.set("bookmarked", false);
-                updated.push(post.get("id"));
+                updated.push(post.id);
+              }
+              if (
+                this.siteSettings.enable_bookmarks_with_reminders &&
+                post.get("bookmarked_with_reminder")
+              ) {
+                post.set("bookmarked_with_reminder", false);
+                updated.push(post.id);
               }
             });
             return updated;
@@ -424,7 +434,9 @@ const Topic = RestModel.extend({
     const unbookmarkedPosts = [];
     if (!bookmark && posts) {
       posts.forEach(
-        post => post.get("bookmarked") && unbookmarkedPosts.push(post)
+        post =>
+          (post.get("bookmarked") || post.get("bookmarked_with_reminder")) &&
+          unbookmarkedPosts.push(post)
       );
     }
 

app/assets/javascripts/discourse/routes/app-route-map.js.es6

@@ -48,7 +48,7 @@ export default function() {
       });
     });
 
-    // filters
+    // filters (e.g. bookmarks, posted, read, unread, latest)
     Site.currentProp("filters").forEach(filter => {
       // legacy route
       this.route(filter + "ParentCategory", { path: "/c/:slug/l/" + filter });

app/assets/javascripts/discourse/templates/modal/bookmark.hbs

@@ -1,4 +1,4 @@
-{{#d-modal-body}}
+{{#d-modal-body id="bookmark-reminder-modal"}}
   {{#conditional-loading-spinner condition=loading}}
     {{#if errorMessage}}
     <div class="control-group">
@@ -13,9 +13,10 @@
         {{i18n 'post.bookmarks.name'}}
       </label>
 
-      {{input value=name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
+      {{input value=name name="name" class="bookmark-name" enter=(action "saveAndClose") placeholder=(i18n "post.bookmarks.name_placeholder")}}
     </div>
 
+    {{#if showBookmarkReminderControls}}
     <div class="control-group">
       <label class="control-label" for="set_reminder">
         {{i18n 'post.bookmarks.set_reminder'}}
@@ -40,6 +41,7 @@
         <div class="alert alert-info">{{{i18n "bookmarks.no_timezone" basePath=basePath }}}</div>
       {{/if}}
     </div>
+    {{/if}}
 
     <div class="control-group">
       {{d-button label="bookmarks.save" class="btn-primary" action=(action "saveAndClose")}}

app/controllers/bookmarks_controller.rb

@@ -13,7 +13,7 @@ class BookmarksController < ApplicationController
 
     bookmark = Bookmark.create(
       user_id: current_user.id,
-      topic_id: params[:topic_id],
+      topic_id: Post.select(:topic_id).find(params[:post_id]).topic_id,
       post_id: params[:post_id],
       name: params[:name],
       reminder_type: Bookmark.reminder_types[params[:reminder_type].to_sym],
@@ -23,4 +23,16 @@ class BookmarksController < ApplicationController
     return render json: success_json if bookmark.save
     render json: failed_json.merge(errors: bookmark.errors.full_messages), status: 400
   end
+
+  def destroy
+    params.require(:id)
+
+    bookmark = Bookmark.find_by(id: params[:id])
+    raise Discourse::NotFound if bookmark.blank?
+
+    raise Discourse::InvalidAccess.new if !guardian.can_delete?(bookmark)
+
+    bookmark.destroy
+    render json: success_json
+  end
 end

app/controllers/posts_controller.rb

@@ -514,7 +514,8 @@ class PostsController < ApplicationController
     existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id)
     existing_bookmark.destroy if existing_bookmark.present?
 
-    render json: success_json
+    topic_bookmarked = Bookmark.exists?(topic_id: existing_bookmark.topic_id, user_id: current_user.id)
+    render json: success_json.merge(topic_bookmarked: topic_bookmarked)
   end
 
   def wiki

app/controllers/topics_controller.rb

@@ -486,11 +486,15 @@ class TopicsController < ApplicationController
   def remove_bookmarks
     topic = Topic.find(params[:topic_id].to_i)
 
-    PostAction.joins(:post)
-      .where(user_id: current_user.id)
-      .where('topic_id = ?', topic.id).each do |pa|
+    if SiteSetting.enable_bookmarks_with_reminders?
+      Bookmark.where(user_id: current_user.id, topic_id: topic.id).destroy_all
+    else
+      PostAction.joins(:post)
+        .where(user_id: current_user.id)
+        .where('topic_id = ?', topic.id).each do |pa|
 
-      PostActionDestroyer.destroy(current_user, pa.post, :bookmark)
+        PostActionDestroyer.destroy(current_user, pa.post, :bookmark)
+      end
     end
 
     render body: nil
@@ -543,8 +547,15 @@ class TopicsController < ApplicationController
     topic = Topic.find(params[:topic_id].to_i)
     first_post = topic.ordered_posts.first
 
-    result = PostActionCreator.create(current_user, first_post, :bookmark)
-    return render_json_error(result) if result.failed?
+    if SiteSetting.enable_bookmarks_with_reminders?
+      if Bookmark.exists?(user: current_user, post: first_post)
+        return render_json_error(I18n.t("bookmark.topic_already_bookmarked"), status: 403)
+      end
+      Bookmark.create(user: current_user, post: first_post, topic: topic)
+    else
+      result = PostActionCreator.create(current_user, first_post, :bookmark)
+      return render_json_error(result) if result.failed?
+    end
 
     render body: nil
   end

app/models/bookmark.rb

@@ -29,7 +29,7 @@ end
 #
 #  id            :bigint           not null, primary key
 #  user_id       :bigint           not null
-#  topic_id      :bigint
+#  topic_id      :bigint           not null
 #  post_id       :bigint           not null
 #  name          :string
 #  reminder_type :integer

app/models/post.rb

@@ -37,6 +37,7 @@ class Post < ActiveRecord::Base
   has_many :uploads, through: :post_uploads
 
   has_one :post_stat
+  has_many :bookmarks
 
   has_one :incoming_email
 

app/models/topic.rb

@@ -96,6 +96,7 @@ class Topic < ActiveRecord::Base
   belongs_to :category
   has_many :category_users, through: :category
   has_many :posts
+  has_many :bookmarks
   has_many :ordered_posts, -> { order(post_number: :asc) }, class_name: "Post"
   has_many :topic_allowed_users
   has_many :topic_allowed_groups

config/locales/client.en.yml

@@ -2649,6 +2649,10 @@ en:
         name: "Name"
         name_placeholder: "Name the bookmark to help jog your memory"
         set_reminder: "Set a reminder"
+        actions:
+          delete_bookmark:
+            name: "Delete bookmark"
+            description: "Removes the bookmark from your profile and stops all reminders for the bookmark"
 
     category:
       can: "can… "

config/routes.rb

@@ -599,7 +599,7 @@ Discourse::Application.routes.draw do
     end
   end
 
-  resources :bookmarks, only: %i[create]
+  resources :bookmarks, only: %i[create destroy]
 
   resources :notifications, except: :show do
     collection do

db/migrate/20191217035630_populate_topic_id_on_bookmarks.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class PopulateTopicIdOnBookmarks < ActiveRecord::Migration[6.0]
+  def up
+    Bookmark.where(topic_id: nil).includes(:post).find_each do |bookmark|
+      bookmark.update_column(:topic_id, bookmark.post.topic_id)
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/migrate/20200203061927_mark_bookmarks_topic_id_not_null.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class MarkBookmarksTopicIdNotNull < ActiveRecord::Migration[6.0]
+  def change
+    change_column_null :bookmarks, :topic_id, false
+  end
+end

lib/guardian.rb

@@ -3,6 +3,7 @@
 require 'guardian/category_guardian'
 require 'guardian/ensure_magic'
 require 'guardian/post_guardian'
+require 'guardian/bookmark_guardian'
 require 'guardian/topic_guardian'
 require 'guardian/user_guardian'
 require 'guardian/post_revision_guardian'
@@ -14,6 +15,7 @@ class Guardian
   include EnsureMagic
   include CategoryGuardian
   include PostGuardian
+  include BookmarkGuardian
   include TopicGuardian
   include UserGuardian
   include PostRevisionGuardian

lib/guardian/bookmark_guardian.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+module BookmarkGuardian
+  def can_delete_bookmark?(bookmark)
+    @user == bookmark.user
+  end
+end

lib/tasks/bookmarks.rake

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require_dependency "rake_helpers"
+
+##
+# This will create records in the new bookmarks table from PostAction
+# records. The task is idempotent, it will not create additional bookmark
+# records for PostActions that have already been created in the new table.
+# You can provide a sync_limit for a smaller batch run.
+#
+desc "migrates old PostAction bookmarks to the new Bookmark model & table"
+task "bookmarks:sync_to_table", [:sync_limit] => :environment do |_t, args|
+  sync_limit = args[:sync_limit] || 0
+  post_action_bookmarks = PostAction
+    .select('post_actions.id', 'post_actions.post_id', 'posts.topic_id', 'post_actions.user_id')
+    .where(post_action_type_id: PostActionType.types[:bookmark])
+    .joins(:post)
+    .where(deleted_at: nil)
+    .joins('LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id')
+    .where('bookmarks.id IS NULL')
+
+  # if sync_limit is provided as an argument this will cap
+  # the number of bookmarks that will be created in a run of
+  # this task (for huge bookmark count communities)
+  if sync_limit > 0
+    post_action_bookmarks = post_action_bookmarks.limit(sync_limit)
+  end
+
+  post_action_bookmark_count = post_action_bookmarks.count('post_actions.id')
+  bookmarks_to_create = []
+  i = 0
+
+  Bookmark.transaction do
+    post_action_bookmarks.find_each(batch_size: 1000) do |pab|
+      RakeHelpers.print_status_with_label("Creating post new bookmarks.......", i, post_action_bookmark_count)
+      now = Time.zone.now
+      bookmarks_to_create << {
+        topic_id: pab.topic_id,
+        post_id: pab.post_id,
+        user_id: pab.user_id,
+        created_at: now,
+        updated_at: now
+      }
+
+      i += 1
+    end
+
+    # this will ignore conflicts in the bookmarks table so
+    # if the user already has a post bookmarked in the new way,
+    # then we don't error and keep on truckin'
+    #
+    # we shouldn't have duplicates here at any rate because of
+    # the above LEFT JOIN but best to be safe knowing this
+    # won't blow up
+    Bookmark.insert_all(bookmarks_to_create)
+  end
+
+  RakeHelpers.print_status_with_label("Bookmark creation complete!            ", i, post_action_bookmark_count)
+  puts ""
+end

spec/fabricators/bookmark_fabricator.rb

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

spec/requests/bookmarks_controller_spec.rb

@@ -5,6 +5,7 @@ require 'rails_helper'
 describe BookmarksController do
   let(:current_user) { Fabricate(:user) }
   let(:bookmark_post) { Fabricate(:post) }
+  let(:bookmark_user) { current_user }
 
   before do
     sign_in(current_user)
@@ -13,7 +14,7 @@ describe BookmarksController do
   describe "#create" do
     context "if the user already has bookmarked the post" do
       before do
-        Fabricate(:bookmark, post: bookmark_post, user: current_user)
+        Fabricate(:bookmark, post: bookmark_post, user: bookmark_user)
       end
 
       it "returns failed JSON with a 422 error" do
@@ -44,4 +45,38 @@ describe BookmarksController do
       end
     end
   end
+
+  describe "#destroy" do
+    let!(:bookmark) { Fabricate(:bookmark, post: bookmark_post, user: bookmark_user) }
+
+    it "destroys the bookmark" do
+      delete "/bookmarks/#{bookmark.id}.json"
+      expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
+    end
+
+    context "if the bookmark has already been destroyed" do
+      it "returns failed JSON with a 403 error" do
+        bookmark.destroy!
+        delete "/bookmarks/#{bookmark.id}.json"
+
+        expect(response.status).to eq(404)
+        expect(JSON.parse(response.body)['errors'].first).to include(
+          I18n.t("not_found")
+        )
+      end
+    end
+
+    context "if the bookmark does not belong to the user" do
+      let(:bookmark_user) { Fabricate(:user) }
+
+      it "returns failed JSON with a 403 error" do
+        delete "/bookmarks/#{bookmark.id}.json"
+
+        expect(response.status).to eq(403)
+        expect(JSON.parse(response.body)['errors'].first).to include(
+          I18n.t("invalid_access")
+        )
+      end
+    end
+  end
 end

spec/requests/topics_controller_spec.rb

@@ -2327,6 +2327,64 @@ RSpec.describe TopicsController do
       put "/t/#{pm.topic_id}/bookmark.json"
       expect(response).to be_forbidden
     end
+
+    context "when SiteSetting.enable_bookmarks_with_reminders is true" do
+      before do
+        SiteSetting.enable_bookmarks_with_reminders = true
+      end
+      it "deletes all the bookmarks for the user in the topic" do
+        sign_in(user)
+        post = create_post
+        Fabricate(:bookmark, post: post, topic: post.topic, user: user)
+        put "/t/#{post.topic_id}/remove_bookmarks.json"
+        expect(Bookmark.where(user: user, topic: topic).count).to eq(0)
+      end
+    end
+  end
+
+  describe "#bookmark" do
+    before do
+      sign_in(user)
+    end
+
+    it "should create a new post action for the bookmark on the first post of the topic" do
+      post = create_post
+      post2 = create_post(topic_id: post.topic_id)
+      put "/t/#{post.topic_id}/bookmark.json"
+
+      expect(PostAction.find_by(user_id: user.id, post_action_type: PostActionType.types[:bookmark]).post_id).to eq(post.id)
+    end
+
+    it "errors if the topic is already bookmarked for the user" do
+      post = create_post
+      PostActionCreator.new(user, post, PostActionType.types[:bookmark]).perform
+
+      put "/t/#{post.topic_id}/bookmark.json"
+      expect(response).to be_forbidden
+    end
+
+    context "when SiteSetting.enable_bookmarks_with_reminders is true" do
+      before do
+        SiteSetting.enable_bookmarks_with_reminders = true
+      end
+      it "should create a new bookmark on the first post of the topic" do
+        post = create_post
+        post2 = create_post(topic_id: post.topic_id)
+        put "/t/#{post.topic_id}/bookmark.json"
+
+        bookmarks_for_topic = Bookmark.where(topic: post.topic, user: user)
+        expect(bookmarks_for_topic.count).to eq(1)
+        expect(bookmarks_for_topic.first.post_id).to eq(post.id)
+      end
+
+      it "errors if the topic is already bookmarked for the user" do
+        post = create_post
+        Bookmark.create(post: post, topic: post.topic, user: user)
+
+        put "/t/#{post.topic_id}/bookmark.json"
+        expect(response).to be_forbidden
+      end
+    end
   end
 
   describe '#reset_new' do

spec/serializers/topic_list_item_serializer_spec.rb

@@ -6,11 +6,10 @@ describe TopicListItemSerializer do
   let(:topic) do
     date = Time.zone.now
 
-    Fabricate.build(:topic,
-      title: 'test',
+    Fabricate(:topic,
+      title: 'This is a test topic title',
       created_at: date - 2.minutes,
-      bumped_at: date,
-      posters: [],
+      bumped_at: date
     )
   end
 
@@ -18,7 +17,7 @@ describe TopicListItemSerializer do
     SiteSetting.topic_featured_link_enabled = true
     serialized = TopicListItemSerializer.new(topic, scope: Guardian.new, root: false).as_json
 
-    expect(serialized[:title]).to eq("test")
+    expect(serialized[:title]).to eq("This is a test topic title")
     expect(serialized[:bumped]).to eq(true)
     expect(serialized[:featured_link]).to eq(nil)
     expect(serialized[:featured_link_root_domain]).to eq(nil)

spec/tasks/bookmarks_spec.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe "bookmarks tasks" do
+  let(:user1) { Fabricate(:user) }
+  let(:user2) { Fabricate(:user) }
+  let(:user3) { Fabricate(:user) }
+  let(:post1) { Fabricate(:post) }
+  let(:post2) { Fabricate(:post) }
+  let(:post3) { Fabricate(:post) }
+
+  before do
+    Rake::Task.clear
+    Discourse::Application.load_tasks
+
+    create_post_actions_and_existing_bookmarks
+  end
+
+  it "migrates all PostActions" do
+    Rake::Task['bookmarks:sync_to_table'].invoke
+
+    expect(Bookmark.all.count).to eq(3)
+  end
+
+  it "does not create bookmarks that already exist in the bookmarks table for a user" do
+    Fabricate(:bookmark, user: user1, post: post1)
+
+    Rake::Task['bookmarks:sync_to_table'].invoke
+
+    expect(Bookmark.all.count).to eq(3)
+    expect(Bookmark.where(post: post1, user: user1).count).to eq(1)
+  end
+
+  it "respects the sync_limit if provided and stops creating bookmarks at the limit (so this can be run progrssively" do
+    Rake::Task['bookmarks:sync_to_table'].invoke(1)
+    expect(Bookmark.all.count).to eq(1)
+  end
+
+  def create_post_actions_and_existing_bookmarks
+    Fabricate(:post_action, user: user1, post: post1, post_action_type_id: PostActionType.types[:bookmark])
+    Fabricate(:post_action, user: user2, post: post2, post_action_type_id: PostActionType.types[:bookmark])
+    Fabricate(:post_action, user: user3, post: post3, post_action_type_id: PostActionType.types[:bookmark])
+  end
+end

test/javascripts/acceptance/topic-test.js.es6

@@ -333,3 +333,17 @@ QUnit.test("Quoting a quote keeps the original poster name", async assert => {
       .indexOf('quote="codinghorror said, post:3, topic:280"') !== -1
   );
 });
+
+acceptance("Topic + Post Bookmarks with Reminders", {
+  loggedIn: true,
+  settings: {
+    enable_bookmarks_with_reminders: true
+  }
+});
+
+QUnit.test("Bookmarks Modal", async assert => {
+  await visit("/t/internationalization-localization/280");
+  await click(".topic-post:first-child button.show-more-actions");
+  await click(".topic-post:first-child button.bookmark");
+  assert.ok(exists("#bookmark-reminder-modal"), "it shows the bookmark modal");
+});

test/javascripts/helpers/site-settings.js

@@ -12,8 +12,8 @@ Discourse.SiteSettingsOriginal = {
   ga_universal_tracking_code: "",
   ga_universal_domain_name: "auto",
   top_menu: "latest|new|unread|categories|top",
-  post_menu: "like|share|flag|edit|bookmark|delete|admin|reply",
-  post_menu_hidden_items: "flag|edit|delete|admin",
+  post_menu: "like|share|flag|edit|bookmark|bookmarkWithReminder|delete|admin|reply",
+  post_menu_hidden_items: "flag|bookmark|bookmarkWithReminder|edit|delete|admin",
   share_links: "twitter|facebook|email",
   category_colors:
     "BF1E2E|F1592A|F7941D|9EB83B|3AB54A|12A89D|25AAE2|0E76BD|652D90|92278F|ED207B|8C6238|231F20|27AA5B|B3B5B4|E45735",
@@ -28,6 +28,7 @@ Discourse.SiteSettingsOriginal = {
   allow_new_registrations: true,
   enable_google_logins: true,
   enable_google_oauth2_logins: false,
+  enable_bookmarks_with_reminders: false,
   enable_twitter_logins: true,
   enable_facebook_logins: true,
   enable_github_logins: true,