From 461b4e5cc6e747d2aa57163d840d79781e10a937 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 15 May 2020 14:04:38 +0100 Subject: [PATCH] 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 | 2 +- app/controllers/groups_controller.rb | 2 +- app/models/group.rb | 14 +---- app/models/user.rb | 56 +++++-------------- lib/discourse_plugin_registry.rb | 31 ++++++++++ lib/plugin/instance.rb | 18 +++--- .../discourse_plugin_registry_spec.rb | 46 +++++++++++++++ spec/requests/admin/groups_controller_spec.rb | 4 +- spec/requests/groups_controller_spec.rb | 4 +- spec/requests/reviewables_controller_spec.rb | 2 +- spec/requests/users_controller_spec.rb | 6 +- spec/serializers/user_serializer_spec.rb | 4 +- 12 files changed, 111 insertions(+), 78 deletions(-) diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index a8413ebefcd..50cef291640 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/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) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index bb763811d08..2716282d6ad 100644 --- a/app/controllers/groups_controller.rb +++ b/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 diff --git a/app/models/group.rb b/app/models/group.rb index 5bc41d7b42b..80c6080bc1b 100644 --- a/app/models/group.rb +++ b/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 - end - - 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 + Discourse.deprecate("Editable group custom fields should be registered using the plugin API", since: "v2.4.0.beta4", drop_from: "v2.5.0") + DiscoursePluginRegistry.register_editable_group_custom_field(custom_field_name, plugin) end def downcase_incoming_email diff --git a/app/models/user.rb b/app/models/user.rb index f932f15abfd..9d682a62bd8 100644 --- a/app/models/user.rb +++ b/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 = [] - - plugin_editable_user_custom_fields.each do |k, v| - 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.push *DiscoursePluginRegistry.self_editable_user_custom_fields + fields.push *DiscoursePluginRegistry.staff_editable_user_custom_fields if by_staff fields.uniq end - def self.plugin_staff_user_custom_fields - @plugin_staff_user_custom_fields ||= {} + def self.register_plugin_editable_user_custom_field(custom_field_name, plugin, staff_only: false) + 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 - end - - def self.plugin_public_user_custom_fields - @plugin_public_user_custom_fields ||= {} + Discourse.deprecate("Staff user custom fields should be registered using the plugin API", since: "v2.4.0.beta4", drop_from: "v2.5.0") + DiscoursePluginRegistry.register_staff_user_custom_field(custom_field_name, plugin) 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 << k if v.enabled? - end + fields.push *DiscoursePluginRegistry.public_user_custom_fields 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('|') - end - plugin_staff_user_custom_fields.each do |k, v| - fields << k if v.enabled? + fields.push *SiteSetting.staff_user_custom_fields.split('|') end + + fields.push *DiscoursePluginRegistry.staff_user_custom_fields end fields.uniq diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index acd35ecc792..ae260e2ce1a 100644 --- a/lib/discourse_plugin_registry.rb +++ b/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 diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 647a453b20a..a993ce52f0b 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -146,27 +146,23 @@ class Plugin::Instance end def whitelist_staff_user_custom_field(field) - reloadable_patch do |plugin| - ::User.register_plugin_staff_custom_field(field, plugin) # plugin.enabled? is checked at runtime - end + DiscoursePluginRegistry.register_staff_user_custom_field(field, self) end def whitelist_public_user_custom_field(field) - reloadable_patch do |plugin| - ::User.register_plugin_public_custom_field(field, plugin) # plugin.enabled? is checked at runtime - end + DiscoursePluginRegistry.register_public_user_custom_field(field, self) end def register_editable_user_custom_field(field, staff_only: false) - reloadable_patch do |plugin| - ::User.register_plugin_editable_user_custom_field(field, plugin, staff_only: staff_only) # plugin.enabled? is checked at runtime + if staff_only + 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| - ::Group.register_plugin_editable_group_custom_field(field, plugin) # plugin.enabled? is checked at runtime - end + DiscoursePluginRegistry.register_editable_group_custom_field(field, self) end def custom_avatar_column(column) diff --git a/spec/components/discourse_plugin_registry_spec.rb b/spec/components/discourse_plugin_registry_spec.rb index 2fb9f41fef6..3dc1bce8821 100644 --- a/spec/components/discourse_plugin_registry_spec.rb +++ b/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! diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb index 1b0f1fd297e..1279d33ed3d 100644 --- a/spec/requests/admin/groups_controller_spec.rb +++ b/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 }) diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 45a4db2d21f..ff387a2ff37 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/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 diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index 88064fa8eb1..c14b0d8cd53 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index cfb1437c065..af09e50f1e3 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1851,8 +1851,7 @@ describe UsersController do end after do - User.plugin_editable_user_custom_fields.clear - User.plugin_staff_editable_user_custom_fields.clear + DiscoursePluginRegistry.reset! 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 - User.plugin_staff_editable_user_custom_fields.clear + DiscoursePluginRegistry.reset! 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 diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 9aa80e5011b..2086c351ba2 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/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