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

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.

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 0b1a266..6cf2549 100644
--- a/app/controllers/users_controller.rb
+++ b/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
diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb
index cf8db86..bd63025 100644
--- a/app/jobs/regular/sync_topic_user_bookmarked.rb
+++ b/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" : ""}
diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 6bc1a01..74a0d27 100644
--- a/app/models/bookmark.rb
+++ b/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
diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index 7762f14..3c280ec 100644
--- a/app/models/post_mover.rb
+++ b/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)
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 6b89da4..95cb1b9 100644
--- a/app/models/topic.rb
+++ b/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
diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb
index 39a494c..e724931 100644
--- a/app/serializers/user_bookmark_serializer.rb
+++ b/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
diff --git a/app/views/users/bookmarks.ics.erb b/app/views/users/bookmarks.ics.erb
index cfb5070..a0c93c1 100644
--- a/app/views/users/bookmarks.ics.erb
+++ b/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") %>
-DTSTART:<%= bookmark.reminder_at.strftime("%Y%m%dT%H%M%SZ") %>
-DTEND:<%= (bookmark.reminder_at + 1.hour).strftime("%Y%m%dT%H%M%SZ") %>
+DTSTAMP:<%= bookmark.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics")) %>

[... diff too long, it was truncated ...]

GitHub sha: 22208836c5f2adcf8c85a62d27b67968743b20f0

This commit appears in #14289 which was approved by tgxworld. It was merged by martin.