DEV: Add framework for filtered plugin registers (#9763)

* DEV: Add framework for filtered plugin registers

Plugins often need to add values to a list, and we need to filter those lists at runtime to ignore values from disabled plugins. This commit provides a re-usable way to do that, which should make it easier to add new registers in future, and also reduce repeated code.

Follow-up commits will migrate existing registers to use this new system

* DEV: Migrate user and group custom field APIs to plugin registry

This gives us a consistent system for checking plugin enabled state, so we are repeating less logic. API changes are backwards compatible

app/controllers/admin/groups_controller.rb

@@ -177,7 +177,7 @@ class Admin::GroupsController < Admin::AdminController
       :usernames,
       :publish_read_state
     ]
-    custom_fields = Group.editable_group_custom_fields
+    custom_fields = DiscoursePluginRegistry.editable_group_custom_fields
     permitted << { custom_fields: custom_fields } unless custom_fields.blank?
 
     params.require(:group).permit(permitted)

app/controllers/groups_controller.rb

@@ -545,7 +545,7 @@ class GroupsController < ApplicationController
             :publish_read_state
           ])
 
-          custom_fields = Group.editable_group_custom_fields
+          custom_fields = DiscoursePluginRegistry.editable_group_custom_fields
           default_params << { custom_fields: custom_fields } unless custom_fields.blank?
         end
 

app/models/group.rb

@@ -266,19 +266,9 @@ class Group < ActiveRecord::Base
     end
   end
 
-  def self.plugin_editable_group_custom_fields
-    @plugin_editable_group_custom_fields ||= {}
-  end
-
   def self.register_plugin_editable_group_custom_field(custom_field_name, plugin)
-    plugin_editable_group_custom_fields[custom_field_name] = plugin
+    Discourse.deprecate("Editable group custom fields should be registered using the plugin API", since: "v2.4.0.beta4", drop_from: "v2.5.0")
-  end
+    DiscoursePluginRegistry.register_editable_group_custom_field(custom_field_name, plugin)
-
-  def self.editable_group_custom_fields
-    plugin_editable_group_custom_fields.reduce([]) do |fields, (k, v)|
-      next(fields) unless v.enabled?
-      fields << k
-    end.uniq
   end
 
   def downcase_incoming_email

app/models/user.rb

@@ -267,72 +267,44 @@ class User < ActiveRecord::Base
     end
   end
 
-  def self.plugin_editable_user_custom_fields
-    @plugin_editable_user_custom_fields ||= {}
-  end
-
-  def self.plugin_staff_editable_user_custom_fields
-    @plugin_staff_editable_user_custom_fields ||= {}
-  end
-
-  def self.register_plugin_editable_user_custom_field(custom_field_name, plugin, staff_only: false)
-    if staff_only
-      plugin_staff_editable_user_custom_fields[custom_field_name] = plugin
-    else
-      plugin_editable_user_custom_fields[custom_field_name] = plugin
-    end
-  end
-
   def self.editable_user_custom_fields(by_staff: false)
     fields = []
-
+    fields.push *DiscoursePluginRegistry.self_editable_user_custom_fields
-    plugin_editable_user_custom_fields.each do |k, v|
+    fields.push *DiscoursePluginRegistry.staff_editable_user_custom_fields if by_staff
-      fields << k if v.enabled?
-    end
-
-    if by_staff
-      plugin_staff_editable_user_custom_fields.each do |k, v|
-        fields << k if v.enabled?
-      end
-    end
 
     fields.uniq
   end
 
-  def self.plugin_staff_user_custom_fields
+  def self.register_plugin_editable_user_custom_field(custom_field_name, plugin, staff_only: false)
-    @plugin_staff_user_custom_fields ||= {}
+    Discourse.deprecate("Editable user custom fields should be registered using the plugin API", since: "v2.4.0.beta4", drop_from: "v2.5.0")
+    DiscoursePluginRegistry.register_editable_user_custom_field(custom_field_name, plugin, staff_only: staff_only)
   end
 
   def self.register_plugin_staff_custom_field(custom_field_name, plugin)
-    plugin_staff_user_custom_fields[custom_field_name] = plugin
+    Discourse.deprecate("Staff user custom fields should be registered using the plugin API",  since: "v2.4.0.beta4", drop_from: "v2.5.0")
-  end
+    DiscoursePluginRegistry.register_staff_user_custom_field(custom_field_name, plugin)
-
-  def self.plugin_public_user_custom_fields
-    @plugin_public_user_custom_fields ||= {}
   end
 
   def self.register_plugin_public_custom_field(custom_field_name, plugin)
