DEV: Ignore bookmarks.topic_id column and remove references to it in code (#14289)

We don't need no stinkin' denormalization! This commit ignores
the topic_id column on bookmarks, to be deleted at a later date.
We don't really need this column and it's better to rely on the
post.topic_id as the canonical topic_id for bookmarks, then we
don't need to remember to update both columns if the bookmarked
post moves to another topic.

app/controllers/users_controller.rb

@@ -1598,7 +1598,10 @@ class UsersController < ApplicationController
         end
       end
       format.ics do
-        @bookmark_reminders = Bookmark.where(user_id: user.id).where.not(reminder_at: nil).joins(:topic)
+        @bookmark_reminders = Bookmark.with_reminders
+          .where(user_id: user.id)
+          .includes(:topic)
+          .order(:reminder_at)
       end
     end
   end

app/jobs/regular/sync_topic_user_bookmarked.rb

@@ -11,7 +11,7 @@ module Jobs
         INNER JOIN posts ON posts.id = b.post_id
         WHERE NOT topic_users.bookmarked AND
           posts.deleted_at IS NULL AND
-          topic_users.topic_id = b.topic_id AND
+          topic_users.topic_id = posts.topic_id AND
           topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
       SQL
 
@@ -22,7 +22,7 @@ module Jobs
             SELECT COUNT(*)
             FROM bookmarks
             INNER JOIN posts ON posts.id = bookmarks.post_id
-            WHERE bookmarks.topic_id = topic_users.topic_id
+            WHERE posts.topic_id = topic_users.topic_id
             AND bookmarks.user_id = topic_users.user_id
             AND posts.deleted_at IS NULL
         ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}

app/models/bookmark.rb

@@ -1,9 +1,15 @@
 # frozen_string_literal: true
 
 class Bookmark < ActiveRecord::Base
+  self.ignored_columns = [
+    "topic_id" # TODO (martin) (2021-12-01): remove
+  ]
+
   belongs_to :user
   belongs_to :post
-  belongs_to :topic
+  has_one :topic, through: :post
+
+  delegate :topic_id, to: :post
 
   def self.reminder_types
     @reminder_types ||= Enum.new(
@@ -39,16 +45,6 @@ class Bookmark < ActiveRecord::Base
   validate :bookmark_limit_not_reached
   validates :name, length: { maximum: 100 }
 
-  # we don't care whether the post or topic is deleted,
-  # they hold important information about the bookmark
-  def post
-    Post.unscoped { super }
-  end
-
-  def topic
-    Topic.unscoped { super }
-  end
-
   def unique_per_post_for_user
     if Bookmark.exists?(user_id: user_id, post_id: post_id)
       self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
@@ -91,6 +87,10 @@ class Bookmark < ActiveRecord::Base
     self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply]
   end
 
+  def reminder_at_ics(offset: 0)
+    (reminder_at + offset).strftime(I18n.t("datetime_formats.formats.calendar_ics"))
+  end
+
   def clear_reminder!
     update!(
       reminder_at: nil,
@@ -100,19 +100,34 @@ class Bookmark < ActiveRecord::Base
     )
   end
 
+  scope :with_reminders, -> do
+    where("reminder_at IS NOT NULL")
+  end
+
   scope :pending_reminders, ->(before_time = Time.now.utc) do
-    where("reminder_at IS NOT NULL AND reminder_at <= :before_time", before_time: before_time)
+    with_reminders.where("reminder_at <= :before_time", before_time: before_time)
   end
 
   scope :pending_reminders_for_user, ->(user) do
     pending_reminders.where(user: user)
   end
 
+  scope :for_user_in_topic, ->(user_id, topic_id) {
+    joins(:post).where(user_id: user_id, posts: { topic_id: topic_id })
+  }
+
   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]
+    if opts[:end_date]
+      result = result.where('bookmarks.created_at <= ?', opts[:end_date])
+    end
+
+    if opts[:category_id]
+      result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id]))
+    end
+
     result.group('date(bookmarks.created_at)')
       .order('date(bookmarks.created_at)')
       .count
@@ -127,9 +142,9 @@ class Bookmark < ActiveRecord::Base
     topics_deleted = DB.query(<<~SQL, grace_time: grace_time)
       DELETE FROM bookmarks b
       USING topics t, posts p
