DEV: Rolling back bookmarkable column changes (#15482)

It is too close to release of 2.8 for incomplete
feature shenanigans. Ignores and drops the columns and drops
the trigger/function introduced in
e21c640a3c.
Will pick this feature back up post-release.

app/models/bookmark.rb

@@ -1,6 +1,13 @@
 # frozen_string_literal: true
 
 class Bookmark < ActiveRecord::Base
+  # these columns were here for a very short amount of time,
+  # hence the very short ignore time
+  self.ignored_columns = [
+    "bookmarkable_id", # TODO (martin) 2022-01-12 remove
+    "bookmarkable_type" # TODO (martin) 2022-01-12 remove
+  ]
+
   belongs_to :user
   belongs_to :post
   has_one :topic, through: :post
@@ -172,12 +179,9 @@ end
 #  auto_delete_preference :integer          default(0), not null
 #  pinned                 :boolean          default(FALSE)
 #  for_topic              :boolean          default(FALSE), not null
-#  bookmarkable_id        :integer
-#  bookmarkable_type      :string
 #
 # Indexes
 #
-#  idx_bookmarks_user_polymorphic_unique                 (user_id,bookmarkable_type,bookmarkable_id) UNIQUE
 #  index_bookmarks_on_post_id                            (post_id)
 #  index_bookmarks_on_reminder_at                        (reminder_at)
 #  index_bookmarks_on_reminder_set_at                    (reminder_set_at)

db/migrate/20220105024605_add_trigger_for_polymorphic_bookmark_columns_to_sync_data.rb

@@ -28,21 +28,29 @@ class AddTriggerForPolymorphicBookmarkColumnsToSyncData < ActiveRecord::Migratio
     SQL
 
     # sync data that already exists in the table
-    DB.exec(<<~SQL)
-      UPDATE bookmarks
-      SET bookmarkable_id = post_id, bookmarkable_type = 'Post'
-      WHERE NOT bookmarks.for_topic
-    SQL
-    DB.exec(<<~SQL)
-      UPDATE bookmarks
-      SET bookmarkable_id = posts.topic_id, bookmarkable_type = 'Topic'
-      FROM posts
-      WHERE bookmarks.for_topic AND posts.id = bookmarks.post_id
-    SQL
+    #
+    # Note from Martin:
+    #
+    # We cannot remove this migration but we also don't want to do this
+    # backfilling until this refactor is complete, removing the backfilling
+    # for now as DropBookmarkPolymorphicTrigger removes the trigger used
+    # here.
+    #
+    # DB.exec(<<~SQL)
+    #   UPDATE bookmarks
+    #   SET bookmarkable_id = post_id, bookmarkable_type = 'Post'
+    #   WHERE NOT bookmarks.for_topic
+    # SQL
+    # DB.exec(<<~SQL)
+    #   UPDATE bookmarks
+    #   SET bookmarkable_id = posts.topic_id, bookmarkable_type = 'Topic'
+    #   FROM posts
+    #   WHERE bookmarks.for_topic AND posts.id = bookmarks.post_id
+    # SQL
   end
 
   def down
-    DB.exec("DROP TRIGGER IF EXISTS bookmarks_polymorphic_data_sync")
+    DB.exec("DROP TRIGGER IF EXISTS bookmarks_polymorphic_data_sync ON bookmarks")
     DB.exec("DROP FUNCTION IF EXISTS sync_bookmarks_polymorphic_column_data")
   end
 end

db/migrate/20220107011124_drop_bookmark_polymorphic_trigger.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class DropBookmarkPolymorphicTrigger < ActiveRecord::Migration[6.1]
+  def up
+    DB.exec("DROP TRIGGER IF EXISTS bookmarks_polymorphic_data_sync ON bookmarks")
+    DB.exec("DROP FUNCTION IF EXISTS sync_bookmarks_polymorphic_column_data")
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/post_migrate/20220107014925_drop_bookmark_polymorphic_columns.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class DropBookmarkPolymorphicColumns < ActiveRecord::Migration[6.1]
+  DROPPED_COLUMNS ||= {
+    bookmarks: %i{bookmarkable_id bookmarkable_type}
+  }
+
+  def up
+    DROPPED_COLUMNS.each do |table, columns|
+      Migration::ColumnDropper.execute_drop(table, columns)
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/lib/bookmark_manager_spec.rb

@@ -12,14 +12,12 @@ RSpec.describe BookmarkManager do
   subject { described_class.new(user) }
 
   describe ".create" do
-    it "creates the bookmark for the user with the correct polymorphic type" do
+    it "creates the bookmark for the user" do
       subject.create(post_id: post.id, name: name)
       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.bookmarkable_id).to eq(post.id)
-      expect(bookmark.bookmarkable_type).to eq("Post")
     end
 
     it "allows creating a bookmark for the topic and for the first post" do
@@ -29,8 +27,6 @@ RSpec.describe BookmarkManager do
       expect(bookmark.post_id).to eq(post.id)
       expect(bookmark.topic_id).to eq(post.topic_id)
       expect(bookmark.for_topic).to eq(true)
-      expect(bookmark.bookmarkable_id).to eq(post.topic_id)
-      expect(bookmark.bookmarkable_type).to eq("Topic")
 
       subject.create(post_id: post.id, name: name)
       bookmark = Bookmark.find_by(user: user, post_id: post.id, for_topic: false)
@@ -38,8 +34,6 @@ RSpec.describe BookmarkManager do
       expect(bookmark.post_id).to eq(post.id)
       expect(bookmark.topic_id).to eq(post.topic_id)
       expect(bookmark.for_topic).to eq(false)
-      expect(bookmark.bookmarkable_id).to eq(post.id)
-      expect(bookmark.bookmarkable_type).to eq("Post")
     end
 
     it "errors when creating a for_topic bookmark for a post that is not the first one" do