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

@@ -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
 

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

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)

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"