-      WHERE (b.topic_id = t.id AND b.post_id = p.id)
+      WHERE (t.id = p.topic_id AND b.post_id = p.id)
         AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time)
-       RETURNING b.topic_id
+       RETURNING t.id AS topic_id
     SQL
 
     topics_deleted_ids = topics_deleted.map(&:topic_id).uniq
@@ -145,7 +160,6 @@ end
 #
 #  id                     :bigint           not null, primary key
 #  user_id                :bigint           not null
-#  topic_id               :bigint           not null
 #  post_id                :bigint           not null
 #  name                   :string(100)
 #  reminder_type          :integer

app/models/post_mover.rb

@@ -176,6 +176,10 @@ class PostMover
 
     DiscourseEvent.trigger(:post_moved, new_post, original_topic.id)
 
+    # we don't want to keep the old topic's OP bookmarked when we are
+    # moving it into a new topic
+    Bookmark.where(post_id: post.id).update_all(post_id: new_post.id)
+
     new_post
   end
 
@@ -479,8 +483,6 @@ class PostMover
   end
 
   def update_bookmarks
-    Bookmark.where(post_id: post_ids).update_all(topic_id: @destination_topic.id)
-
     DB.after_commit do
       Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @original_topic.id)
       Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @destination_topic.id)

app/models/topic.rb

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

app/serializers/user_bookmark_serializer.rb

@@ -30,12 +30,16 @@ class UserBookmarkSerializer < ApplicationSerializer
              :post_user_avatar_template,
              :post_user_name
 
+  def topic_id
+    post.topic_id
+  end
+
   def topic
-    @topic ||= object.topic || Topic.unscoped.find(object.topic_id)
+    @topic ||= object.topic
   end
 
   def post
-    @post ||= object.post || Post.unscoped.find(object.post_id)
+    @post ||= object.post
   end
 
   def closed

app/views/users/bookmarks.ics.erb

@@ -4,9 +4,9 @@ PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_versi
 <% @bookmark_reminders.each do |bookmark| %>
 BEGIN:VEVENT
 UID:bookmark_reminder_#<%= bookmark.id %>@<%= Discourse.current_hostname %>
-DTSTAMP:<%= bookmark.updated_at.strftime("%Y%m%dT%H%M%SZ") %>
+DTSTAMP:<%= bookmark.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics")) %>
-DTSTART:<%= bookmark.reminder_at.strftime("%Y%m%dT%H%M%SZ") %>
+DTSTART:<%= bookmark.reminder_at_ics %>
-DTEND:<%= (bookmark.reminder_at + 1.hour).strftime("%Y%m%dT%H%M%SZ") %>
+DTEND:<%= bookmark.reminder_at_ics(offset: 1.hour) %>
 SUMMARY:<%= bookmark.name.presence || bookmark.topic.title %>
 DESCRIPTION:<%= Discourse.base_url %>/t/-/<%= bookmark.topic_id %>
 URL:<%= Discourse.base_url %>/t/-/<%= bookmark.topic_id %>

config/locales/server.en.yml

@@ -19,6 +19,8 @@ en:
       long: "%B %-d, %Y, %l:%M%P"
       # Format directives: https://ruby-doc.org/core/Time.html#method-i-strftime
       no_day: "%B %Y"
+      # Format directives: https://ruby-doc.org/core/Time.html#method-i-strftime
+      calendar_ics: "%Y%m%dT%H%M%SZ"
   date:
     # Do not remove the brackets and commas and do not translate the first month name. It should be "null".
     month_names:

db/post_migrate/20210909041448_make_topic_id_nullable_for_bookmarks.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class MakeTopicIdNullableForBookmarks < ActiveRecord::Migration[6.1]
+  def up
+    change_column_null :bookmarks, :topic_id, true
+    Migration::ColumnDropper.mark_readonly(:bookmarks, :topic_id)
+  end
+
+  def down
+    Migration::ColumnDropper.drop_readonly(:bookmarks, :topic_id)
+    change_column_null :bookmarks, :topic_id, false
+  end
+end

lib/bookmark_manager.rb

