From 360d0dde650704a0f01fd6d8b525e933b1d7fcf2 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 8 Mar 2023 10:39:12 +1000
Subject: [PATCH] DEV: Change Bookmarkable registration to
 DiscoursePluginRegistry (#20556)

Similar spirit to e195e6f614de7a4c4f151ad947578fb69f8917f0,
this moves the Bookmarkable registration to DiscoursePluginRegistry
so plugins which are not enabled do not register additional
bookmarkable classes.
---
 app/models/bookmark.rb                        | 33 +++++--------------
 app/services/base_bookmarkable.rb             |  2 +-
 app/services/registered_bookmarkable.rb       |  4 +--
 lib/discourse_plugin_registry.rb              |  1 +
 lib/plugin/instance.rb                        | 10 ++++++
 plugins/chat/plugin.rb                        |  3 +-
 .../lib/chat_message_bookmarkable_spec.rb     |  4 ++-
 plugins/chat/spec/models/chat_message_spec.rb |  2 --
 plugins/chat/spec/plugin_helper.rb            |  3 --
 spec/lib/bookmark_query_spec.rb               |  9 ++---
 spec/models/bookmark_spec.rb                  |  2 ++
 spec/models/user_bookmark_list_spec.rb        |  2 ++
 spec/rails_helper.rb                          |  3 --
 spec/requests/users_controller_spec.rb        |  2 +-
 .../user_bookmark_list_serializer_spec.rb     |  2 ++
 spec/support/bookmarkable_helper.rb           |  7 ++--
 16 files changed, 43 insertions(+), 46 deletions(-)

diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index e69e67fd02f..845ba5c76a0 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -1,36 +1,21 @@
 # frozen_string_literal: true
 
 class Bookmark < ActiveRecord::Base
-  cattr_accessor :registered_bookmarkables
-  self.registered_bookmarkables = []
+  DEFAULT_BOOKMARKABLES = [
+    RegisteredBookmarkable.new(PostBookmarkable),
+    RegisteredBookmarkable.new(TopicBookmarkable),
+  ]
+
+  def self.registered_bookmarkables
+    Set.new(DEFAULT_BOOKMARKABLES | DiscoursePluginRegistry.bookmarkables)
+  end
 
   def self.registered_bookmarkable_from_type(type)
     Bookmark.registered_bookmarkables.find { |bm| bm.model.name == type }
   end
 
-  def self.register_bookmarkable(bookmarkable_klass)
-    return if Bookmark.registered_bookmarkable_from_type(bookmarkable_klass.model.name).present?
-    Bookmark.registered_bookmarkables << RegisteredBookmarkable.new(bookmarkable_klass)
-  end
-
-  ##
-  # This is called when the app loads, similar to AdminDashboardData.reset_problem_checks,
-  # so the default Post and Topic bookmarkables are registered on
-  # boot.
-  #
-  # This method also can be used in testing to reset bookmarkables between
-  # tests. It will also fire multiple times in development mode because
-  # classes are not cached.
-  def self.reset_bookmarkables
-    self.registered_bookmarkables = []
-
-    Bookmark.register_bookmarkable(PostBookmarkable)
-    Bookmark.register_bookmarkable(TopicBookmarkable)
-  end
-  reset_bookmarkables
-
   def self.valid_bookmarkable_types
-    Bookmark.registered_bookmarkables.map(&:model).map(&:to_s)
+    Bookmark.registered_bookmarkables.map { |bm| bm.model.to_s }
   end
 
   belongs_to :user
diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb
index 77c19c7147f..c7da12abc88 100644
--- a/app/services/base_bookmarkable.rb
+++ b/app/services/base_bookmarkable.rb
@@ -2,7 +2,7 @@
 
 ##
 # Anything that we want to be able to bookmark must be registered as a
-# bookmarkable type using Bookmark.register_bookmarkable(bookmarkable_klass),
+# bookmarkable type using Plugin::Instance#register_bookmarkable(bookmarkable_klass),
 # where the bookmarkable_klass is a class that implements this BaseBookmarkable
 # interface. Some examples are TopicBookmarkable and PostBookmarkable.
 #
diff --git a/app/services/registered_bookmarkable.rb b/app/services/registered_bookmarkable.rb
index 50cd45735a9..e71f45156b2 100644
--- a/app/services/registered_bookmarkable.rb
+++ b/app/services/registered_bookmarkable.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 #
-# Should only be created via the Bookmark.register_bookmarkable
+# Should only be created via the Plugin::Instance#register_bookmarkable
 # method; this is used to let the BookmarkQuery class query and
 # search additional bookmarks for the user bookmark list, and
 # also to enumerate on the registered RegisteredBookmarkable types.
@@ -11,7 +11,7 @@
 # when trying to save the Bookmark record. All things that are bookmarkable
 # must be registered in this way.
 #
-# See Bookmark#reset_bookmarkables for some examples on how registering
+# See Plugin::Instance#register_bookmarkable for some examples on how registering
 # bookmarkables works.
 #
 # See BaseBookmarkable for documentation on what return types should be
diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb
index ef05d3845ab..53a46066ec5 100644
--- a/lib/discourse_plugin_registry.rb
+++ b/lib/discourse_plugin_registry.rb
@@ -109,6 +109,7 @@ class DiscoursePluginRegistry
   define_filtered_register :search_groups_set_query_callbacks
 
   define_filtered_register :about_stat_groups
+  define_filtered_register :bookmarkables
 
   def self.register_auth_provider(auth_provider)
     self.auth_providers << auth_provider
diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb
index 26f9b4c5091..ce8ce5e8b11 100644
--- a/lib/plugin/instance.rb
+++ b/lib/plugin/instance.rb
@@ -1221,6 +1221,16 @@ class Plugin::Instance
     DiscoursePluginRegistry.register_user_destroyer_on_content_deletion_callback(callback, self)
   end
 
+  ##
+  # Register a class that implements [BaseBookmarkable], which represents another
+  # [ActiveRecord::Model] that may be bookmarked via the [Bookmark] model's
+  # polymorphic association. The class handles create and destroy hooks, querying,
+  # and reminders among other things.
+  def register_bookmarkable(klass)
+    return if Bookmark.registered_bookmarkable_from_type(klass.model.name).present?
+    DiscoursePluginRegistry.register_bookmarkable(RegisteredBookmarkable.new(klass), self)
+  end
+
   protected
 
   def self.js_path
diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb
index 5f85f631e87..29f7347f35c 100644
--- a/plugins/chat/plugin.rb
+++ b/plugins/chat/plugin.rb
@@ -280,7 +280,6 @@ after_initialize do
     Category.prepend Chat::CategoryExtension
     User.prepend Chat::UserExtension
     Jobs::UserEmail.prepend Chat::UserEmailExtension
-    Bookmark.register_bookmarkable(ChatMessageBookmarkable)
   end
 
   if Oneboxer.respond_to?(:register_local_handler)
@@ -777,6 +776,8 @@ after_initialize do
   register_user_destroyer_on_content_deletion_callback(
     Proc.new { |user| Jobs.enqueue(:delete_user_messages, user_id: user.id) },
   )
+
+  register_bookmarkable(ChatMessageBookmarkable)
 end
 
 if Rails.env == "test"
diff --git a/plugins/chat/spec/lib/chat_message_bookmarkable_spec.rb b/plugins/chat/spec/lib/chat_message_bookmarkable_spec.rb
index a78aa55588c..e9678c84089 100644
--- a/plugins/chat/spec/lib/chat_message_bookmarkable_spec.rb
+++ b/plugins/chat/spec/lib/chat_message_bookmarkable_spec.rb
@@ -11,10 +11,12 @@ describe ChatMessageBookmarkable do
   fab!(:channel) { Fabricate(:category_channel) }
 
   before do
-    Bookmark.register_bookmarkable(ChatMessageBookmarkable)
+    register_test_bookmarkable(ChatMessageBookmarkable)
     UserChatChannelMembership.create(chat_channel: channel, user: user, following: true)
   end
 
+  after { DiscoursePluginRegistry.reset! }
+
   let!(:message1) { Fabricate(:chat_message, chat_channel: channel) }
   let!(:message2) { Fabricate(:chat_message, chat_channel: channel) }
   let!(:bookmark1) do
diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb
index 5f2d5c21509..93d98badb26 100644
--- a/plugins/chat/spec/models/chat_message_spec.rb
+++ b/plugins/chat/spec/models/chat_message_spec.rb
@@ -499,8 +499,6 @@ describe ChatMessage do
     end
 
     describe "bookmarks" do
-      before { Bookmark.register_bookmarkable(ChatMessageBookmarkable) }
-
       it "destroys bookmarks" do
         message_1 = Fabricate(:chat_message)
         bookmark_1 = Fabricate(:bookmark, bookmarkable: message_1)
diff --git a/plugins/chat/spec/plugin_helper.rb b/plugins/chat/spec/plugin_helper.rb
index 834dbbe1f41..c3c52cf6a5e 100644
--- a/plugins/chat/spec/plugin_helper.rb
+++ b/plugins/chat/spec/plugin_helper.rb
@@ -18,9 +18,6 @@ module ChatSystemHelpers
     end
 
     Group.refresh_automatic_groups!
-
-    # this is reset after each test
-    Bookmark.register_bookmarkable(ChatMessageBookmarkable)
   end
 
   def chat_thread_chain_bootstrap(channel:, users:, messages_count: 4)
diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb
index 0017dfbf595..3ce8ad94711 100644
--- a/spec/lib/bookmark_query_spec.rb
+++ b/spec/lib/bookmark_query_spec.rb
@@ -12,7 +12,6 @@ RSpec.describe BookmarkQuery do
 
   describe "#list_all" do
     before do
-      Bookmark.reset_bookmarkables
       register_test_bookmarkable
 
       Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
@@ -20,14 +19,14 @@ RSpec.describe BookmarkQuery do
       user_bookmark
     end
 
+    after { DiscoursePluginRegistry.reset! }
+
     let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
     let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
     let(:user_bookmark) do
       Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen"))
     end
 
-    after { Bookmark.reset_bookmarkables }
-
     it "returns all the bookmarks for a user" do
       expect(bookmark_query.list_all.count).to eq(3)
     end
@@ -77,9 +76,7 @@ RSpec.describe BookmarkQuery do
         )
       end
 
