From 628873de24049963c88a2e6d6acf6f8dd07a7afa Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 8 Jan 2024 09:57:25 +1000
Subject: [PATCH] FIX: Sort plugins by their setting category name (#25128)

Some plugins have names (e.g. discourse-x-yz) that
are totally different from what they are actually called,
and that causes issues when showing them in a sorted way
in the admin plugin list.

Now, we should use the setting category name from client.en.yml
if it exists, otherwise fall back to the name, for sorting.
This is what we do on the client to determine what text to
show for the plugin name as well.
---
 lib/discourse.rb                             |  6 ++-
 lib/plugin/instance.rb                       | 16 +++++-
 lib/plugin/metadata.rb                       |  4 --
 plugins/chat/config/locales/client.en.yml    |  5 +-
 spec/lib/discourse_spec.rb                   | 24 +++++++++
 spec/lib/plugin/instance_spec.rb             | 52 +++++++++++++++++++-
 spec/support/sample_plugin_site_settings.yml |  4 +-
 7 files changed, 101 insertions(+), 10 deletions(-)

diff --git a/lib/discourse.rb b/lib/discourse.rb
index ccf650b9ed8..bc7cdb5363b 100644
--- a/lib/discourse.rb
+++ b/lib/discourse.rb
@@ -378,8 +378,10 @@ module Discourse
   end
 
   def self.plugins_sorted_by_name(enabled_only: true)
-    return visible_plugins.filter(&:enabled?).sort_by(&:name_without_prefix) if enabled_only
-    visible_plugins.sort_by(&:name_without_prefix)
+    if enabled_only
+      return visible_plugins.filter(&:enabled?).sort_by { |plugin| plugin.humanized_name.downcase }
+    end
+    visible_plugins.sort_by { |plugin| plugin.humanized_name.downcase }
   end
 
   def self.plugin_themes
diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb
index 63ba5527934..f9725917496 100644
--- a/lib/plugin/instance.rb
+++ b/lib/plugin/instance.rb
@@ -122,7 +122,11 @@ class Plugin::Instance
     @enabled_site_setting ? SiteSetting.get(@enabled_site_setting) : true
   end
 
-  delegate :name, :name_without_prefix, to: :metadata
+  delegate :name, to: :metadata
+
+  def humanized_name
+    (setting_category_name || name).delete_prefix("Discourse ").delete_prefix("discourse-")
+  end
 
   def add_to_serializer(
     serializer,
@@ -1369,6 +1373,16 @@ class Plugin::Instance
 
   private
 
+  def setting_category
+    return if @enabled_site_setting.blank?
+    SiteSetting.categories[enabled_site_setting]
+  end
+
+  def setting_category_name
+    return if setting_category.blank? || setting_category == "plugins"
+    I18n.t("admin_js.admin.site_settings.categories.#{setting_category}")
+  end
+
   def validate_directory_column_name(column_name)
     match = /\A[_a-z]+\z/.match(column_name)
     unless match
diff --git a/lib/plugin/metadata.rb b/lib/plugin/metadata.rb
index 561c36924c2..14e269fcbc2 100644
--- a/lib/plugin/metadata.rb
+++ b/lib/plugin/metadata.rb
@@ -133,10 +133,6 @@ class Plugin::Metadata
       end
   end
 
-  def name_without_prefix
-    name.downcase.delete_prefix("discourse-")
-  end
-
   def self.parse(text)
     metadata = self.new
     text.each_line { |line| break unless metadata.parse_line(line) }
diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml
index 71315d8a513..b0f08ca96c7 100644
--- a/plugins/chat/config/locales/client.en.yml
+++ b/plugins/chat/config/locales/client.en.yml
@@ -1,9 +1,12 @@
 en:
-  js:
+  admin_js:
     admin:
       site_settings:
         categories:
           chat: Chat
+  js:
+    admin:
+      site_settings:
         chat_separate_sidebar_mode:
           always: "Always"
           fullscreen: "When chat is in fullscreen"
diff --git a/spec/lib/discourse_spec.rb b/spec/lib/discourse_spec.rb
index 13edcda3a9c..becf38e8528 100644
--- a/spec/lib/discourse_spec.rb
+++ b/spec/lib/discourse_spec.rb
@@ -67,6 +67,30 @@ RSpec.describe Discourse do
     end
   end
 
+  describe ".plugins_sorted_by_name" do
+    before do
+      Discourse.stubs(:visible_plugins).returns(
+        [
+          stub(enabled?: false, name: "discourse-doctor-sleep", humanized_name: "Doctor Sleep"),
+          stub(enabled?: true, name: "discourse-shining", humanized_name: "The Shining"),
+          stub(enabled?: true, name: "discourse-misery", humanized_name: "misery"),
+        ],
+      )
+    end
+
+    it "sorts enabled plugins by humanized name" do
+      expect(Discourse.plugins_sorted_by_name.map(&:name)).to eq(
+        %w[discourse-misery discourse-shining],
+      )
+    end
+
+    it "sorts both enabled and disabled plugins when that option is provided" do
+      expect(Discourse.plugins_sorted_by_name(enabled_only: false).map(&:name)).to eq(
+        %w[discourse-doctor-sleep discourse-misery discourse-shining],
+      )
+    end
+  end
+
   describe "plugins" do
     let(:plugin_class) do
       Class.new(Plugin::Instance) do
diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb
index 463f399b7d3..773e2f769c4 100644
--- a/spec/lib/plugin/instance_spec.rb
+++ b/spec/lib/plugin/instance_spec.rb
@@ -1,10 +1,60 @@
 # frozen_string_literal: true
 
 RSpec.describe Plugin::Instance do
-  subject(:plugin_instance) { described_class.new }
+  subject(:plugin_instance) { described_class.new(metadata) }
+
+  let(:metadata) { Plugin::Metadata.parse <<TEXT }
+# name: discourse-sample-plugin
+# about: about: my plugin
+# version: 0.1
+# authors: Frank Zappa
+# contact emails: frankz@example.com
+# url: http://discourse.org
+# required version: 1.3.0beta6+48
+# meta_topic_id: 1234
+# label: experimental
+
+some_ruby
+TEXT
 
   after { DiscoursePluginRegistry.reset! }
 
+  # NOTE: sample_plugin_site_settings.yml is always loaded in tests in site_setting.rb
+
+  describe ".humanized_name" do
+    before do
+      TranslationOverride.upsert!(
+        "en",
+        "admin_js.admin.site_settings.categories.discourse_sample_plugin",
+        "Sample Plugin Category Name",
+      )
+    end
+
+    it "defaults to using the plugin name with the discourse- prefix removed" do
+      expect(plugin_instance.humanized_name).to eq("sample-plugin")
+    end
+
+    it "uses the plugin setting category name if it exists" do
+      plugin_instance.enabled_site_setting(:discourse_sample_plugin_enabled)
+      expect(plugin_instance.humanized_name).to eq("Sample Plugin Category Name")
+    end
+
+    it "the plugin name the plugin site settings are still under the generic plugins: category" do
+      plugin_instance.stubs(:setting_catgory).returns("plugins")
+      expect(plugin_instance.humanized_name).to eq("sample-plugin")
+    end
+
+    it "removes any Discourse prefix from the setting category name" do
+      TranslationOverride.upsert!(
+        "en",
+        "admin_js.admin.site_settings.categories.discourse_sample_plugin",
+        "Discourse Sample Plugin Category Name",
+      )
+      plugin_instance.enabled_site_setting(:discourse_sample_plugin_enabled)
+      expect(plugin_instance.humanized_name).to eq("Sample Plugin Category Name")
+    end
+  end
+
   describe "find_all" do
     it "can find plugins correctly" do
       plugins = Plugin::Instance.find_all("#{Rails.root}/spec/fixtures/plugins")
diff --git a/spec/support/sample_plugin_site_settings.yml b/spec/support/sample_plugin_site_settings.yml
index 967d8eb209c..98126c778a5 100644
--- a/spec/support/sample_plugin_site_settings.yml
+++ b/spec/support/sample_plugin_site_settings.yml
@@ -1,7 +1,9 @@
+discourse_sample_plugin:
+  discourse_sample_plugin_enabled: true
 site_settings:
   plugin_setting:
     default: "some value"
   test_some_topic_id:
     default: 0
   test_some_other_topic_id:
-    default: 0
\ No newline at end of file
+    default: 0