@@ -21,7 +21,6 @@ class BookmarkManager
     bookmark = Bookmark.create(
       {
         user_id: @user.id,
-        topic: post.topic,
         post: post,
         name: name,
         reminder_type: reminder_type,
@@ -50,7 +49,7 @@ class BookmarkManager
   end
 
   def destroy_for_topic(topic, filter = {}, opts = {})
-    topic_bookmarks = Bookmark.where(user_id: @user.id, topic_id: topic.id)
+    topic_bookmarks = Bookmark.for_user_in_topic(@user.id, topic.id)
     topic_bookmarks = topic_bookmarks.where(filter)
 
     Bookmark.transaction do
@@ -113,7 +112,7 @@ class BookmarkManager
   def update_topic_user_bookmarked(topic, opts = {})
     # PostCreator can specify whether auto_track is enabled or not, don't want to
     # create a TopicUser in that case
-    bookmarks_remaining_in_topic = Bookmark.exists?(topic_id: topic.id, user: @user)
+    bookmarks_remaining_in_topic = Bookmark.for_user_in_topic(@user.id, topic.id).exists?
     return bookmarks_remaining_in_topic if opts.key?(:auto_track) && !opts[:auto_track]
 
     TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic)

lib/bookmark_query.rb

@@ -69,9 +69,8 @@ class BookmarkQuery
 
   def user_bookmarks
     Bookmark.where(user: @user)
-      .includes(topic: :tags)
       .includes(post: :user)
-      .references(:topic)
+      .includes(post: { topic: :tags })
       .references(:post)
   end
 end

lib/bookmark_reminder_notification_handler.rb

@@ -4,9 +4,12 @@ class BookmarkReminderNotificationHandler
   def self.send_notification(bookmark)
     return if bookmark.blank?
     Bookmark.transaction do
-      if bookmark.post.blank? || bookmark.post.deleted_at.present?
+      # we don't send reminders for deleted posts or topics,
+      # just as we don't allow creation of bookmarks for deleted
+      # posts or topics
+      if bookmark.post.blank? || bookmark.topic.blank?
         clear_reminder(bookmark)
-      elsif bookmark.topic
+      else
         create_notification(bookmark)
 
         if bookmark.auto_delete_when_reminder_sent?

lib/tasks/bookmarks.rake

@@ -31,7 +31,7 @@ task "bookmarks:sync_to_table" => :environment do |_t, args|
 
     post_action_bookmarks.each do |pab|
       now = Time.zone.now
-      bookmarks_to_create << "(#{pab.topic_id}, #{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')"
+      bookmarks_to_create << "(#{pab.post_id}, #{pab.user_id}, '#{now}', '#{now}')"
     end
 
     create_bookmarks(bookmarks_to_create)
@@ -54,7 +54,7 @@ def create_bookmarks(bookmarks_to_create)
   #
   DB.exec(
     <<~SQL
-      INSERT INTO bookmarks (topic_id, post_id, user_id, created_at, updated_at)
+      INSERT INTO bookmarks (post_id, user_id, created_at, updated_at)
       VALUES #{bookmarks_to_create.join(",\n")}
       ON CONFLICT DO NOTHING
     SQL

lib/topic_view.rb

@@ -392,6 +392,7 @@ class TopicView
 
   def has_bookmarks?
     return false if @user.blank?
+    return false if @topic.trashed?
     @topic.bookmarks.exists?(user_id: @user.id)
   end
 

spec/components/post_creator_spec.rb

@@ -786,13 +786,13 @@ describe PostCreator do
 
     context "when the user has bookmarks with auto_delete_preference on_owner_reply" do
       before do
-        Fabricate(:bookmark, topic: topic, user: user, auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
+        Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
-        Fabricate(:bookmark, topic: topic, user: user, auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
+        Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
         TopicUser.create!(topic: topic, user: user, bookmarked: true)
       end
 
       it "deletes the bookmarks, but not the ones without an auto_delete_preference" do
-        Fabricate(:bookmark, topic: topic, user: user)
+        Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user)
         Fabricate(:bookmark, user: user)
         creator.create
         expect(Bookmark.where(user: user).count).to eq(2)

spec/components/topic_view_spec.rb

@@ -424,16 +424,25 @@ describe TopicView do
       end
 
       context "when the topic is deleted" do
-        it "gets the first post bookmark reminder at for the user" do
+        it "returns nil" do
           topic_view = TopicView.new(topic, user)
           PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
           topic.reload
 
-          first, second = topic_view.bookmarked_posts
+          expect(topic_view.bookmarked_posts).to eq(nil)
+        end
+      end
+
+      context "when one of the posts is deleted" do
+        it "does not return that post's bookmark" do
+          topic_view = TopicView.new(topic, user)
+          PostDestroyer.new(Fabricate(:admin), topic.posts.second).destroy
+          topic.reload
+
+          expect(topic_view.bookmarked_posts.length).to eq(1)
+          first = topic_view.bookmarked_posts.first
           expect(first[:post_id]).to eq(bookmark1.post_id)
           expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at)
-          expect(second[:post_id]).to eq(bookmark2.post_id)
-          expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at)
         end
       end
     end

spec/fabricators/bookmark_fabricator.rb

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

spec/jobs/sync_topic_user_bookmarked_spec.rb

@@ -15,8 +15,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
     tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true)
     tu5 = Fabricate(:topic_user, bookmarked: false)
 