-    plugin_public_user_custom_fields[custom_field_name] = plugin
+    Discourse.deprecate("Public user custom fields should be registered using the plugin API", since: "v2.4.0.beta4", drop_from: "v2.5.0")
+    DiscoursePluginRegistry.register_public_user_custom_field(custom_field_name, plugin)
   end
 
   def self.whitelisted_user_custom_fields(guardian)
     fields = []
 
-    plugin_public_user_custom_fields.each do |k, v|
+    fields.push *DiscoursePluginRegistry.public_user_custom_fields
-      fields << k if v.enabled?
-    end
 
     if SiteSetting.public_user_custom_fields.present?
-      fields += SiteSetting.public_user_custom_fields.split('|')
+      fields.push *SiteSetting.public_user_custom_fields.split('|')
     end
 
     if guardian.is_staff?
       if SiteSetting.staff_user_custom_fields.present?
-        fields += SiteSetting.staff_user_custom_fields.split('|')
+        fields.push *SiteSetting.staff_user_custom_fields.split('|')
-      end
-      plugin_staff_user_custom_fields.each do |k, v|
-        fields << k if v.enabled?
       end
+
+      fields.push *DiscoursePluginRegistry.staff_user_custom_fields
     end
 
     fields.uniq

lib/discourse_plugin_registry.rb

@@ -24,6 +24,29 @@ class DiscoursePluginRegistry
     end
   end
 
+  # Create a new register (see `define_register`) with some additions:
+  #   - Register is created in a class variable using the specified name/type
+  #   - Defines singleton method to access the register
+  #   - Defines instance method as a shortcut to the singleton method
+  #   - Automatically deletes the register on ::clear!
+  def self.define_filtered_register(register_name)
+    define_register(register_name, Array)
+
+    singleton_class.alias_method :"_raw_#{register_name}", :"#{register_name}"
+
+    define_singleton_method(register_name) do
+      unfiltered = public_send(:"_raw_#{register_name}")
+      unfiltered
+        .filter { |v| v[:plugin].enabled? }
+        .map { |v| v[:value] }
+        .uniq
+    end
+
+    define_singleton_method("register_#{register_name.to_s.singularize}") do |value, plugin|
+      public_send(:"_raw_#{register_name}") << { plugin: plugin, value: value }
+    end
+  end
+
   define_register :javascripts, Set
   define_register :auth_providers, Set
   define_register :service_workers, Set
@@ -44,6 +67,14 @@ class DiscoursePluginRegistry
   define_register :vendored_pretty_text, Set
   define_register :vendored_core_pretty_text, Set
 
+  define_filtered_register :staff_user_custom_fields
+  define_filtered_register :public_user_custom_fields
+
+  define_filtered_register :self_editable_user_custom_fields
+  define_filtered_register :staff_editable_user_custom_fields
+
+  define_filtered_register :editable_group_custom_fields
+
   def self.register_auth_provider(auth_provider)
     self.auth_providers << auth_provider
   end

lib/plugin/instance.rb

@@ -146,27 +146,23 @@ class Plugin::Instance
   end
 
   def whitelist_staff_user_custom_field(field)
-    reloadable_patch do |plugin|
+    DiscoursePluginRegistry.register_staff_user_custom_field(field, self)
-      ::User.register_plugin_staff_custom_field(field, plugin) # plugin.enabled? is checked at runtime
-    end
   end
 
   def whitelist_public_user_custom_field(field)
-    reloadable_patch do |plugin|
+    DiscoursePluginRegistry.register_public_user_custom_field(field, self)
-      ::User.register_plugin_public_custom_field(field, plugin) # plugin.enabled? is checked at runtime
-    end
   end
 
   def register_editable_user_custom_field(field, staff_only: false)
-    reloadable_patch do |plugin|
+    if staff_only
-      ::User.register_plugin_editable_user_custom_field(field, plugin, staff_only: staff_only) # plugin.enabled? is checked at runtime
+      DiscoursePluginRegistry.register_staff_editable_user_custom_field(field, self)
+    else
+      DiscoursePluginRegistry.register_self_editable_user_custom_field(field, self)
     end
   end
 
   def register_editable_group_custom_field(field)
-    reloadable_patch do |plugin|
+    DiscoursePluginRegistry.register_editable_group_custom_field(field, self)
-      ::Group.register_plugin_editable_group_custom_field(field, plugin) # plugin.enabled? is checked at runtime
-    end
   end
 
   def custom_avatar_column(column)

spec/components/discourse_plugin_registry_spec.rb

@@ -10,6 +10,52 @@ describe DiscoursePluginRegistry do
   let(:registry) { TestRegistry }
   let(:registry_instance) { registry.new }
 
