From 52a50f10281c004c6ecf79b9249f80995c2ef842 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Fri, 7 Feb 2025 11:23:43 +1000
Subject: [PATCH] PERF: Admin plugin preload settings routes (#31215)

Followup 503f9b6f02ac5c4918d41611848c886b8755e5a0

This previous commit introduced an autogenerated
settings route for every plugin with more than one
setting defined. Plugins with only one setting
only have enabled_site_settings defined, which are
handled using the toggle in the admin plugin list,
so we don't need a dedicated setting page for them.

However in production this introduced a performance
issue, since we were looking through SiteSetting.all_settings
for every plugin, which could be quite slow in some
cases especially on our hosting.

Instead, we already have all the plugin settings cached
inside `SiteSetting.plugins`. We can instead use this to
count how many settings the plugin has, then if there is > 1
for a plugin we use the settings route. This is a much faster lookup
than
searching through SiteSetting.all_settings.
---
 app/serializers/admin_plugin_serializer.rb |  6 +-
 lib/plugin/instance.rb                     | 15 +++--
 spec/lib/plugin/instance_spec.rb           | 76 ++++++++++++++++++++++
 3 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/app/serializers/admin_plugin_serializer.rb b/app/serializers/admin_plugin_serializer.rb
index a462f9d16f7..73efef681d7 100644
--- a/app/serializers/admin_plugin_serializer.rb
+++ b/app/serializers/admin_plugin_serializer.rb
@@ -61,15 +61,15 @@ class AdminPluginSerializer < ApplicationSerializer
   end
 
   def plugin_settings
-    @plugin_settings ||= SiteSetting.plugins.select { |_, v| v == id }
+    object.plugin_settings
   end
 
   def has_settings
-    plugin_settings.values.any?
+    object.any_settings?
   end
 
   def has_only_enabled_setting
-    plugin_settings.keys.length == 1 && plugin_settings.keys.first == enabled_setting
+    object.has_only_enabled_setting?
   end
 
   def include_url?
diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb
index aa282453812..58e62079037 100644
--- a/lib/plugin/instance.rb
+++ b/lib/plugin/instance.rb
@@ -123,8 +123,7 @@ class Plugin::Instance
     route = self.admin_route
 
     if route.blank?
-      return if !any_settings?
-
+      return if !any_settings? || has_only_enabled_setting?
       route = default_admin_route
     end
 
@@ -137,11 +136,15 @@ class Plugin::Instance
   end
 
   def any_settings?
-    return false if !configurable?
+    configurable? && plugin_settings.values.length.positive?
+  end
 
-    SiteSetting
-      .all_settings(filter_plugin: self.name)
-      .any? { |s| s[:setting] != @enabled_site_setting }
+  def has_only_enabled_setting?
+    any_settings? && plugin_settings.values.one?
+  end
+
+  def plugin_settings
+    @plugin_settings ||= SiteSetting.plugins.select { |_, plugin_name| plugin_name == self.name }
   end
 
   def configurable?
diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb
index 1691d279a21..02d64d8d6ba 100644
--- a/spec/lib/plugin/instance_spec.rb
+++ b/spec/lib/plugin/instance_spec.rb
@@ -1103,4 +1103,80 @@ TEXT
       )
     end
   end
+
+  describe "#full_admin_route" do
+    context "when there is no admin route defined for the plugin" do
+      context "if the plugin has more than one setting" do
+        before do
+          plugin_instance.stubs(:plugin_settings).returns(
+            { enabled_setting: plugin_instance.name, other_setting: plugin_instance.name },
+          )
+        end
+
+        it "returns the default settings route" do
+          expect(plugin_instance.full_admin_route).to eq(
+            {
+              auto_generated: true,
+              full_location: "adminPlugins.show",
+              label: "discourse_sample_plugin.title",
+              location: "discourse-sample-plugin",
+              use_new_show_route: true,
+            },
+          )
+        end
+      end
+
+      context "if the plugin has only one setting (which is the enabled setting)" do
+        before do
+          plugin_instance.stubs(:plugin_settings).returns({ enabled_setting: plugin_instance.name })
+        end
+
+        it "returns nothing" do
+          expect(plugin_instance.full_admin_route).to be_nil
+        end
+      end
+
+      context "if the plugin is not configurable" do
+        before { plugin_instance.stubs(:configurable?).returns(false) }
+
+        it "returns nothing" do
+          expect(plugin_instance.full_admin_route).to be_nil
+        end
+      end
+    end
+
+    context "when there is an admin route defined for the plugin" do
+      context "when using the new show route" do
+        before { plugin_instance.add_admin_route("test", "testIndex", use_new_show_route: true) }
+
+        it "returns the correct details" do
+          expect(plugin_instance.full_admin_route).to eq(
+            {
+              auto_generated: false,
+              full_location: "adminPlugins.show",
+              label: "test",
+              location: "testIndex",
+              use_new_show_route: true,
+            },
+          )
+        end
+      end
+
+      context "when not using the new show route" do
+        before { plugin_instance.add_admin_route("test", "testIndex") }
+
+        it "returns the correct details" do
+          expect(plugin_instance.full_admin_route).to eq(
+            {
+              auto_generated: false,
+              full_location: "adminPlugins.testIndex",
+              label: "test",
+              location: "testIndex",
+              use_new_show_route: false,
+            },
+          )
+        end
+      end
+    end
+  end
 end