From 4895e76ef797127ce45b33ba3a9a2174293ce9d8 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 24 Apr 2023 06:47:28 +0100 Subject: [PATCH] DEV: Improve `add_to_serializer` `include_*` options (#21073) - Move the old '`define_include_method`' arg to a `respect_plugin_enabled` kwarg - Introduce an `include_condition` kwarg which can be passed a lambda with inclusion logic. Lambda will be run via `instance_exec` in the context of the serializer instance This is backwards compatible - old-style invocations will trigger a deprecation message Update chat and poll plugins to new pattern --- lib/plugin/instance.rb | 30 +++++++++-- plugins/chat/plugin.rb | 86 ++++++++++++++++++-------------- plugins/poll/plugin.rb | 22 ++++---- spec/lib/plugin/instance_spec.rb | 28 +++++++++++ 4 files changed, 113 insertions(+), 53 deletions(-) diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 60eeb014e06..cad7061ad12 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -109,7 +109,27 @@ class Plugin::Instance delegate :name, to: :metadata - def add_to_serializer(serializer, attr, define_include_method = true, &block) + def add_to_serializer( + serializer, + attr, + deprecated_respect_plugin_enabled = nil, + respect_plugin_enabled: true, + include_condition: nil, + &block + ) + if !deprecated_respect_plugin_enabled.nil? + Discourse.deprecate( + "add_to_serializer's respect_plugin_enabled argument should be passed as a keyword argument", + ) + respect_plugin_enabled = deprecated_respect_plugin_enabled + end + + if attr.to_s.starts_with?("include_") + Discourse.deprecate( + "add_to_serializer should not be used to directly override include_*? methods. Use the include_condition keyword argument instead", + ) + end + reloadable_patch do |plugin| base = begin @@ -123,9 +143,13 @@ class Plugin::Instance unless attr.to_s.start_with?("include_") klass.attributes(attr) - if define_include_method + if respect_plugin_enabled || include_condition # Don't include serialized methods if the plugin is disabled - klass.public_send(:define_method, "include_#{attr}?") { plugin.enabled? } + klass.public_send(:define_method, "include_#{attr}?") do + next false if respect_plugin_enabled && !plugin.enabled? + next instance_exec(&include_condition) if include_condition + true + end end end diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 83433ccdf60..d041adfeb3b 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -181,31 +181,48 @@ after_initialize do scope.user.id != object.id && scope.can_chat? && Guardian.new(object).can_chat? end - add_to_serializer(:current_user, :can_chat) { true } + add_to_serializer( + :current_user, + :can_chat, + include_condition: -> do + return @can_chat if defined?(@can_chat) + @can_chat = SiteSetting.chat_enabled && scope.can_chat? + end, + ) { true } - add_to_serializer(:current_user, :include_can_chat?) do - return @can_chat if defined?(@can_chat) + add_to_serializer( + :current_user, + :has_chat_enabled, + include_condition: -> do + return @has_chat_enabled if defined?(@has_chat_enabled) + @has_chat_enabled = include_can_chat? && object.user_option.chat_enabled + end, + ) { true } - @can_chat = SiteSetting.chat_enabled && scope.can_chat? - end + add_to_serializer( + :current_user, + :chat_sound, + include_condition: -> { include_has_chat_enabled? && object.user_option.chat_sound }, + ) { object.user_option.chat_sound } - add_to_serializer(:current_user, :has_chat_enabled) { true } + add_to_serializer( + :current_user, + :needs_channel_retention_reminder, + include_condition: -> do + include_has_chat_enabled? && object.staff? && + !object.user_option.dismissed_channel_retention_reminder && + !SiteSetting.chat_channel_retention_days.zero? + end, + ) { true } - add_to_serializer(:current_user, :include_has_chat_enabled?) do - return @has_chat_enabled if defined?(@has_chat_enabled) - - @has_chat_enabled = include_can_chat? && object.user_option.chat_enabled - end - - add_to_serializer(:current_user, :chat_sound) { object.user_option.chat_sound } - - add_to_serializer(:current_user, :include_chat_sound?) do - include_has_chat_enabled? && object.user_option.chat_sound - end - - add_to_serializer(:current_user, :needs_channel_retention_reminder) { true } - - add_to_serializer(:current_user, :needs_dm_retention_reminder) { true } + add_to_serializer( + :current_user, + :needs_dm_retention_reminder, + include_condition: -> do + include_has_chat_enabled? && !object.user_option.dismissed_dm_retention_reminder && + !SiteSetting.chat_dm_retention_days.zero? + end, + ) { true } add_to_serializer(:current_user, :has_joinable_public_channels) do Chat::ChannelFetcher.secured_public_channel_search( @@ -221,18 +238,11 @@ after_initialize do Chat::ChannelIndexSerializer.new(structured, scope: self.scope, root: false).as_json end - add_to_serializer(:current_user, :include_needs_channel_retention_reminder?) do - include_has_chat_enabled? && object.staff? && - !object.user_option.dismissed_channel_retention_reminder && - !SiteSetting.chat_channel_retention_days.zero? - end - - add_to_serializer(:current_user, :include_needs_dm_retention_reminder?) do - include_has_chat_enabled? && !object.user_option.dismissed_dm_retention_reminder && - !SiteSetting.chat_dm_retention_days.zero? - end - - add_to_serializer(:current_user, :chat_drafts) do + add_to_serializer( + :current_user, + :chat_drafts, + include_condition: -> { include_has_chat_enabled? }, + ) do Chat::Draft .where(user_id: object.id) .order(updated_at: :desc) @@ -241,13 +251,13 @@ after_initialize do .map { |row| { channel_id: row[0], data: row[1] } } end - add_to_serializer(:current_user, :include_chat_drafts?) { include_has_chat_enabled? } - add_to_serializer(:user_option, :chat_enabled) { object.chat_enabled } - add_to_serializer(:user_option, :chat_sound) { object.chat_sound } - - add_to_serializer(:user_option, :include_chat_sound?) { !object.chat_sound.blank? } + add_to_serializer( + :user_option, + :chat_sound, + include_condition: -> { !object.chat_sound.blank? }, + ) { object.chat_sound } add_to_serializer(:user_option, :only_chat_push_notifications) do object.only_chat_push_notifications diff --git a/plugins/poll/plugin.rb b/plugins/poll/plugin.rb index 84b337a823b..8ad92eae268 100644 --- a/plugins/poll/plugin.rb +++ b/plugins/poll/plugin.rb @@ -195,7 +195,7 @@ after_initialize do end end - add_to_serializer(:post, :preloaded_polls, false) do + add_to_class(PostSerializer, :preloaded_polls) do @preloaded_polls ||= if @topic_view.present? @topic_view.polls[object.id] @@ -204,15 +204,18 @@ after_initialize do end end - add_to_serializer(:post, :include_preloaded_polls?) { false } - - add_to_serializer(:post, :polls, false) do + add_to_serializer(:post, :polls, include_condition: -> { preloaded_polls.present? }) do preloaded_polls.map { |p| PollSerializer.new(p, root: false, scope: self.scope) } end - add_to_serializer(:post, :include_polls?) { SiteSetting.poll_enabled && preloaded_polls.present? } - - add_to_serializer(:post, :polls_votes, false) do + add_to_serializer( + :post, + :polls_votes, + include_condition: -> do + scope.user&.id.present? && preloaded_polls.present? && + preloaded_polls.any? { |p| p.has_voted?(scope.user) } + end, + ) do preloaded_polls .map do |poll| user_poll_votes = @@ -227,11 +230,6 @@ after_initialize do .to_h end - add_to_serializer(:post, :include_polls_votes?) do - SiteSetting.poll_enabled && scope.user&.id.present? && preloaded_polls.present? && - preloaded_polls.any? { |p| p.has_voted?(scope.user) } - end - register_search_advanced_filter(/in:polls/) do |posts, match| if SiteSetting.poll_enabled posts.joins(:polls) diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb index 750226c8acf..de983da141e 100644 --- a/spec/lib/plugin/instance_spec.rb +++ b/spec/lib/plugin/instance_spec.rb @@ -75,6 +75,14 @@ RSpec.describe Plugin::Instance do # Serializer @plugin.add_to_serializer(:trout, :scales) { 1024 } + @plugin.add_to_serializer(:trout, :unconditional_scales, respect_plugin_enabled: false) do + 2048 + end + @plugin.add_to_serializer( + :trout, + :conditional_scales, + include_condition: -> { !!object.data&.[](:has_scales) }, + ) { 4096 } @serializer = TroutSerializer.new(@trout) @child_serializer = TroutJuniorSerializer.new(@trout) @@ -100,6 +108,7 @@ RSpec.describe Plugin::Instance do expect(@hello_count).to eq(1) expect(@serializer.scales).to eq(1024) expect(@serializer.include_scales?).to eq(false) + expect(@serializer.include_unconditional_scales?).to eq(true) expect(@serializer.name).to eq("a trout") expect(@child_serializer.scales).to eq(1024) @@ -107,6 +116,25 @@ RSpec.describe Plugin::Instance do expect(@child_serializer.name).to eq("a trout jr") end + it "can control the include_* implementation" do + @plugin.enabled = true + + expect(@serializer.scales).to eq(1024) + expect(@serializer.include_scales?).to eq(true) + + expect(@serializer.unconditional_scales).to eq(2048) + expect(@serializer.include_unconditional_scales?).to eq(true) + + expect(@serializer.include_conditional_scales?).to eq(false) + @trout.data = { has_scales: true } + expect(@serializer.include_conditional_scales?).to eq(true) + + @plugin.enabled = false + expect(@serializer.include_scales?).to eq(false) + expect(@serializer.include_unconditional_scales?).to eq(true) + expect(@serializer.include_conditional_scales?).to eq(false) + end + it "only returns HTML if enabled" do ctx = Trout.new ctx.data = "hello"