FEATURE: Promote bookmarks with reminders to core functionality (#9369)

The main thrust of this PR is to take all the conditional checks based on the `enable_bookmarks_with_reminders` away and only keep the code from the `true` path, making bookmarks with reminders the core bookmarks feature. There is also a migration to create `Bookmark` records out of `PostAction` bookmarks for a site.

### Summary

* Remove logic based on whether enable_bookmarks_with_reminders is true. This site setting is now obsolete, the old bookmark functionality is being removed. Retain the setting and set the value to `true` in a migration.
* Use the code from the rake task to create a database migration that creates bookmarks from post actions.
* Change the bookmark report to read from the new table.
* Get rid of old endpoints for bookmarks
* Link to the new bookmarks list from the user summary page

app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js

@@ -1,6 +1,5 @@
 import Controller from "@ember/controller";
 import ModalFunctionality from "discourse/mixins/modal-functionality";
-import { setting } from "discourse/lib/computed";
 
 const KEY = "keyboard_shortcuts_help";
 
@@ -57,8 +56,6 @@ export default Controller.extend(ModalFunctionality, {
     this.set("shortcuts", null);
   },
 
-  showBookmarkShortcuts: setting("enable_bookmarks_with_reminders"),
-
   _defineShortcuts() {
     this.set("shortcuts", {
       jump_to: {

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

@@ -652,10 +652,7 @@ export default Controller.extend(bufferedProperty("model"), {
       if (!this.currentUser) {
         return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
       } else if (post) {
-        if (this.siteSettings.enable_bookmarks_with_reminders) {
+        return post.toggleBookmarkWithReminder();
-          return post.toggleBookmarkWithReminder();
-        }
-        return post.toggleBookmark().catch(popupAjaxError);
       } else {
         return this.model.toggleBookmark().then(changedIds => {
           if (!changedIds) {

app/assets/javascripts/discourse/controllers/user-activity.js

@@ -3,7 +3,6 @@ import { inject as service } from "@ember/service";
 import Controller, { inject as controller } from "@ember/controller";
 import { exportUserArchive } from "discourse/lib/export-csv";
 import { observes } from "discourse-common/utils/decorators";
-import { setting } from "discourse/lib/computed";
 
 export default Controller.extend({
   application: controller(),
@@ -12,7 +11,6 @@ export default Controller.extend({
   userActionType: null,
 
   canDownloadPosts: alias("user.viewingSelf"),
-  bookmarksWithRemindersEnabled: setting("enable_bookmarks_with_reminders"),
 
   @observes("userActionType", "model.stream.itemsLoaded")
   _showFooter: function() {

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

@@ -400,10 +400,8 @@ const Topic = RestModel.extend({
   afterTopicBookmarked(firstPost) {
     if (firstPost) {
       firstPost.set("bookmarked", true);
-      if (this.siteSettings.enable_bookmarks_with_reminders) {
+      firstPost.set("bookmarked_with_reminder", true);
-        this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at);
+      this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at);
-        firstPost.set("bookmarked_with_reminder", true);
-      }
       return [firstPost.id];
     }
   },
@@ -438,24 +436,10 @@ const Topic = RestModel.extend({
     return this.firstPost().then(firstPost => {
       const toggleBookmarkOnServer = () => {
         if (bookmark) {
-          if (this.siteSettings.enable_bookmarks_with_reminders) {
+          return firstPost.toggleBookmarkWithReminder().then(() => {
-            return firstPost.toggleBookmarkWithReminder().then(response => {
+            this.set("bookmarking", false);
-              this.set("bookmarking", false);
+            return this.afterTopicBookmarked(firstPost);
-              if (response && response.closedWithoutSaving) {
+          });
-                this.set("bookmarked", false);
-              } else {
-                return this.afterTopicBookmarked(firstPost);
-              }
-            });
-          } else {
-            return ajax(`/t/${this.id}/bookmark`, { type: "PUT" })
-              .then(() => {
-                this.toggleProperty("bookmarked");
-                return this.afterTopicBookmarked(firstPost);
-              })
-              .catch(popupAjaxError)
-              .finally(() => this.set("bookmarking", false));
-          }
         } else {
           return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" })
             .then(() => {
@@ -474,10 +458,7 @@ const Topic = RestModel.extend({
                     post.set("bookmarked", false);
                     updated.push(post.id);
                   }
-                  if (
+                  if (post.bookmarked_with_reminder) {
-                    this.siteSettings.enable_bookmarks_with_reminders &&
-                    post.bookmarked_with_reminder
-                  ) {
                     post.setProperties(clearedBookmarkProps);
                     updated.push(post.id);
                   }

app/assets/javascripts/discourse/routes/discovery.js

@@ -17,10 +17,7 @@ export default DiscourseRoute.extend(OpenComposer, {
     // including being able to show links to multiple posts to the same topic
     // and being based on a different model. better to just redirect
     const url = transition.intent.url;
-    if (
+    if (url === "/bookmarks") {
-      this.siteSettings.enable_bookmarks_with_reminders &&
-      url === "/bookmarks"
-    ) {
       this.transitionTo(
         "userActivity.bookmarksWithReminders",
         this.currentUser

app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs

@@ -55,24 +55,22 @@
         <li>{{html-safe shortcuts.actions.quote_post}}</li>
       </ul>
     </section>
-    {{#if showBookmarkShortcuts}}
+    <section class="keyboard-shortcuts-bookmark-section">
-      <section class="keyboard-shortcuts-bookmark-section">
+      <h4>{{i18n "keyboard_shortcuts_help.bookmarks.title"}}</h4>
-        <h4>{{i18n "keyboard_shortcuts_help.bookmarks.title"}}</h4>
+      <ul>
-        <ul>
+        <li>{{html-safe shortcuts.bookmarks.enter}}</li>
-          <li>{{html-safe shortcuts.bookmarks.enter}}</li>
+        <li>{{html-safe shortcuts.bookmarks.later_today}}</li>
-          <li>{{html-safe shortcuts.bookmarks.later_today}}</li>
+        <li>{{html-safe shortcuts.bookmarks.later_this_week}}</li>
-          <li>{{html-safe shortcuts.bookmarks.later_this_week}}</li>
+        <li>{{html-safe shortcuts.bookmarks.tomorrow}}</li>
-          <li>{{html-safe shortcuts.bookmarks.tomorrow}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_week}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_week}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_month}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_month}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_business_week}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_business_week}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_business_day}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_business_day}}</li>
+        <li>{{html-safe shortcuts.bookmarks.custom}}</li>
-          <li>{{html-safe shortcuts.bookmarks.custom}}</li>
+        <li>{{html-safe shortcuts.bookmarks.none}}</li>
-          <li>{{html-safe shortcuts.bookmarks.none}}</li>
+        <li>{{html-safe shortcuts.bookmarks.delete}}</li>
-          <li>{{html-safe shortcuts.bookmarks.delete}}</li>
+      </ul>
-        </ul>
+    </section>
-      </section>
-    {{/if}}
   </div>
   <div class="column">
     <section>

app/assets/javascripts/discourse/templates/user/activity.hbs

@@ -18,15 +18,9 @@
       {{#link-to "userActivity.likesGiven"}}{{i18n "user_action_groups.1"}}{{/link-to}}
     </li>
     {{#if user.showBookmarks}}
-      {{#if bookmarksWithRemindersEnabled}}
+      <li>
-        <li>
+        {{#link-to 'userActivity.bookmarksWithReminders'}}{{i18n 'user_action_groups.3'}}{{/link-to}}
-          {{#link-to "userActivity.bookmarksWithReminders"}}{{i18n "user_action_groups.3"}}{{/link-to}}
+      </li>
-        </li>
-      {{else}}
-        <li>
-          {{#link-to "userActivity.bookmarks"}}{{i18n "user_action_groups.3"}}{{/link-to}}
-        </li>
-      {{/if}}
     {{/if}}
     {{plugin-outlet
       name="user-activity-bottom"

app/assets/javascripts/discourse/templates/user/summary.hbs

@@ -27,7 +27,7 @@
         </li>
         {{#if model.bookmark_count}}
           <li class="linked-stat">
-            {{#link-to "userActivity.bookmarks"}}
+            {{#link-to "userActivity.bookmarksWithReminders"}}
               {{user-stat value=model.bookmark_count label="user.summary.bookmark_count"}}
             {{/link-to}}
           </li>

app/assets/javascripts/discourse/widgets/post-menu.js

@@ -302,8 +302,8 @@ registerButton("bookmark", attrs => {
   };
 });
 
-registerButton("bookmarkWithReminder", (attrs, state, siteSettings) => {
+registerButton("bookmarkWithReminder", attrs => {
-  if (!attrs.canBookmark || !siteSettings.enable_bookmarks_with_reminders) {
+  if (!attrs.canBookmark) {
     return;
   }
 
@@ -469,10 +469,7 @@ export default createWidget("post-menu", {
 
     // filter menu items based on site settings
     const orderedButtons = this.menuItems().filter(button => {
-      if (
+      if (button === "bookmark") {
-        this.siteSettings.enable_bookmarks_with_reminders &&
-        button === "bookmark"
-      ) {
         return false;
       }
       return true;

app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js

@@ -16,11 +16,7 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", {
   },
 
   showAllHref() {
-    if (this.siteSettings.enable_bookmarks_with_reminders) {
+    return `${this.attrs.path}/activity/bookmarks-with-reminders`;
-      return `${this.attrs.path}/activity/bookmarks-with-reminders`;
-    } else {
-      return `${this.attrs.path}/activity/bookmarks`;
-    }
   },
 
   emptyStatePlaceholderItem() {
@@ -28,11 +24,7 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", {
   },
 
   findNewItems() {
-    if (this.siteSettings.enable_bookmarks_with_reminders) {
+    return this.loadBookmarksWithReminders();
-      return this.loadBookmarksWithReminders();
-    } else {
-      return this.loadUserActivityBookmarks();
-    }
   },
 
   itemHtml(bookmark) {

app/assets/javascripts/discourse/widgets/user-menu.js

@@ -56,16 +56,13 @@ createWidget("user-menu-links", {
   },
 
   bookmarksGlyph() {
-    let path = this.siteSettings.enable_bookmarks_with_reminders
-      ? "bookmarks-with-reminders"
-      : "bookmarks";
     return {
       action: UserMenuAction.QUICK_ACCESS,
       actionParam: QuickAccess.BOOKMARKS,
       label: "user.bookmarks",
       className: "user-bookmarks-link",
       icon: "bookmark",
-      href: `${this.attrs.path}/activity/${path}`
+      href: `${this.attrs.path}/activity/bookmarks-with-reminders`
     };
   },
 

app/controllers/posts_controller.rb

@@ -488,26 +488,6 @@ class PostsController < ApplicationController
     render body: nil
   end
 
-  def bookmark
-    if params[:bookmarked] == "true"
-      post = find_post_from_params
-      result = PostActionCreator.create(current_user, post, :bookmark)
-      return render_json_error(result) if result.failed?
-    else
-      post_action = PostAction.find_by(post_id: params[:post_id], user_id: current_user.id)
-      raise Discourse::NotFound unless post_action
-
-      post = Post.with_deleted.find_by(id: post_action&.post_id)
-      raise Discourse::NotFound unless post
-
-      result = PostActionDestroyer.destroy(current_user, post, :bookmark)
-      return render_json_error(result) if result.failed?
-    end
-
-    topic_user = TopicUser.get(post.topic, current_user)
-    render_json_dump(topic_bookmarked: topic_user.try(:bookmarked))
-  end
-
   def destroy_bookmark
     params.require(:post_id)
 

app/controllers/topics_controller.rb

@@ -97,9 +97,9 @@ class TopicsController < ApplicationController
       Notification.remove_for(current_user.id, params[:topic_id]) if current_user
 
       deleted = guardian.can_see_topic?(ex.obj, false) ||
-                (!guardian.can_see_topic?(ex.obj) &&
+        (!guardian.can_see_topic?(ex.obj) &&
-                 ex.obj&.access_topic_via_group &&
+         ex.obj&.access_topic_via_group &&
-                 ex.obj.deleted_at)
+         ex.obj.deleted_at)
 
       if SiteSetting.detailed_404
         if deleted
@@ -231,11 +231,14 @@ class TopicsController < ApplicationController
 
     fetch_topic_view(options)
 
-    render_json_dump(TopicViewPostsSerializer.new(@topic_view,
+    render_json_dump(
-      scope: guardian,
+      TopicViewPostsSerializer.new(
-      root: false,
+        @topic_view,
-      include_raw: !!params[:include_raw]
+        scope: guardian,
-    ))
+        root: false,
+        include_raw: !!params[:include_raw]
+      )
+    )
   end
 
   def excerpts
@@ -261,12 +264,12 @@ class TopicsController < ApplicationController
       .joins("LEFT JOIN users u on u.id = posts.user_id")
       .pluck(:id, :cooked, :username)
       .map do |post_id, cooked, username|
-        {
+      {
-          post_id: post_id,
+        post_id: post_id,
-          username: username,
+        username: username,
-          excerpt: PrettyText.excerpt(cooked, 800, keep_emoji_images: true)
+        excerpt: PrettyText.excerpt(cooked, 800, keep_emoji_images: true)
-        }
+      }
-      end
+    end
 
     render json: @posts.to_json
   end
@@ -490,18 +493,7 @@ class TopicsController < ApplicationController
 
   def remove_bookmarks
     topic = Topic.find(params[:topic_id].to_i)
-
+    BookmarkManager.new(current_user).destroy_for_topic(topic)
-    if SiteSetting.enable_bookmarks_with_reminders?
-      BookmarkManager.new(current_user).destroy_for_topic(topic)
-    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)
-      end
-    end
-
     render body: nil
   end
 
@@ -552,16 +544,11 @@ class TopicsController < ApplicationController
     topic = Topic.find(params[:topic_id].to_i)
     first_post = topic.ordered_posts.first
 
-    if SiteSetting.enable_bookmarks_with_reminders?
+    bookmark_manager = BookmarkManager.new(current_user)
-      bookmark_manager = BookmarkManager.new(current_user)
+    bookmark_manager.create(post_id: first_post.id)
-      bookmark_manager.create(post_id: first_post.id)
 
-      if bookmark_manager.errors.any?
+    if bookmark_manager.errors.any?
-        return render_json_error(bookmark_manager, status: 400)
+      return render_json_error(bookmark_manager, status: 400)
-      end
-    else
-      result = PostActionCreator.create(current_user, first_post, :bookmark)
-      return render_json_error(result) if result.failed?
     end
 
     render body: nil
@@ -628,9 +615,9 @@ class TopicsController < ApplicationController
     topic = Topic.find_by(id: params[:topic_id])
 
     unless pm_has_slots?(topic)
-      return render_json_error(I18n.t("pm_reached_recipients_limit",
+      return render_json_error(
-        recipients_limit: SiteSetting.max_allowed_message_recipients
+        I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients)
-      ))
+      )
     end
 
     if topic.private_message?
@@ -654,9 +641,9 @@ class TopicsController < ApplicationController
     )
 
     unless pm_has_slots?(topic)
-      return render_json_error(I18n.t("pm_reached_recipients_limit",
+      return render_json_error(
-        recipients_limit: SiteSetting.max_allowed_message_recipients
+        I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients)
-      ))
+      )
     end
 
     guardian.ensure_can_invite_to!(topic)
@@ -683,7 +670,8 @@ class TopicsController < ApplicationController
 
           if group_names.present?
             json.merge!(errors: [
-              I18n.t("topic_invite.failed_to_invite",
+              I18n.t(
+                "topic_invite.failed_to_invite",
                 group_names: group_names
               )
             ])
@@ -1011,7 +999,8 @@ class TopicsController < ApplicationController
       return
     end
 
-    topic_view_serializer = TopicViewSerializer.new(@topic_view,
+    topic_view_serializer = TopicViewSerializer.new(
+      @topic_view,
       scope: guardian,
       root: false,
       include_raw: !!params[:include_raw]

app/jobs/scheduled/bookmark_reminder_notifications.rb

@@ -21,8 +21,6 @@ module Jobs
     end
 
     def execute(args = nil)
-      return if !SiteSetting.enable_bookmarks_with_reminders?
-
       bookmarks = Bookmark.pending_reminders
         .where.not(reminder_type: Bookmark.reminder_types[:at_desktop])
         .includes(:user).order('reminder_at ASC')

app/models/bookmark.rb

@@ -72,6 +72,16 @@ class Bookmark < ActiveRecord::Base
       later_this_week: 8
     )
   end
+
+  def self.count_per_day(opts = nil)
+    opts ||= {}
+    result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago)
+    result = result.where('bookmarks.created_at <= ?', opts[:end_date]) if opts[:end_date]
+    result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id])) if opts[:category_id]
+    result.group('date(bookmarks.created_at)')
+      .order('date(bookmarks.created_at)')
+      .count
+  end
 end
 
 # == Schema Information

app/models/reports/bookmarks.rb

@@ -3,5 +3,16 @@
 Report.add_report('bookmarks') do |report|
   report.icon = 'bookmark'
 
-  post_action_report report, PostActionType.types[:bookmark]
+  category_filter = report.filters.dig(:category)
+  report.add_filter('category', default: category_filter)
+
+  report.data = []
+  Bookmark.count_per_day(
+    category_id: category_filter,
+    start_date: report.start_date,
+    end_date: report.end_date
+  ).each do |date, count|
+    report.data << { x: date, y: count }
+  end
+  add_counts report, Bookmark, 'bookmarks.created_at'
 end

app/models/user_summary.rb

@@ -117,14 +117,7 @@ class UserSummary
   end
 
   def bookmark_count
-    if SiteSetting.enable_bookmarks_with_reminders
+    Bookmark.where(user: @user).count
-      Bookmark.where(user: @user).count
-    else
-      UserAction
-        .where(user: @user)
-        .where(action_type: UserAction::BOOKMARK)
-        .count
-    end
   end
 
   def recent_time_read

app/serializers/post_serializer.rb

@@ -345,7 +345,7 @@ class PostSerializer < BasicPostSerializer
   end
 
   def post_bookmark
-    return nil if !SiteSetting.enable_bookmarks_with_reminders? || @topic_view.blank?
+    return nil if @topic_view.blank?
     @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id }
   end
 

app/serializers/topic_view_serializer.rb

@@ -186,15 +186,11 @@ class TopicViewSerializer < ApplicationSerializer
   end
 
   def bookmarked
-    if SiteSetting.enable_bookmarks_with_reminders?
+    object.has_bookmarks?
-      object.has_bookmarks?
-    else
-      object.topic_user&.bookmarked
-    end
   end
 
   def include_bookmark_reminder_at?
-    SiteSetting.enable_bookmarks_with_reminders? && bookmarked
+    bookmarked
   end
 
   def bookmark_reminder_at

config/routes.rb

@@ -585,7 +585,6 @@ Discourse::Application.routes.draw do
   put "admin/groups/:id/members" => "groups#add_members", constraints: AdminConstraint.new
 
   resources :posts do
-    put "bookmark"
     delete "bookmark", to: "posts#destroy_bookmark"
     put "wiki"
     put "post_type"

config/site_settings.yml

@@ -291,7 +291,7 @@ basic:
     default: false
   enable_bookmarks_with_reminders:
     client: true
-    default: false
+    default: true
     hidden: true
   enable_bookmark_at_desktop_reminders:
     client: true

db/migrate/20200409033412_create_bookmarks_from_post_action_bookmarks.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+class CreateBookmarksFromPostActionBookmarks < ActiveRecord::Migration[6.0]
+  def up
+    SiteSetting.enable_bookmarks_with_reminders = true
+
+    bookmarks_to_create = []
+    loop do
+      # post action type id 1 is :bookmark. we do not need to OFFSET here for
+      # paging because the WHERE bookmarks.id IS NULL clause handles this effectively,
+      # because we do not get bookmarks back that have already been inserted
+      post_action_bookmarks = DB.query(
+        <<~SQL, type_id: 1
+        SELECT post_actions.id, post_actions.post_id, posts.topic_id, post_actions.user_id
+        FROM post_actions
+        INNER JOIN posts ON posts.id = post_actions.post_id
+        LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id
+        INNER JOIN topics ON topics.id = posts.topic_id
+        WHERE bookmarks.id IS NULL AND post_action_type_id = :type_id AND post_actions.deleted_at IS NULL AND posts.deleted_at IS NULL
+        LIMIT 2000
+        SQL
+      )
+      break if post_action_bookmarks.count.zero?
+
+      post_action_bookmarks.each do |pab|
+        now = Time.zone.now
+        bookmarks_to_create << "(#{pab.topic_id}, #{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')"
+      end
+
+      create_bookmarks(bookmarks_to_create)
+      bookmarks_to_create = []
+    end # loop
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+
+  def create_bookmarks(bookmarks_to_create)
+    return if bookmarks_to_create.empty?
+
+    # 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
+    #
+    DB.exec(
+      <<~SQL
+      INSERT INTO bookmarks (topic_id, post_id, user_id, created_at, updated_at)
+      VALUES #{bookmarks_to_create.join(",\n")}
+      ON CONFLICT DO NOTHING
+      SQL
+    )
+  end
+end

lib/bookmark_reminder_notification_handler.rb

@@ -52,8 +52,6 @@ class BookmarkReminderNotificationHandler
   end
 
   def self.send_at_desktop_reminder(user:, request_user_agent:)
-    return if !SiteSetting.enable_bookmarks_with_reminders
-
     return if MobileDetection.mobile_device?(request_user_agent)
 
     return if !user_has_pending_at_desktop_reminders?(user)

lib/search.rb

@@ -381,11 +381,7 @@ class Search
 
   advanced_filter(/^in:(bookmarks)$/) do |posts, match|
     if @guardian.user
-      if SiteSetting.enable_bookmarks_with_reminders
+      posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})")
-        posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})")
-      else
-        post_action_type_filter(posts, PostActionType.types[:bookmark])
-      end
     end
   end
 

lib/tasks/bookmarks.rake

@@ -9,58 +9,40 @@ require_dependency "rake_helpers"
 # 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|
+task "bookmarks:sync_to_table" => :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')
-    .joins('INNER JOIN topics ON topics.id = posts.topic_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')
-  i = 0
-
   bookmarks_to_create = []
-  post_action_bookmarks.find_each(batch_size: 2000) do |pab|
+  loop do
-    RakeHelpers.print_status_with_label("Creating post new bookmarks.......", i, post_action_bookmark_count)
+    # post action type id 1 is :bookmark. we do not need to OFFSET here for
-    now = Time.zone.now
+    # paging because the WHERE bookmarks.id IS NULL clause handles this effectively,
-    bookmarks_to_create << {
+    # because we do not get bookmarks back that have already been inserted
-      topic_id: pab.topic_id,
+    post_action_bookmarks = DB.query(
-      post_id: pab.post_id,
+      <<~SQL, type_id: 1
-      user_id: pab.user_id,
+        SELECT post_actions.id, post_actions.post_id, posts.topic_id, post_actions.user_id
-      created_at: now,
+        FROM post_actions
-      updated_at: now
+        INNER JOIN posts ON posts.id = post_actions.post_id
-    }
+        LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id
+        INNER JOIN topics ON topics.id = posts.topic_id
+        WHERE bookmarks.id IS NULL AND post_action_type_id = :type_id AND post_actions.deleted_at IS NULL AND posts.deleted_at IS NULL
+        LIMIT 2000
+      SQL
+    )
+    break if post_action_bookmarks.count.zero?
 
-    i += 1
+    post_action_bookmarks.each do |pab|
-
+      now = Time.zone.now
-    # once we get to 2000 records to create, insert them all and reset
+      bookmarks_to_create << "(#{pab.topic_id}, #{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')"
-    # the array and counter to make sure we don't have too many in memory
-    if i >= 2000
-      create_bookmarks(bookmarks_to_create)
-      i = 0
-      bookmarks_to_create = []
     end
-  end
 
-  # if there was < 2000 bookmarks this finishes off the last ones
+    create_bookmarks(bookmarks_to_create)
-  create_bookmarks(bookmarks_to_create)
+    bookmarks_to_create = []
+  end # loop
 
-  RakeHelpers.print_status_with_label("Bookmark creation complete!            ", i, post_action_bookmark_count)
+  puts "Bookmark creation complete!"
-  puts ""
 end
 
 def create_bookmarks(bookmarks_to_create)
+  return if bookmarks_to_create.empty?
+
   # 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'
@@ -68,5 +50,12 @@ def create_bookmarks(bookmarks_to_create)
   # 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)
+  #
+  DB.exec(
+    <<~SQL
+      INSERT INTO bookmarks (topic_id, post_id, user_id, created_at, updated_at)
+      VALUES #{bookmarks_to_create.join(",\n")}
+      ON CONFLICT DO NOTHING
+    SQL
+  )
 end

plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb

@@ -238,11 +238,7 @@ module DiscourseNarrativeBot
       return unless @post.user_id == self.discobot_user.id
 
       profile_page_url = url_helpers(:user_url, username: @user.username)
-      bookmark_url = if SiteSetting.enable_bookmarks_with_reminders?
+      bookmark_url = "#{profile_page_url}/activity/bookmarks-with-reminders"
-        "#{profile_page_url}/activity/bookmarks-with-reminders"
-      else
-        "#{profile_page_url}/activity/bookmarks"
-      end
       raw = <<~RAW
         #{I18n.t("#{I18N_KEY}.bookmark.reply", i18n_post_args(bookmark_url: bookmark_url))}
 

plugins/discourse-narrative-bot/plugin.rb

@@ -241,10 +241,8 @@ after_initialize do
   end
 
   self.add_model_callback(Bookmark, :after_commit, on: :create) do
-    if SiteSetting.enable_bookmarks_with_reminders?
+    if self.post && self.user.enqueue_narrative_bot_job?
-      if self.post && self.user.enqueue_narrative_bot_job?
+      Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: :bookmark)
-        Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: :bookmark)
-      end
     end
   end
 

plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb

@@ -247,26 +247,7 @@ describe DiscourseNarrativeBot::NewUserNarrative do
         end
       end
 
-      it 'should create the right reply' do
-        post.update!(user: discobot_user)
-        narrative.expects(:enqueue_timeout_job).with(user)
-
-        narrative.input(:bookmark, user, post: post)
-        new_post = Post.last
-        profile_page_url = "#{Discourse.base_url}/u/#{user.username}"
-
-        expected_raw = <<~RAW
-          #{I18n.t('discourse_narrative_bot.new_user_narrative.bookmark.reply', bookmark_url: "#{profile_page_url}/activity/bookmarks", base_uri: '')}
-
-          #{I18n.t('discourse_narrative_bot.new_user_narrative.onebox.instructions', base_uri: '')}
-        RAW
-
-        expect(new_post.raw).to eq(expected_raw.chomp)
-        expect(narrative.get_data(user)[:state].to_sym).to eq(:tutorial_onebox)
-      end
-
       it 'should create the right reply when bookmarks with reminders are enabled' do
-        SiteSetting.enable_bookmarks_with_reminders = true
         post.update!(user: discobot_user)
         narrative.expects(:enqueue_timeout_job).with(user)
 

spec/jobs/bookmark_reminder_notifications_spec.rb

@@ -18,7 +18,6 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
   end
 
   before do
-    SiteSetting.enable_bookmarks_with_reminders = true
     # this is done to avoid model validations on Bookmark
     bookmark1.update_column(:reminder_at, five_minutes_ago - 10.minutes)
     bookmark2.update_column(:reminder_at, five_minutes_ago - 5.minutes)
@@ -63,14 +62,6 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
     expect(Discourse.redis.get(described_class::JOB_RUN_NUMBER_KEY)).to eq('1')
   end
 
-  context "when the bookmark with reminder site setting is disabled" do
-    it "does nothing" do
-      Bookmark.expects(:where).never
-      SiteSetting.enable_bookmarks_with_reminders = false
-      subject.execute
-    end
-  end
-
   context "when one of the bookmark reminder types is at_desktop" do
     let(:bookmark1) { Fabricate(:bookmark, reminder_type: :at_desktop) }
     it "is not included in the run, because it is not time-based" do

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -8,7 +8,6 @@ RSpec.describe BookmarkReminderNotificationHandler do
   fab!(:user) { Fabricate(:user) }
 
   before do
-    SiteSetting.enable_bookmarks_with_reminders = true
     Discourse.redis.flushall
   end
 
@@ -89,17 +88,6 @@ RSpec.describe BookmarkReminderNotificationHandler do
       end
     end
 
-    context "when enable bookmarks with reminders is disabled" do
-      before do
-        SiteSetting.enable_bookmarks_with_reminders = false
-      end
-
-      it "does nothing" do
-        BrowserDetection.expects(:device).never
-        send_reminder
-      end
-    end
-
     def send_reminder
       subject.send_at_desktop_reminder(user: user, request_user_agent: user_agent)
     end

spec/requests/admin/users_controller_spec.rb

@@ -124,11 +124,11 @@ RSpec.describe Admin::UsersController do
   end
 
   describe '#suspend' do
-    fab!(:post) { Fabricate(:post) }
+    fab!(:created_post) { Fabricate(:post) }
     let(:suspend_params) do
       { suspend_until: 5.hours.from_now,
         reason: "because of this post",
-        post_id: post.id }
+        post_id: created_post.id }
     end
 
     it "works properly" do
@@ -156,13 +156,13 @@ RSpec.describe Admin::UsersController do
         expect(response.status).to eq(200)
 
         log = UserHistory.where(target_user_id: user.id).order('id desc').first
-        expect(log.post_id).to eq(post.id)
+        expect(log.post_id).to eq(created_post.id)
       end
 
       it "can delete an associated post" do
         put "/admin/users/#{user.id}/suspend.json", params: suspend_params.merge(post_action: 'delete')
-        post.reload
+        created_post.reload
-        expect(post.deleted_at).to be_present
+        expect(created_post.deleted_at).to be_present
         expect(response.status).to eq(200)
       end
 
@@ -200,17 +200,17 @@ RSpec.describe Admin::UsersController do
         reply = PostCreator.create(
           Fabricate(:user),
           raw: 'this is the reply text',
-          reply_to_post_number: post.post_number,
+          reply_to_post_number: created_post.post_number,
-          topic_id: post.topic_id
+          topic_id: created_post.topic_id
         )
         nested_reply = PostCreator.create(
           Fabricate(:user),
           raw: 'this is the reply text2',
           reply_to_post_number: reply.post_number,
-          topic_id: post.topic_id
+          topic_id: created_post.topic_id
         )
         put "/admin/users/#{user.id}/suspend.json", params: suspend_params.merge(post_action: 'delete_replies')
-        expect(post.reload.deleted_at).to be_present
+        expect(created_post.reload.deleted_at).to be_present
         expect(reply.reload.deleted_at).to be_present
         expect(nested_reply.reload.deleted_at).to be_present
         expect(response.status).to eq(200)
@@ -223,9 +223,9 @@ RSpec.describe Admin::UsersController do
         )
 
         expect(response.status).to eq(200)
-        post.reload
+        created_post.reload
-        expect(post.deleted_at).to be_blank
+        expect(created_post.deleted_at).to be_blank
-        expect(post.raw).to eq("this is the edited content")
+        expect(created_post.raw).to eq("this is the edited content")
         expect(response.status).to eq(200)
       end
     end
@@ -252,9 +252,8 @@ RSpec.describe Admin::UsersController do
 
     it "also prevents use of any api keys" do
       api_key = Fabricate(:api_key, user: user)
-
+      post "/bookmarks.json", params: {
-      put "/posts/#{Fabricate(:post).id}/bookmark.json", params: {
+        post_id: Fabricate(:post).id
-        bookmarked: "true"
       }, headers: { HTTP_API_KEY: api_key.key }
       expect(response.status).to eq(200)
 
@@ -264,10 +263,9 @@ RSpec.describe Admin::UsersController do
       user.reload
       expect(user).to be_suspended
 
-      put "/posts/#{Fabricate(:post).id}/bookmark.json", params: {
+      post "/bookmarks.json", params: {
-        bookmarked: "true",
+        post_id: Fabricate(:post).id
-        api_key: api_key.key
+      }, headers: { HTTP_API_KEY: api_key.key }
-      }
       expect(response.status).to eq(403)
     end
   end

spec/requests/posts_controller_spec.rb

@@ -482,131 +482,6 @@ describe PostsController do
     end
   end
 
-  describe '#bookmark' do
-    include_examples 'action requires login', :put, "/posts/2/bookmark.json"
-    let!(:post) { post_by_user }
-
-    describe 'when logged in' do
-      before do
-        sign_in(user)
-      end
-
-      fab!(:private_message) { Fabricate(:private_message_post) }
-
-      it "raises an error if the user doesn't have permission to see the post" do
-        put "/posts/#{private_message.id}/bookmark.json", params: { bookmarked: "true" }
-        expect(response).to be_forbidden
-      end
-
-      it 'creates a bookmark' do
-        put "/posts/#{post.id}/bookmark.json", params: { bookmarked: "true" }
-        expect(response.status).to eq(200)
-
-        post_action = PostAction.find_by(user: user, post: post)
-        expect(post_action.post_action_type_id).to eq(PostActionType.types[:bookmark])
-      end
-
-      context "removing a bookmark" do
-        let(:post_action) { PostActionCreator.create(user, post, :bookmark).post_action }
-
-        it "returns the right response when post is not bookmarked" do
-          put "/posts/#{post_by_user.id}/bookmark.json"
-          expect(response.status).to eq(404)
-        end
-
-        it "should be able to remove a bookmark" do
-          post_action
-          put "/posts/#{post.id}/bookmark.json"
-
-          expect(PostAction.find_by(id: post_action.id)).to eq(nil)
-        end
-
-        describe "when user doesn't have permission to see bookmarked post" do
-          it "should still be able to remove a bookmark" do
-            post_action
-            post = post_action.post
-            topic = post.topic
-            topic.convert_to_private_message(admin)
-            topic.remove_allowed_user(admin, user.username)
-
-            expect(Guardian.new(user).can_see_post?(post.reload)).to eq(false)
-
-            put "/posts/#{post.id}/bookmark.json"
-
-            expect(PostAction.find_by(id: post_action.id)).to eq(nil)
-          end
-        end
-
-        describe "when post has been deleted" do
-          it "should still be able to remove a bookmark" do
-            post = post_action.post
-            post.trash!
-
-            put "/posts/#{post.id}/bookmark.json"
-
-            expect(PostAction.find_by(id: post_action.id)).to eq(nil)
-          end
-        end
-      end
-    end
-
-    context "api" do
-      let(:api_key) { Fabricate(:api_key, user: user) }
-      let(:master_key) { Fabricate(:api_key, user: nil) }
-
-      # choosing an arbitrarily easy to mock trusted activity
-      it 'allows users with api key to bookmark posts' do
-        put "/posts/#{post.id}/bookmark.json",
-          params: { bookmarked: "true" },
-          headers: { HTTP_API_KEY: api_key.key }
-
-        expect(response.status).to eq(200)
-        expect(PostAction.where(
-          post: post,
-          user: user,
-          post_action_type_id: PostActionType.types[:bookmark]
-        ).count).to eq(1)
-      end
-
-      it 'raises an error with a user key that does not match an optionally specified username' do
-        put "/posts/#{post.id}/bookmark.json",
-          params: { bookmarked: "true" },
-          headers: { HTTP_API_KEY: api_key.key, HTTP_API_USERNAME: 'made_up' }
-
-        expect(response.status).to eq(403)
-      end
-
-      it 'allows users with a master api key to bookmark posts' do
-        put "/posts/#{post.id}/bookmark.json",
-          params: { bookmarked: "true" },
-          headers: { HTTP_API_KEY: master_key.key, HTTP_API_USERNAME: user.username }
-
-        expect(response.status).to eq(200)
-        expect(PostAction.where(
-          post: post,
-          user: user,
-          post_action_type_id: PostActionType.types[:bookmark]
-        ).count).to eq(1)
-      end
-
-      it 'disallows phonies to bookmark posts' do
-        put "/posts/#{post.id}/bookmark.json",
-          params: { bookmarked: "true" },
-          headers: { HTTP_API_KEY: SecureRandom.hex(32), HTTP_API_USERNAME: user.username }
-
-        expect(response.status).to eq(403)
-      end
-
-      it 'disallows blank api' do
-        put "/posts/#{post.id}/bookmark.json",
-          params: { bookmarked: "true" },
-          headers: { HTTP_API_KEY: "", HTTP_API_USERNAME: user.username }
-
-        expect(response.status).to eq(403)
-      end
-    end
-  end
-
   describe '#wiki' do
     include_examples "action requires login", :put, "/posts/2/wiki.json"
 

spec/requests/topics_controller_spec.rb

@@ -2346,19 +2346,15 @@ RSpec.describe TopicsController do
 
   describe '#remove_bookmarks' do
     it "should remove bookmarks properly from non first post" do
-      bookmark = PostActionType.types[:bookmark]
       sign_in(user)
 
       post = create_post
       post2 = create_post(topic_id: post.topic_id)
-
+      Fabricate(:bookmark, user: user, post: post)
-      PostActionCreator.new(user, post2, bookmark).perform
+      Fabricate(:bookmark, user: user, post: post2)
-
-      put "/t/#{post.topic_id}/bookmark.json"
-      expect(PostAction.where(user_id: user.id, post_action_type: bookmark).count).to eq(2)
 
       put "/t/#{post.topic_id}/remove_bookmarks.json"
-      expect(PostAction.where(user_id: user.id, post_action_type: bookmark).count).to eq(0)
+      expect(Bookmark.where(user: user).count).to eq(0)
     end
 
     it "should disallow bookmarks on posts you have no access to" do
@@ -2369,10 +2365,7 @@ RSpec.describe TopicsController do
       expect(response).to be_forbidden
     end
 
-    context "when SiteSetting.enable_bookmarks_with_reminders is true" do
+    context "bookmarks with reminders" 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
@@ -2388,26 +2381,23 @@ RSpec.describe TopicsController do
       sign_in(user)
     end
 
-    it "should create a new post action for the bookmark on the first post of the topic" do
+    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"
 
-      expect(PostAction.find_by(user_id: user.id, post_action_type: PostActionType.types[:bookmark]).post_id).to eq(post.id)
+      expect(Bookmark.find_by(user_id: user.id).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
+      Bookmark.create(post: post, user: user, topic: post.topic)
 
       put "/t/#{post.topic_id}/bookmark.json"
-      expect(response).to be_forbidden
+      expect(response.status).to eq(400)
     end
 
-    context "when SiteSetting.enable_bookmarks_with_reminders is true" do
+    context "bookmarks with reminders" 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)

spec/serializers/post_serializer_spec.rb

@@ -254,11 +254,7 @@ describe PostSerializer do
     context "when a Bookmark record exists for the user on the post" do
       let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post) }
 
-      context "when the site setting for bookmarks with reminders is enabled" do
+      context "bookmarks with reminders" do
-        before do
-          SiteSetting.enable_bookmarks_with_reminders = true
-        end
-
         it "returns true" do
           expect(serialized.as_json[:bookmarked_with_reminder]).to eq(true)
         end
@@ -275,16 +271,6 @@ describe PostSerializer do
           end
         end
       end
-
-      context "when the site setting for bookmarks with reminders is disabled" do
-        it "does not return the bookmarked_with_reminder attribute" do
-          expect(serialized.as_json.key?(:bookmarked_with_reminder)).to eq(false)
-        end
-
-        it "does not return the bookmark_reminder_at attribute" do
-          expect(serialized.as_json.key?(:bookmark_reminder_at)).to eq(false)
-        end
-      end
     end
   end
 

spec/tasks/bookmarks_spec.rb

@@ -38,11 +38,6 @@ RSpec.describe "bookmarks tasks" do
     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
-    invoke_task(1)
-    expect(Bookmark.all.count).to eq(1)
-  end
-
   it "skips post actions where the post topic no longer exists and does not error" do
     post1.topic.delete
     post1.reload

test/javascripts/acceptance/topic-test.js

@@ -397,10 +397,7 @@ QUnit.test(
 );
 
 acceptance("Topic + Post Bookmarks with Reminders", {
-  loggedIn: true,
+  loggedIn: true
-  settings: {
-    enable_bookmarks_with_reminders: true
-  }
 });
 
 QUnit.test("Bookmarks Modal", async assert => {

test/javascripts/controllers/preferences-account-test.js

@@ -12,7 +12,7 @@ QUnit.test("updating of associated accounts", function(assert) {
       is_anonymous: true
     }),
     currentUser: EmberObject.create({
-      id: 1234,
+      id: 1234
     }),
     site: EmberObject.create({
       isMobileDevice: false

test/javascripts/fixtures/user_fixtures.js

@@ -397,6 +397,34 @@ export default {
       featured_user_badge_ids: [5870, 40673, 5868]
     }
   },
+  "/u/eviltrout/bookmarks.json": {
+    user_bookmark_list: {
+      bookmarks: [
+        {
+          excerpt: "Here this is my new topic where I yell.",
+          tags: [],
+          id: 576,
+          created_at: "2020-04-07T05:30:40.446Z",
+          topic_id: 119,
+          linked_post_number: 1,
+          post_id: 281,
+          name: "test",
+          reminder_at: null,
+          title: "Yelling topic title :/",
+          deleted: false,
+          hidden: false,
+          category_id: 1,
+          closed: false,
+          archived: false,
+          archetype: "regular",
+          highest_post_number: 5,
+          bumped_at: "2020-04-06T05:20:00.172Z",
+          slug: "yelling-topic-title",
+          username: "someguy"
+        }
+      ]
+    }
+  },
   "/user_actions.json": {
     user_actions: [
       {

test/javascripts/helpers/site-settings.js

@@ -31,7 +31,6 @@ 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,

test/javascripts/widgets/post-test.js

@@ -532,19 +532,19 @@ widgetTest("can't bookmark", {
 
 widgetTest("bookmark", {
   template:
-    '{{mount-widget widget="post" args=args toggleBookmark=(action "toggleBookmark")}}',
+    '{{mount-widget widget="post" args=args toggleBookmarkWithReminder=(action "toggleBookmarkWithReminder")}}',
   beforeEach() {
     const args = { canBookmark: true };
 
     this.set("args", args);
-    this.on("toggleBookmark", () => (args.bookmarked = true));
+    this.on(
+      "toggleBookmarkWithReminder",
+      () => (args.bookmarked_with_reminder = true)
+    );
   },
   async test(assert) {
     assert.equal(find(".post-menu-area .bookmark").length, 1);
     assert.equal(find("button.bookmarked").length, 0);
-
-    await click("button.bookmark");
-    assert.equal(find("button.bookmarked").length, 1);
   }
 });
 

test/javascripts/widgets/user-menu-test.js

@@ -145,11 +145,9 @@ widgetTest("bookmarks", {
     const bookmark = find(".quick-access-panel li a")[0];
     assert.ok(bookmark);
 
+    assert.ok(bookmark.href.includes("/t/yelling-topic-title/119"));
     assert.ok(
-      bookmark.href.includes("/t/how-to-check-the-user-level-via-ajax/11993")
+      bookmark.innerHTML.includes("someguy"),
-    );
-    assert.ok(
-      bookmark.innerHTML.includes("Abhishek_Gupta"),
       "should include the last poster's username"
     );
     assert.ok(