-      before { Bookmark.reset_bookmarkables }
-
-      after { Bookmark.reset_bookmarkables }
+      after { DiscoursePluginRegistry.reset! }
 
       let(:bookmark3) do
         Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post))
diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb
index 362c1ee1ead..188a965c47d 100644
--- a/spec/models/bookmark_spec.rb
+++ b/spec/models/bookmark_spec.rb
@@ -4,6 +4,8 @@ RSpec.describe Bookmark do
   fab!(:post) { Fabricate(:post) }
 
   describe "Validations" do
+    after { DiscoursePluginRegistry.reset! }
+
     it "does not allow a user to create a bookmark with only one polymorphic column" do
       user = Fabricate(:user)
       bm = Bookmark.create(bookmarkable_id: post.id, user: user)
diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb
index a087f7af22e..1876ef3f827 100644
--- a/spec/models/user_bookmark_list_spec.rb
+++ b/spec/models/user_bookmark_list_spec.rb
@@ -13,6 +13,8 @@ RSpec.describe UserBookmarkList do
     user_bookmark
   end
 
+  after { DiscoursePluginRegistry.reset! }
+
   let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
   let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
   let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index ecdd9227047..666e83a87b7 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -145,9 +145,6 @@ module TestSetup
     # Don't queue badge grant in test mode
     BadgeGranter.disable_queue
 
