DEV: Single admin plugin page for consistent admin plugin UX (#26024)

This commit adds new plugin show routes (`/admin/plugins/:plugin_id`) as we move
towards every plugin having a consistent UI/landing page.

As part of this, we are introducing a consistent way for plugins
to show an inner sidebar in their config page, via a new plugin
API `register_admin_config_nav_routes`

This accepts an array of links with a label/text, and an
ember route. Once this commit is merged we can start the process
of conforming other plugins to follow this pattern, as well
as supporting a single-page version of this for simpler plugins
that don't require an inner sidebar.

Part of /t/122841 internally

app/assets/javascripts/admin/addon/components/admin-plugin-config-area.gjs

@@ -0,0 +1,42 @@
+import Component from "@glimmer/component";
+import { LinkTo } from "@ember/routing";
+import concatClass from "discourse/helpers/concat-class";
+import I18n from "discourse-i18n";
+
+export default class AdminPluginConfigArea extends Component {
+  linkText(navLink) {
+    if (navLink.label) {
+      return I18n.t(navLink.label);
+    } else {
+      return navLink.text;
+    }
+  }
+
+  <template>
+    {{#if @innerSidebarNavLinks}}
+      <nav class="admin-nav admin-plugin-inner-sidebar-nav pull-left">
+        <ul class="nav nav-stacked">
+          {{#each @innerSidebarNavLinks as |navLink|}}
+            <li
+              class={{concatClass
+                "admin-plugin-inner-sidebar-nav__item"
+                navLink.route
+              }}
+            >
+              <LinkTo
+                @route={{navLink.route}}
+                @model={{navLink.model}}
+                title={{this.linkText navLink}}
+              >
+                {{this.linkText navLink}}
+              </LinkTo>
+            </li>
+          {{/each}}
+        </ul>
+      </nav>
+    {{/if}}
+    <section class="admin-plugin-config-area">
+      {{yield}}
+    </section>
+  </template>
+}

app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs

@@ -0,0 +1,57 @@
+import Component from "@glimmer/component";
+import { inject as service } from "@ember/service";
+import i18n from "discourse-common/helpers/i18n";
+import AdminPluginConfigArea from "./admin-plugin-config-area";
+
+export default class extends Component {
+  @service currentUser;
+
+  get configNavRoutes() {
+    return this.args.plugin.configNavRoutes || [];
+  }
+
+  get mainAreaClasses() {
+    let classes = ["admin-plugin-config-page__main-area"];
+
+    if (this.configNavRoutes.length) {
+      classes.push("-with-inner-sidebar");
+    } else {
+      classes.push("-without-inner-sidebar");
+    }
+
+    return classes.join(" ");
+  }
+
+  <template>
+    <div class="admin-plugin-config-page">
+      <div class="admin-plugin-config-page__metadata">
+        <h2>
+          {{@plugin.nameTitleized}}
+        </h2>
+        <p>
+          {{@plugin.about}}
+          {{#if @plugin.linkUrl}}
+            |
+            <a
+              href={{@plugin.linkUrl}}
+              rel="noopener noreferrer"
+              target="_blank"
+            >
+              {{i18n "admin.plugins.learn_more"}}
+            </a>
+          {{/if}}
+
+        </p>
+      </div>
+      <div class="admin-plugin-config-page__content">
+        <div class={{this.mainAreaClasses}}>
+          <AdminPluginConfigArea
+            @innerSidebarNavLinks={{@plugin.configNavRoutes}}
+          >
+            {{yield}}
+          </AdminPluginConfigArea>
+        </div>
+      </div>
+    </div>
+  </template>
+}

app/assets/javascripts/admin/addon/controllers/admin-plugins.js

@@ -5,15 +5,11 @@ export default class AdminPluginsController extends Controller {
   @service router;
 
   get adminRoutes() {
-    return this.allAdminRoutes.filter((route) =>
+    return this.allAdminRoutes.filter((route) => this.routeExists(route));
-      this.routeExists(route.full_location)
-    );
   }
 
   get brokenAdminRoutes() {
-    return this.allAdminRoutes.filter(
+    return this.allAdminRoutes.filter((route) => !this.routeExists(route));
-      (route) => !this.routeExists(route.full_location)
-    );
   }
 
   get allAdminRoutes() {
@@ -25,9 +21,13 @@ export default class AdminPluginsController extends Controller {
       .filter(Boolean);
   }
 
-  routeExists(routeName) {
+  routeExists(route) {
     try {
-      this.router.urlFor(routeName);
+      if (route.use_new_show_route) {
+        this.router.urlFor(route.full_location, route.location);
+      } else {
+        this.router.urlFor(route.full_location);
+      }
       return true;
     } catch (e) {
       return false;

app/assets/javascripts/admin/addon/models/admin-plugin.js

@@ -26,6 +26,7 @@ export default class AdminPlugin {
     this.version = args.version;
     this.metaUrl = args.meta_url;
     this.authors = args.authors;
+    this.configNavRoutes = args.admin_config_nav_routes;
   }
 
   get snakeCaseName() {

app/assets/javascripts/admin/addon/routes/admin-plugins-show.js

@@ -0,0 +1,16 @@
+import Route from "@ember/routing/route";
+import { inject as service } from "@ember/service";
+import { ajax } from "discourse/lib/ajax";
+import { sanitize } from "discourse/lib/text";
+import AdminPlugin from "admin/models/admin-plugin";
+
+export default class AdminPluginsShowRoute extends Route {
+  @service router;
+
+  model(params) {
+    const pluginId = sanitize(params.plugin_id).substring(0, 100);
+    return ajax(`/admin/plugins/${pluginId}.json`).then((plugin) => {
+      return AdminPlugin.create(plugin);
+    });
+  }
+}

app/assets/javascripts/admin/addon/routes/admin-route-map.js

@@ -218,6 +218,9 @@ export default function () {
       { path: "/plugins", resetNamespace: true },
       function () {
         this.route("index", { path: "/" });
+        this.route("show", { path: "/:plugin_id" }, function () {
+          this.route("settings");
+        });
       }
     );
   });

app/assets/javascripts/admin/addon/templates/plugins-show-settings.hbs

@@ -0,0 +1 @@
+<div class="content-body admin-plugin-config-area__settings"></div>

app/assets/javascripts/admin/addon/templates/plugins-show.hbs

@@ -0,0 +1,3 @@
+<AdminPluginConfigPage @plugin={{this.model}}>
+  {{outlet}}
+</AdminPluginConfigPage>

app/assets/javascripts/admin/addon/templates/plugins.hbs

@@ -2,7 +2,15 @@
   <HorizontalOverflowNav class="main-nav nav plugin-nav">
     <NavItem @route="adminPlugins.index" @label="admin.plugins.title" />
     {{#each this.adminRoutes as |route|}}
-      <NavItem @route={{route.full_location}} @label={{route.label}} />
+      {{#if route.use_new_show_route}}
+        <NavItem
+          @route={{route.full_location}}
+          @label={{route.label}}
+          @routeParam={{route.location}}
+        />
+      {{else}}
+        <NavItem @route={{route.full_location}} @label={{route.label}} />
+      {{/if}}
     {{/each}}
   </HorizontalOverflowNav>
 </div>

app/assets/javascripts/discourse/app/components/back-button.gjs

@@ -0,0 +1,10 @@
+import { LinkTo } from "@ember/routing";
+import dIcon from "discourse-common/helpers/d-icon";
+import i18n from "discourse-common/helpers/i18n";
+
+<template>
+  <LinkTo class="btn btn-flat back-button" @route={{@route}}>
+    {{dIcon "chevron-left"}}
+    {{i18n "back_button"}}
+  </LinkTo>
+</template>

app/assets/javascripts/discourse/app/components/nav-item.gjs

@@ -24,7 +24,21 @@ export default class NavItem extends Component {
       return;
     }
 
-    if (this.args.routeParam && this.router.currentRoute) {
+    // This is needed because the setting route is underneath /admin/plugins/:plugin_id,
+    // but is not a child route of the plugin routes themselves. E.g. discourse-ai
+    // for the plugin ID has its own nested routes defined in the plugin.
+    if (this.router.currentRoute.name === "adminPlugins.show.settings") {
+      return (
+        this.router.currentRoute.parent.params.plugin_id ===
+        this.args.routeParam
+      );
+    }
+
+    if (
+      this.args.routeParam &&
+      this.router.currentRoute &&
+      this.router.currentRoute.params.filter
+    ) {
       return this.router.currentRoute.params.filter === this.args.routeParam;
     }
 
@@ -37,6 +51,7 @@ export default class NavItem extends Component {
         <LinkTo
           @route={{@route}}
           @model={{@routeParam}}
+          @current-when={{this.active}}
         >{{this.contents}}</LinkTo>
       {{else if @route}}
         <LinkTo @route={{@route}}>{{this.contents}}</LinkTo>

app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js

@@ -16,8 +16,9 @@ export function clearAdditionalAdminSidebarSectionLinks() {
 }
 
 class SidebarAdminSectionLink extends BaseCustomSidebarSectionLink {
-  constructor({ adminSidebarNavLink }) {
+  constructor({ adminSidebarNavLink, router }) {
     super(...arguments);
+    this.router = router;
     this.adminSidebarNavLink = adminSidebarNavLink;
   }
 
@@ -62,9 +63,26 @@ class SidebarAdminSectionLink extends BaseCustomSidebarSectionLink {
   get title() {
     return this.adminSidebarNavLink.text;
   }
+
+  get currentWhen() {
+    // This is needed because the setting route is underneath /admin/plugins/:plugin_id,
+    // but is not a child route of the plugin routes themselves. E.g. discourse-ai
+    // for the plugin ID has its own nested routes defined in the plugin.
+    if (this.router.currentRoute.name === "adminPlugins.show.settings") {
+      if (
+        this.adminSidebarNavLink.route?.includes(
+          this.router.currentRoute.parent.params.plugin_id
+        )
+      ) {
+        return this.router.currentRoute.name;
+      }
+    }
+
+    return this.adminSidebarNavLink.route;
+  }
 }
 
-function defineAdminSection(adminNavSectionData) {
+function defineAdminSection(adminNavSectionData, router) {
   const AdminNavSection = class extends BaseCustomSidebarSection {
     constructor() {
       super(...arguments);
@@ -95,6 +113,7 @@ function defineAdminSection(adminNavSectionData) {
         (sectionLinkData) =>
           new SidebarAdminSectionLink({
             adminSidebarNavLink: sectionLinkData,
+            router,
           })
       );
     }
@@ -183,7 +202,12 @@ function pluginAdminRouteLinks() {
     (pluginAdminRoute) => {
       return {
         name: `admin_plugin_${pluginAdminRoute.location}`,
-        route: `adminPlugins.${pluginAdminRoute.location}`,
+        route: pluginAdminRoute.use_new_show_route
+          ? `adminPlugins.show.${pluginAdminRoute.location}`
+          : `adminPlugins.${pluginAdminRoute.location}`,
+        routeModels: pluginAdminRoute.use_new_show_route
+          ? [pluginAdminRoute.location]
+          : [],
         label: pluginAdminRoute.label,
         icon: "cog",
       };
@@ -203,6 +227,7 @@ export default class AdminSidebarPanel extends BaseCustomSidebarPanel {
     const siteSettings = getOwnerWithFallback(this).lookup(
       "service:site-settings"
     );
+    const router = getOwnerWithFallback(this).lookup("service:router");
     const session = getOwnerWithFallback(this).lookup("service:session");
     if (!currentUser.use_admin_sidebar) {
       return [];
@@ -231,7 +256,7 @@ export default class AdminSidebarPanel extends BaseCustomSidebarPanel {
     const navConfig = useAdminNavConfig(navMap);
 
     return navConfig.map((adminNavSectionData) => {
-      return defineAdminSection(adminNavSectionData);
+      return defineAdminSection(adminNavSectionData, router);
     });
   }
 

app/assets/javascripts/discourse/tests/integration/components/admin-plugin-config-area-test.js

@@ -0,0 +1,39 @@
+import { render } from "@ember/test-helpers";
+import { hbs } from "ember-cli-htmlbars";
+import { module, test } from "qunit";
+import { setupRenderingTest } from "discourse/tests/helpers/component-test";
+
+module("Integration | Component | admin-plugin-config-area", function (hooks) {
+  setupRenderingTest(hooks);
+
+  test("it renders the plugin config nav and content", async function (assert) {
+    this.set("innerSidebarNavLinks", [
+      {
+        route: "adminPlugins.show.discourse-test-plugin.one",
+        label: "admin.title",
+      },
+      {
+        route: "adminPlugins.show.discourse-test-plugin.two",
+        label: "admin.back_to_forum",
+      },
+    ]);
+
+    await render(hbs`
+      <AdminPluginConfigArea @innerSidebarNavLinks={{this.innerSidebarNavLinks}}>
+        Test content
+      </AdminPluginConfigArea>
+    `);
+
+    assert.strictEqual(
+      document.querySelectorAll(".admin-plugin-inner-sidebar-nav__item").length,
+      2,
+      "it renders the correct number of nav items"
+    );
+
+    assert.strictEqual(
+      document.querySelector(".admin-plugin-config-area").textContent.trim(),
+      "Test content",
+      "it renders the yielded content"
+    );
+  });
+});

app/assets/stylesheets/common/admin/plugins.scss

@@ -78,3 +78,19 @@
     }
   }
 }
+
+.admin-plugin-config-page {
+  &__main-area {
+    .admin-detail {
+      padding-top: 15px;
+    }
+
+    &.-without-inner-sidebar {
+      .admin-detail {
+        border-left: 0;
+        padding-left: 0;
+        width: 100%;
+      }
+    }
+  }
+}

app/assets/stylesheets/common/components/buttons.scss

@@ -374,6 +374,9 @@
       color: var(--primary);
     }
   }
+  &.back-button {
+    margin-bottom: 1em;
+  }
 }
 
 .btn-link {

app/controllers/admin/plugins_controller.rb

@@ -8,4 +8,16 @@ class Admin::PluginsController < Admin::StaffController
       root: "plugins",
     )
   end
+
+  def show
+    plugin = Discourse.plugins_by_name[params[:plugin_id]]
+
+    # An escape hatch in case a plugin is using an un-prefixed
+    # version of their plugin name for a route.
+    plugin = Discourse.plugins_by_name["discourse-#{params[:plugin_id]}"] if !plugin
+
+    raise Discourse::NotFound if !plugin
+
+    render_serialized(plugin, AdminPluginSerializer, root: nil)
+  end
 end

app/serializers/admin_plugin_serializer.rb

@@ -16,7 +16,12 @@ class AdminPluginSerializer < ApplicationSerializer
              :commit_hash,
              :commit_url,
              :meta_url,
-             :authors
+             :authors,
+             :admin_config_nav_routes
+
+  def admin_config_nav_routes
+    object.admin_config_nav_routes
+  end
 
   def id
     object.directory_name
@@ -67,7 +72,12 @@ class AdminPluginSerializer < ApplicationSerializer
     return unless route
 
     ret = route.slice(:location, :label)
-    ret[:full_location] = "adminPlugins.#{ret[:location]}"
+    if route[:use_new_show_route]
+      ret[:full_location] = "adminPlugins.show.#{ret[:location]}"
+      ret[:use_new_show_route] = true
+    else
+      ret[:full_location] = "adminPlugins.#{ret[:location]}"
+    end
     ret
   end
 

config/locales/client.en.yml

@@ -213,6 +213,7 @@ en:
       dismiss: "Dismiss"
 
     bootstrap_mode: "Getting started"
+    back_button: "Back"
 
     themes:
       default_description: "Default"

config/routes.rb

@@ -110,6 +110,8 @@ Discourse::Application.routes.draw do
       get "" => "admin#index"
 
       get "plugins" => "plugins#index"
+      get "plugins/:plugin_id" => "plugins#show"
+      get "plugins/:plugin_id/settings" => "plugins#show"
 
       resources :site_settings, only: %i[index update], constraints: AdminConstraint.new do
         collection { get "category/:id" => "site_settings#index" }

lib/plugin/instance.rb

@@ -45,6 +45,7 @@ end
 class Plugin::Instance
   attr_accessor :path, :metadata
   attr_reader :admin_route
+  attr_reader :admin_config_nav_routes
 
   # Memoized array readers
   %i[
@@ -105,8 +106,25 @@ class Plugin::Instance
     Middleware::AnonymousCache.compile_key_builder
   end
 
-  def add_admin_route(label, location)
+  def add_admin_route(label, location, opts = {})
-    @admin_route = { label: label, location: location }
+    @admin_route = {
+      label: label,
+      location: location,
+      use_new_show_route: opts.fetch(:use_new_show_route, false),
+    }
+  end
+
+  def register_admin_config_nav_routes(plugin_id, nav)
+    @admin_config_nav_routes =
+      nav.each do |n|
+        if !n.key?(:label) || !n.key?(:route)
+          raise ArgumentError.new(
+                  "Admin config nav routes must have a `route` value that matches an Ember route and a `label` value that matches a client I18n key",
+                )
+        end
+
+        n[:model] = plugin_id
+      end
   end
 
   def configurable?

plugins/chat/plugin.rb

@@ -132,6 +132,16 @@ after_initialize do
     end
   end
 
+  add_to_serializer(
+    :admin_plugin,
+    :incoming_chat_webhooks,
+    include_condition: -> { self.name == "chat" },
+  ) { Chat::IncomingWebhook.includes(:chat_channel).all }
+
+  add_to_serializer(:admin_plugin, :chat_channels, include_condition: -> { self.name == "chat" }) do
+    Chat::Channel.public_channels
+  end
+
   add_to_serializer(:user_card, :can_chat_user) do
     return false if !SiteSetting.chat_enabled
     return false if scope.user.blank?

plugins/chat/spec/plugin_spec.rb

@@ -125,6 +125,24 @@ describe Chat do
     end
   end
 
+  describe "admin plugin serializer extension" do
+    let(:admin) { Fabricate(:admin) }
+    let(:chat_plugin) do
+      Plugin::Instance.parse_from_source(File.join(Rails.root, "plugins", "chat", "plugin.rb"))
+    end
+    let(:serializer) { AdminPluginSerializer.new(chat_plugin, scope: admin.guardian) }
+
+    it "includes all incoming webhooks via :incoming_chat_webhooks" do
+      webhook = Fabricate(:incoming_chat_webhook)
+      expect(serializer.incoming_chat_webhooks).to contain_exactly(webhook)
+    end
+
+    it "includes all chat channels via :chat_channels" do
+      channel = Fabricate(:chat_channel)
+      expect(serializer.chat_channels).to contain_exactly(channel)
+    end
+  end
+
   describe "chat oneboxes" do
     fab!(:chat_channel) { Fabricate(:category_channel) }
     fab!(:user)

plugins/chat/spec/requests/application_controller_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe ApplicationController do
       sign_in(admin)
       get "/latest"
       expect(JSON.parse(preloaded_json["enabledPluginAdminRoutes"])).to include(
-        { "label" => "chat.admin.title", "location" => "chat" },
+        { "label" => "chat.admin.title", "location" => "chat", "use_new_show_route" => false },
       )
     end
   end

plugins/discourse-details/assets/stylesheets/details.scss

@@ -2,7 +2,8 @@ details {
   position: relative;
 
   .topic-body .cooked &,
-  .d-editor-preview & {
+  .d-editor-preview,
+  &.details__boxed {
     background-color: var(--primary-very-low);
     padding: 0.25rem 0.75rem;
     margin-bottom: 0.5rem;
@@ -23,6 +24,12 @@ details {
   }
 }
 
+details.details__boxed {
+  summary {
+    font-weight: bold;
+  }
+}
+
 details > *,
 details .lightbox-wrapper {
   display: none;

spec/lib/plugin/instance_spec.rb

@@ -999,4 +999,30 @@ TEXT
       expect(sum).to eq(3)
     end
   end
+
+  describe "#register_admin_config_nav_routes" do
+    let(:plugin) { Plugin::Instance.new }
+
+    it "adds the specified plugin id as the 'model' for the route" do
+      plugin.register_admin_config_nav_routes(
+        "discourse-awesome",
+        [{ route: "adminPlugins.show", label: "some.i18n.label" }],
+      )
+      expect(plugin.admin_config_nav_routes).to eq(
+        [{ route: "adminPlugins.show", label: "some.i18n.label", model: "discourse-awesome" }],
+      )
+    end
+
+    it "errors if the route or label is not provided" do
+      expect {
+        plugin.register_admin_config_nav_routes("discourse-awesome", [{ label: "some.i18n.label" }])
+      }.to raise_error(ArgumentError)
+      expect {
+        plugin.register_admin_config_nav_routes(
+          "discourse-awesome",
+          [{ route: "adminPlugins.show" }],
+        )
+      }.to raise_error(ArgumentError)
+    end
+  end
 end

spec/requests/admin/plugins_controller_spec.rb

@@ -39,4 +39,66 @@ RSpec.describe Admin::PluginsController do
       end
     end
   end
+
+  describe "#show" do
+    before do
+      spoiler_alert =
+        Plugin::Instance.parse_from_source(
+          File.join(Rails.root, "plugins", "spoiler-alert", "plugin.rb"),
+        )
+      poll =
+        Plugin::Instance.parse_from_source(File.join(Rails.root, "plugins", "poll", "plugin.rb"))
+
+      Discourse.stubs(:plugins_by_name).returns(
+        { "discourse-spoiler-alert" => spoiler_alert, "poll" => poll },
+      )
+    end
+
+    context "while logged in as an admin" do
+      before { sign_in(admin) }
+
+      it "returns a plugin" do
+        get "/admin/plugins/poll.json"
+
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["name"]).to eq("poll")
+      end
+
+      it "returns a plugin with the discourse- prefix if the prefixless version is queried" do
+        get "/admin/plugins/spoiler-alert.json"
+
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["name"]).to eq("spoiler-alert")
+      end
+
+      it "404s if the plugin is not found" do
+        get "/admin/plugins/casino.json"
+
+        expect(response.status).to eq(404)
+        expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
+      end
+    end
+
+    context "when logged in as a moderator" do
+      before { sign_in(moderator) }
+
+      it "returns plugins" do
+        get "/admin/plugins/poll.json"
+
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["name"]).to eq("poll")
+      end
+    end
+
+    context "when logged in as a non-staff user" do
+      before { sign_in(user) }
+
+      it "denies access with a 404 response" do
+        get "/admin/plugins/poll.json"
+
+        expect(response.status).to eq(404)
+        expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
+      end
+    end
+  end
 end