-    Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample)
+    Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample)
-    Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample)
+    Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample)
 
     subject.execute(topic_id: topic.id)
 
@@ -34,8 +34,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
     tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
     tu2 = Fabricate(:topic_user, topic: topic, bookmarked: true)
 
-    Fabricate(:bookmark, user: tu1.user, topic: topic, post: post1)
+    Fabricate(:bookmark, user: tu1.user, post: post1)
-    Fabricate(:bookmark, user: tu2.user, topic: topic, post: post1)
+    Fabricate(:bookmark, user: tu2.user, post: post1)
 
     post1.trash!
 
@@ -57,8 +57,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
     tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true)
     tu5 = Fabricate(:topic_user, bookmarked: false)
 
-    Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample)
+    Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample)
-    Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample)
+    Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample)
 
     subject.execute
 

spec/lib/bookmark_manager_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe BookmarkManager do
       bookmark = Bookmark.find_by(user: user)
 
       expect(bookmark.post_id).to eq(post.id)
-      expect(bookmark.topic_id).to eq(post.topic_id)
+      expect(bookmark.topic.id).to eq(post.topic_id)
     end
 
     it "when topic is deleted it raises invalid access from guardian check" do
@@ -65,7 +65,7 @@ RSpec.describe BookmarkManager do
 
     context "when the bookmark already exists for the user & post" do
       before do
-        Bookmark.create(post: post, user: user, topic: post.topic)
+        Bookmark.create(post: post, user: user)
       end
 
       it "adds an error to the manager" do
@@ -228,19 +228,19 @@ RSpec.describe BookmarkManager do
 
   describe ".destroy_for_topic" do
     let!(:topic) { Fabricate(:topic) }
-    let!(:bookmark1) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }
+    let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) }
-    let!(:bookmark2) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }
+    let!(:bookmark2) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) }
 
     it "destroys all bookmarks for the topic for the specified user" do
       subject.destroy_for_topic(topic)
-      expect(Bookmark.where(user: user, topic: topic).length).to eq(0)
+      expect(Bookmark.for_user_in_topic(user.id, topic.id).length).to eq(0)
     end
 
     it "does not destroy any other user's topic bookmarks" do
       user2 = Fabricate(:user)
-      Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user2)
+      Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user2)
       subject.destroy_for_topic(topic)
-      expect(Bookmark.where(user: user2, topic: topic).length).to eq(1)
+      expect(Bookmark.for_user_in_topic(user2.id, topic.id).length).to eq(1)
     end
 
     it "updates the topic user bookmarked column to false" do

spec/lib/bookmark_query_spec.rb

@@ -45,7 +45,7 @@ RSpec.describe BookmarkQuery do
       before do
         @post = Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs"))
         @bookmark3 = Fabricate(:bookmark, user: user, name: "Check up later")
-        @bookmark4 = Fabricate(:bookmark, user: user, post: @post, topic: @post.topic)
+        @bookmark4 = Fabricate(:bookmark, user: user, post: @post)
       end
 
       it "can search by bookmark name" do
@@ -90,7 +90,7 @@ RSpec.describe BookmarkQuery do
     context "for a private message topic bookmark" do
       let(:pm_topic) { Fabricate(:private_message_topic) }
       before do
-        bookmark1.update(topic: pm_topic, post: Fabricate(:post, topic: pm_topic))
+        bookmark1.update(post: Fabricate(:post, topic: pm_topic))
         TopicUser.change(user.id, pm_topic.id, total_msecs_viewed: 1)
       end
 