-    # Make sure the default Post and Topic bookmarkables are registered
-    Bookmark.reset_bookmarkables
-
     OmniAuth.config.test_mode = false
   end
 end
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index a86dd23089e..108192418a3 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -5947,7 +5947,7 @@ RSpec.describe UsersController do
       bookmark3 && bookmark4
     end
 
-    after { Bookmark.registered_bookmarkables = [] }
+    after { DiscoursePluginRegistry.reset! }
 
     let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) }
     let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) }
diff --git a/spec/serializers/user_bookmark_list_serializer_spec.rb b/spec/serializers/user_bookmark_list_serializer_spec.rb
index 9c90ef0f47a..6c5934ccb25 100644
--- a/spec/serializers/user_bookmark_list_serializer_spec.rb
+++ b/spec/serializers/user_bookmark_list_serializer_spec.rb
@@ -11,6 +11,8 @@ RSpec.describe UserBookmarkListSerializer do
       user_bookmark
     end
 
+    after { DiscoursePluginRegistry.reset! }
+
     let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
     let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
     let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
diff --git a/spec/support/bookmarkable_helper.rb b/spec/support/bookmarkable_helper.rb
index 7a9f053fd78..a9644b8d963 100644
--- a/spec/support/bookmarkable_helper.rb
+++ b/spec/support/bookmarkable_helper.rb
@@ -72,6 +72,9 @@ class UserTestBookmarkable < BaseBookmarkable
   end
 end
 
-def register_test_bookmarkable
-  Bookmark.register_bookmarkable(UserTestBookmarkable)
+def register_test_bookmarkable(klass = UserTestBookmarkable)
+  DiscoursePluginRegistry.register_bookmarkable(
+    RegisteredBookmarkable.new(klass),
+    stub(enabled?: true),
+  )
 end