+  context '::define_register' do
+    let(:fresh_registry) { Class.new(TestRegistry) }
+
+    let(:plugin_class) do
+      Class.new(Plugin::Instance) do
+        attr_accessor :enabled
+        def enabled?
+          @enabled
+        end
+      end
+    end
+
+    let(:plugin) { plugin_class.new }
+
+    it 'works for a set' do
+      fresh_registry.define_register(:test_things, Set)
+      fresh_registry.test_things << "My Thing"
+      expect(fresh_registry.test_things).to contain_exactly("My Thing")
+      fresh_registry.reset!
+      expect(fresh_registry.test_things.length).to eq(0)
+    end
+
+    it 'works for a hash' do
+      fresh_registry.define_register(:test_things, Hash)
+      fresh_registry.test_things[:test] = "hello world"
+      expect(fresh_registry.test_things[:test]).to eq("hello world")
+      fresh_registry.reset!
+      expect(fresh_registry.test_things[:test]).to eq(nil)
+    end
+
+    context '::define_filtered_register' do
+      it 'works' do
+        fresh_registry.define_filtered_register(:test_things)
+        expect(fresh_registry.test_things.length).to eq(0)
+
+        fresh_registry.register_test_thing("mything", plugin)
+
+        plugin.enabled = true
+        expect(fresh_registry.test_things).to contain_exactly("mything")
+
+        plugin.enabled = false
+        expect(fresh_registry.test_things.length).to eq(0)
+      end
+    end
+  end
+
   context '#stylesheets' do
     it 'defaults to an empty Set' do
       registry.reset!

spec/requests/admin/groups_controller_spec.rb

@@ -46,7 +46,7 @@ RSpec.describe Admin::GroupsController do
       end
 
       after do
-        Group.plugin_editable_group_custom_fields.clear
+        DiscoursePluginRegistry.reset!
       end
 
       it "only updates allowed user fields" do
@@ -63,7 +63,7 @@ RSpec.describe Admin::GroupsController do
       end
 
       it "is secure when there are no registered editable fields" do
-        Group.plugin_editable_group_custom_fields.clear
+        DiscoursePluginRegistry.reset!
         params = group_params
         params[:group].merge!(custom_fields: { test: :hello1, test2: :hello2 })
 

spec/requests/groups_controller_spec.rb

@@ -620,7 +620,7 @@ describe GroupsController do
       end
 
       after do
-        Group.plugin_editable_group_custom_fields.clear
+        DiscoursePluginRegistry.reset!
       end
 
       it "only updates allowed user fields" do
@@ -634,7 +634,7 @@ describe GroupsController do
       end
 
       it "is secure when there are no registered editable fields" do
-        Group.plugin_editable_group_custom_fields.clear
+        DiscoursePluginRegistry.reset!
         put "/groups/#{@group.id}.json", params: { group: { custom_fields: { test: :hello1, test2: :hello2 } } }
 
         @group.reload

spec/requests/reviewables_controller_spec.rb

@@ -200,7 +200,7 @@ describe ReviewablesController do
         end
 
         after do
-          User.plugin_public_user_custom_fields.clear
+          DiscoursePluginRegistry.reset!
         end
 
         it "returns user data with custom fields" do

spec/requests/users_controller_spec.rb

@@ -1851,8 +1851,7 @@ describe UsersController do
             end
 
             after do
-              User.plugin_editable_user_custom_fields.clear
+              DiscoursePluginRegistry.reset!
-              User.plugin_staff_editable_user_custom_fields.clear
             end
 
             it "only updates allowed user fields" do
@@ -1903,8 +1902,7 @@ describe UsersController do
             end
 
             it "is secure when there are no registered editable fields" do
-              User.plugin_editable_user_custom_fields.clear
+              DiscoursePluginRegistry.reset!
-              User.plugin_staff_editable_user_custom_fields.clear
               put "/u/#{user.username}.json", params: { custom_fields: { test1: :hello1, test2: :hello2, test3: :hello3 } }
               expect(response.status).to eq(200)
               expect(user.custom_fields["test1"]).to be_blank

spec/serializers/user_serializer_spec.rb

@@ -257,10 +257,10 @@ describe UserSerializer do
       end
 
       after do
-        User.plugin_public_user_custom_fields.clear
+        DiscoursePluginRegistry.reset!
       end
 
-      it "serializes the fields listed in plugin_public_user_custom_fields" do
+      it "serializes the fields listed in public_user_custom_fields" do
         expect(json[:custom_fields]['public_field']).to eq(user.custom_fields['public_field'])
         expect(json[:custom_fields]['secret_field']).to eq(nil)
       end