@@ -160,7 +160,7 @@ RSpec.describe BookmarkQuery do
         Topic.expects(:preload_custom_fields)
         expect(
           bookmark_query.list_all.find do |b|
-            b.topic_id = bookmark1.topic_id
+            b.topic.id = bookmark1.topic.id
           end.topic.custom_fields['test_field']
         ).not_to eq(nil)
       end

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -30,10 +30,33 @@ RSpec.describe BookmarkReminderNotificationHandler do
 
     it "clears the reminder" do
       subject.send_notification(bookmark)
-      bookmark.reload
       expect(bookmark.reload.no_reminder?).to eq(true)
     end
 
+    context "when the topic is deleted" do
+      before do
+        bookmark.topic.trash!
+        bookmark.reload
+      end
+
+      it "does not send a notification and clears the reminder" do
+        expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
+        expect(bookmark.reload.no_reminder?).to eq(true)
+      end
+    end
+
+    context "when the post is deleted" do
+      before do
+        bookmark.post.trash!
+        bookmark.reload
+      end
+
+      it "does not send a notification and clears the reminder" do
+        expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
+        expect(bookmark.reload.no_reminder?).to eq(true)
+      end
+    end
+
     context "when the auto_delete_preference is when_reminder_sent" do
       before do
         TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true)
@@ -52,7 +75,7 @@ RSpec.describe BookmarkReminderNotificationHandler do
 
       context "if there are still other bookmarks in the topic" do
         before do
-          Fabricate(:bookmark, topic: bookmark.topic, post: Fabricate(:post, topic: bookmark.topic), user: user)
+          Fabricate(:bookmark, post: Fabricate(:post, topic: bookmark.topic), user: user)
         end
 
         it "does not change the TopicUser bookmarked column to false" do

spec/models/bookmark_spec.rb

@@ -7,12 +7,11 @@ describe Bookmark do
 
   context 'validations' do
     it 'does not allow user to bookmark a post twice' do
-      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      bookmark = Fabricate(:bookmark, post: post)
       user = bookmark.user
 
       bookmark_2 = Fabricate.build(:bookmark,
         post: post,
-        topic: post.topic,
         user: user
       )
 
@@ -22,7 +21,7 @@ describe Bookmark do
 
   describe "#cleanup!" do
     it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do
-      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      bookmark = Fabricate(:bookmark, post: post)
       bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
       post.trash!
       post.update(deleted_at: 4.days.ago)
@@ -33,10 +32,10 @@ describe Bookmark do
 
     it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do
       post2 = Fabricate(:post)
-      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      bookmark = Fabricate(:bookmark, post: post)
       bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
-      bookmark3 = Fabricate(:bookmark, post: post2, topic: post2.topic)
+      bookmark3 = Fabricate(:bookmark, post: post2)
-      bookmark4 = Fabricate(:bookmark, post: post2, topic: post2.topic)
+      bookmark4 = Fabricate(:bookmark, post: post2)
       post.trash!
       post.update(deleted_at: 4.days.ago)
       post2.trash!
@@ -51,8 +50,8 @@ describe Bookmark do
     end
 
     it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do
-      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      bookmark = Fabricate(:bookmark, post: post)
-      bookmark2 = Fabricate(:bookmark, topic: post.topic, post: Fabricate(:post, topic: post.topic))
+      bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
       bookmark3 = Fabricate(:bookmark)
       post.topic.trash!
       post.topic.update(deleted_at: 4.days.ago)
@@ -63,7 +62,7 @@ describe Bookmark do
     end
 
     it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do
-      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      bookmark = Fabricate(:bookmark, post: post)
       bookmark2 = Fabricate(:bookmark)
       Bookmark.cleanup!
       expect(Bookmark.find(bookmark.id)).to eq(bookmark)
@@ -73,6 +72,57 @@ describe Bookmark do
       expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2)
     end
 
