From e21c640a3cb0fc6322ee90843a198ae2df7decf7 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 6 Jan 2022 08:56:05 +1000 Subject: [PATCH] DEV: Add polymorphic bookmarkable columns (#15454) We are planning on attaching bookmarks to more and more other models, so it makes sense to make a polymorphic relationship to handle this. This commit adds the new columns and backfills them in the bookmark table, and makes sure that any new bookmark changes fill in the columns via DB triggers. This way we can gradually change the frontend and backend to use these new columns, and eventually delete the old post_id and for_topic columns in `bookmarks`. --- app/models/bookmark.rb | 4 ++ app/models/post.rb | 3 ++ app/models/topic.rb | 8 ++++ ...053343_add_bookmark_polymorphic_columns.rb | 10 ++++ ...lymorphic_bookmark_columns_to_sync_data.rb | 48 +++++++++++++++++++ spec/lib/bookmark_manager_spec.rb | 8 +++- 6 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220104053343_add_bookmark_polymorphic_columns.rb create mode 100644 db/migrate/20220105024605_add_trigger_for_polymorphic_bookmark_columns_to_sync_data.rb diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index c90f0454ad4..cbf83181e0c 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -4,6 +4,7 @@ class Bookmark < ActiveRecord::Base belongs_to :user belongs_to :post has_one :topic, through: :post + belongs_to :bookmarkable, polymorphic: true delegate :topic_id, to: :post @@ -171,9 +172,12 @@ 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/app/models/post.rb b/app/models/post.rb index 7120db505e8..04df7e932fc 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -46,6 +46,9 @@ class Post < ActiveRecord::Base has_many :uploads, through: :post_uploads has_one :post_stat + + # When we are ready we can add as: :bookmarkable here to use the + # polymorphic association. has_many :bookmarks has_one :incoming_email diff --git a/app/models/topic.rb b/app/models/topic.rb index 1f835e4237a..1a6948c6cc6 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -203,7 +203,15 @@ class Topic < ActiveRecord::Base belongs_to :category has_many :category_users, through: :category has_many :posts + + # When we are ready we can add as: :bookmarkable here to use the + # polymorphic association. + # + # At that time we may also want to make another association for example + # :topic_bookmarks that get all of the bookmarks for that topic's bookmarkable id + # and type, because this one gets all of the post 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/db/migrate/20220104053343_add_bookmark_polymorphic_columns.rb b/db/migrate/20220104053343_add_bookmark_polymorphic_columns.rb new file mode 100644 index 00000000000..ebf87f84d83 --- /dev/null +++ b/db/migrate/20220104053343_add_bookmark_polymorphic_columns.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddBookmarkPolymorphicColumns < ActiveRecord::Migration[6.1] + def change + add_column :bookmarks, :bookmarkable_id, :integer + add_column :bookmarks, :bookmarkable_type, :string + + add_index :bookmarks, [:user_id, :bookmarkable_type, :bookmarkable_id], name: "idx_bookmarks_user_polymorphic_unique", unique: true + end +end 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 new file mode 100644 index 00000000000..f1ae13477ff --- /dev/null +++ b/db/migrate/20220105024605_add_trigger_for_polymorphic_bookmark_columns_to_sync_data.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +class AddTriggerForPolymorphicBookmarkColumnsToSyncData < ActiveRecord::Migration[6.1] + def up + DB.exec <<~SQL + CREATE OR REPLACE FUNCTION sync_bookmarks_polymorphic_column_data() + RETURNS TRIGGER + LANGUAGE PLPGSQL AS $rcr$ + BEGIN + IF NEW.for_topic + THEN + NEW.bookmarkable_id = (SELECT topic_id FROM posts WHERE posts.id = NEW.post_id); + NEW.bookmarkable_type = 'Topic'; + ELSE + NEW.bookmarkable_id = NEW.post_id; + NEW.bookmarkable_type = 'Post'; + END IF; + RETURN NEW; + END + $rcr$; + SQL + + DB.exec <<~SQL + CREATE TRIGGER bookmarks_polymorphic_data_sync + BEFORE INSERT OR UPDATE OF post_id, for_topic ON bookmarks + FOR EACH ROW + EXECUTE FUNCTION sync_bookmarks_polymorphic_column_data(); + 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 + end + + def down + DB.exec("DROP TRIGGER IF EXISTS bookmarks_polymorphic_data_sync") + DB.exec("DROP FUNCTION IF EXISTS sync_bookmarks_polymorphic_column_data") + end +end diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index d2d910cb676..c1e4caf1b48 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -12,12 +12,14 @@ RSpec.describe BookmarkManager do subject { described_class.new(user) } describe ".create" do - it "creates the bookmark for the user" do + it "creates the bookmark for the user with the correct polymorphic type" 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 @@ -27,6 +29,8 @@ 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) @@ -34,6 +38,8 @@ 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