diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index cbf83181e0c..cd54b4a851c 100644 --- a/app/models/bookmark.rb +++ b/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) diff --git a/db/migrate/20220105024605_add_trigger_for_polymorphic_bookmark_columns_to_sync_data.rb b/db/migrate/20220105024605_add_trigger_for_polymorphic_bookmark_columns_to_sync_data.rb index f1ae13477ff..9b7389105ab 100644 --- a/db/migrate/20220105024605_add_trigger_for_polymorphic_bookmark_columns_to_sync_data.rb +++ b/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 diff --git a/db/migrate/20220107011124_drop_bookmark_polymorphic_trigger.rb b/db/migrate/20220107011124_drop_bookmark_polymorphic_trigger.rb new file mode 100644 index 00000000000..c0401d7d632 --- /dev/null +++ b/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 diff --git a/db/post_migrate/20220107014925_drop_bookmark_polymorphic_columns.rb b/db/post_migrate/20220107014925_drop_bookmark_polymorphic_columns.rb new file mode 100644 index 00000000000..0fbe6cb7625 --- /dev/null +++ b/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 diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index c1e4caf1b48..d2d910cb676 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/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