+    describe "#count_per_day" do
+      let(:category) { Fabricate(:category) }
+      let(:topic_in_category) { Fabricate(:topic, category: category) }
+      let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) }
+      let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) }
+      let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) }
+      let!(:bookmark4) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) }
+      let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) }
+
+      it "gets the count of bookmarks grouped by date within the last 30 days by default" do
+        expect(Bookmark.count_per_day).to eq({
+          1.day.ago.to_date => 1,
+          2.days.ago.to_date => 1,
+          3.days.ago.to_date => 2
+        })
+      end
+
+      it "respects the start_date option" do
+        expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({
+          1.day.ago.to_date => 1,
+        })
+      end
+
+      it "respects the since_days_ago option" do
+        expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({
+          1.day.ago.to_date => 1,
+        })
+      end
+
+      it "respects the end_date option" do
+        expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({
+          2.days.ago.to_date => 1,
+          3.days.ago.to_date => 2,
+        })
+      end
+
+      it "respects the category_id option" do
+        expect(Bookmark.count_per_day(category_id: category.id)).to eq({
+          3.days.ago.to_date => 1,
+        })
+      end
+
+      it "does not include deleted posts or topics" do
+        bookmark4.post.trash!
+        expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+        bookmark4.post.recover!
+        bookmark4.topic.trash!
+        expect(Bookmark.count_per_day(category_id: category.id)).to eq({})
+      end
+    end
+
     describe "bookmark limits" do
       fab!(:user) { Fabricate(:user) }
 

spec/models/post_mover_spec.rb

@@ -396,10 +396,10 @@ describe PostMover do
           it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do
             some_user = Fabricate(:user)
             new_post = Fabricate(:post, topic: p1.topic)
-            bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: some_user)
+            bookmark1 = Fabricate(:bookmark, post: p1, user: some_user)
-            bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4)
+            bookmark2 = Fabricate(:bookmark, post: p4)
-            bookmark3 = Fabricate(:bookmark, topic: p4.topic, post: p4)
+            bookmark3 = Fabricate(:bookmark, post: p4)
-            bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post)
+            bookmark4 = Fabricate(:bookmark, post: new_post)
             new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name")
             expect(bookmark1.reload.topic_id).to eq(new_topic.id)
             expect(bookmark2.reload.topic_id).to eq(new_topic.id)
@@ -412,11 +412,11 @@ describe PostMover do
             user1 = Fabricate(:user)
             user2 = Fabricate(:user)
 
-            bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: user1)
+            bookmark1 = Fabricate(:bookmark, post: p1, user: user1)
-            bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user1)
+            bookmark2 = Fabricate(:bookmark, post: p4, user: user1)
 
-            bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: user2)
+            bookmark3 = Fabricate(:bookmark, post: p3, user: user2)
-            bookmark4 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user2)
+            bookmark4 = Fabricate(:bookmark, post: p4, user: user2)
 
             tu1 = Fabricate(
               :topic_user,
@@ -441,6 +441,10 @@ describe PostMover do
             new_topic_user1 = TopicUser.find_by(topic: new_topic, user: user1)
             new_topic_user2 = TopicUser.find_by(topic: new_topic, user: user2)
 
+            original_topic_id = p1.topic_id
+            expect(p1.reload.topic_id).to eq(original_topic_id)
+            expect(p4.reload.topic_id).to eq(new_topic.id)
+
             expect(tu1.reload.bookmarked).to eq(false)
             expect(tu2.reload.bookmarked).to eq(true)
             expect(new_topic_user1.bookmarked).to eq(true)
@@ -724,10 +728,10 @@ describe PostMover do
           it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do
             some_user = Fabricate(:user)
             new_post = Fabricate(:post, topic: p3.topic)
-            bookmark1 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: some_user)
+            bookmark1 = Fabricate(:bookmark, post: p3, user: some_user)
-            bookmark2 = Fabricate(:bookmark, topic: p3.topic, post: p3)
+            bookmark2 = Fabricate(:bookmark, post: p3)
-            bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3)
+            bookmark3 = Fabricate(:bookmark, post: p3)
-            bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post)
+            bookmark4 = Fabricate(:bookmark, post: new_post)
             topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id)
             expect(bookmark1.reload.topic_id).to eq(destination_topic.id)
             expect(bookmark2.reload.topic_id).to eq(destination_topic.id)

spec/requests/posts_controller_spec.rb

@@ -564,7 +564,7 @@ describe PostsController do
 
   describe "#destroy_bookmark" do
     fab!(:post) { Fabricate(:post) }
-    fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post, topic: post.topic) }
+    fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post) }
 
     before do
       sign_in(user)
@@ -578,7 +578,7 @@ describe PostsController do
 
     context "when the user still has bookmarks in the topic" do
       before do
-        Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic), topic: post.topic)
+        Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic))
       end
       it "marks topic_bookmarked as true" do
         delete "/posts/#{post.id}/bookmark.json"

spec/requests/topics_controller_spec.rb

@@ -3108,9 +3108,9 @@ RSpec.describe TopicsController do
       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)
+        Fabricate(:bookmark, post: post, user: user)
         put "/t/#{post.topic_id}/remove_bookmarks.json"
-        expect(Bookmark.where(user: user, topic: topic).count).to eq(0)
+        expect(Bookmark.for_user_in_topic(user.id, post.topic_id).count).to eq(0)
       end
     end
   end
@@ -3130,7 +3130,7 @@ RSpec.describe TopicsController do
 
     it "errors if the topic is already bookmarked for the user" do
       post = create_post
-      Bookmark.create(post: post, user: user, topic: post.topic)
+      Bookmark.create(post: post, user: user)
 
       put "/t/#{post.topic_id}/bookmark.json"
       expect(response.status).to eq(400)
@@ -3143,14 +3143,14 @@ RSpec.describe TopicsController do
         put "/t/#{post.topic_id}/bookmark.json"
         expect(response.status).to eq(200)
 
-        bookmarks_for_topic = Bookmark.where(topic: post.topic, user: user)
+        bookmarks_for_topic = Bookmark.for_user_in_topic(user.id, post.topic_id)
         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)
+        Bookmark.create(post: post, user: user)
 
         put "/t/#{post.topic_id}/bookmark.json"
         expect(response.status).to eq(400)

spec/requests/users_controller_spec.rb

@@ -5061,6 +5061,39 @@ describe UsersController do
       expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array([bookmark1.id, bookmark2.id])
     end
 
+    it "returns an .ics file of bookmark reminders for the user in date order" do
+      bookmark1.update!(name: nil, reminder_at: 1.day.from_now)
+      bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now)
+
+      sign_in(user)
+      get "/u/#{user.username}/bookmarks.ics"
+      expect(response.status).to eq(200)
+      expect(response.body).to eq(<<~ICS)
+        BEGIN:VCALENDAR
+        VERSION:2.0
+        PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN
+        BEGIN:VEVENT
+        UID:bookmark_reminder_##{bookmark1.id}@#{Discourse.current_hostname}
+        DTSTAMP:#{bookmark1.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics"))}
+        DTSTART:#{bookmark1.reminder_at_ics}
+        DTEND:#{bookmark1.reminder_at_ics(offset: 1.hour)}
+        SUMMARY:#{bookmark1.topic.title}
+        DESCRIPTION:#{Discourse.base_url}/t/-/#{bookmark1.topic_id}
+        URL:#{Discourse.base_url}/t/-/#{bookmark1.topic_id}
+        END:VEVENT
+        BEGIN:VEVENT
+        UID:bookmark_reminder_##{bookmark2.id}@#{Discourse.current_hostname}
+        DTSTAMP:#{bookmark2.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics"))}
+        DTSTART:#{bookmark2.reminder_at_ics}
+        DTEND:#{bookmark2.reminder_at_ics(offset: 1.hour)}
+        SUMMARY:Some bookmark note
+        DESCRIPTION:#{Discourse.base_url}/t/-/#{bookmark2.topic_id}
+        URL:#{Discourse.base_url}/t/-/#{bookmark2.topic_id}
+        END:VEVENT
+        END:VCALENDAR
+      ICS
+    end
+
     it "does not show another user's bookmarks" do
       sign_in(user)
       get "/u/#{bookmark3.user.username}/bookmarks.json"

spec/serializers/user_bookmark_serializer_spec.rb

@@ -5,7 +5,7 @@ require 'rails_helper'
 RSpec.describe UserBookmarkSerializer do
   let(:user) { Fabricate(:user) }
   let(:post) { Fabricate(:post, user: user) }
-  let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post, topic: post.topic) }
+  let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post) }
   let(:bookmark_list) { BookmarkQuery.new(user: bookmark.user).list_all.to_ary }
 
   it "serializes all properties correctly" do