UX: Change admin plugins list to follow UI guidelines (#28478)

This commit introduces a little bit of duplication
since the old plugin UIs not using the new plugin show
page look different from ones like AI and Gamification
which have been converted. We can use the new admin
header component on the plugins list, but for the other
pages we are manually rendering a breadcrumb trail and
the list of plugin tabs.

Over time as we convert more plugins to use the new UI
guidelines and show page we can get rid of this duplication.

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

@@ -6,6 +6,8 @@ import SiteSetting from "admin/models/site-setting";
 
 export default class AdminPluginsIndexController extends Controller {
   @service session;
+  @service adminPluginNavManager;
+  @service router;
 
   @action
   async togglePluginEnabled(plugin) {
@@ -21,4 +23,31 @@ export default class AdminPluginsIndexController extends Controller {
       popupAjaxError(e);
     }
   }
+
+  // NOTE: See also AdminPluginsController, there is some duplication here
+  // while we convert plugins to use_new_show_route
+  get adminRoutes() {
+    return this.allAdminRoutes.filter((route) => this.routeExists(route));
+  }
+
+  get allAdminRoutes() {
+    return this.model
+      .filter((plugin) => plugin?.enabled && plugin?.adminRoute)
+      .map((plugin) => {
+        return Object.assign(plugin.adminRoute, { plugin_id: plugin.id });
+      });
+  }
+
+  routeExists(route) {
+    try {
+      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/controllers/admin-plugins.js

@@ -13,19 +13,21 @@ export default class AdminPluginsController extends Controller {
     return this.allAdminRoutes.filter((route) => !this.routeExists(route));
   }
 
+  // NOTE: See also AdminPluginsIndexController, there is some duplication here
+  // while we convert plugins to use_new_show_route
   get allAdminRoutes() {
     return this.model
-      .filter((plugin) => plugin?.enabled)
+      .filter((plugin) => plugin?.enabled && plugin?.adminRoute)
       .map((plugin) => {
-        return plugin.adminRoute;
-      })
-      .filter(Boolean);
+        return Object.assign(plugin.adminRoute, { plugin_id: plugin.id });
+      });
   }
 
   get showTopNav() {
     return (
-      !this.adminPluginNavManager.currentPlugin ||
-      this.adminPluginNavManager.isSidebarMode
+      !this.adminPluginNavManager.viewingPluginsList &&
+      (!this.adminPluginNavManager.currentPlugin ||
+        this.adminPluginNavManager.isSidebarMode)
     );
   }
 

app/assets/javascripts/admin/addon/routes/admin-backups-logs.js

@@ -1,8 +1,9 @@
 import EmberObject from "@ember/object";
-import Route from "@ember/routing/route";
 import PreloadStore from "discourse/lib/preload-store";
+import DiscourseRoute from "discourse/routes/discourse";
+import I18n from "discourse-i18n";
 
-export default class AdminBackupsLogsRoute extends Route {
+export default class AdminBackupsLogsRoute extends DiscourseRoute {
   // since the logs are pushed via the message bus
   // we only want to preload them (hence the beforeModel hook)
   beforeModel() {
@@ -25,4 +26,8 @@ export default class AdminBackupsLogsRoute extends Route {
   setupController() {
     /* prevent default behavior */
   }
+
+  titleToken() {
+    return I18n.t("admin.backups.menu.logs");
+  }
 }

app/assets/javascripts/admin/addon/routes/admin-backups-settings.js

@@ -1,11 +1,16 @@
-import Route from "@ember/routing/route";
+import DiscourseRoute from "discourse/routes/discourse";
+import I18n from "discourse-i18n";
 import SiteSetting from "admin/models/site-setting";
 
-export default class AdminBackupsSettingsRoute extends Route {
+export default class AdminBackupsSettingsRoute extends DiscourseRoute {
   queryParams = {
     filter: { replace: true },
   };
 
+  titleToken() {
+    return I18n.t("admin.backups.settings");
+  }
+
   async model(params) {
     return {
       settings: await SiteSetting.findAll({ categories: ["backups"] }),

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

@@ -0,0 +1,14 @@
+import Route from "@ember/routing/route";
+import { service } from "@ember/service";
+
+export default class AdminPluginsIndexRoute extends Route {
+  @service adminPluginNavManager;
+
+  afterModel() {
+    this.adminPluginNavManager.viewingPluginsList = true;
+  }
+
+  deactivate() {
+    this.adminPluginNavManager.viewingPluginsList = false;
+  }
+}

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

@@ -1,12 +1,17 @@
-import Route from "@ember/routing/route";
 import { service } from "@ember/service";
+import DiscourseRoute from "discourse/routes/discourse";
+import I18n from "discourse-i18n";
 import AdminPlugin from "admin/models/admin-plugin";
 
-export default class AdminPluginsRoute extends Route {
+export default class AdminPluginsRoute extends DiscourseRoute {
   @service router;
 
   async model() {
     const plugins = await this.store.findAll("plugin");
     return plugins.map((plugin) => AdminPlugin.create(plugin));
   }
+
+  titleToken() {
+    return I18n.t("admin.plugins.title");
+  }
 }

app/assets/javascripts/admin/addon/services/admin-plugin-nav-manager.js

@@ -10,6 +10,11 @@ export default class AdminPluginNavManager extends Service {
   @service currentUser;
   @tracked currentPlugin;
 
+  // NOTE (martin) This is a temporary solution so we know whether to
+  // show the expanded header / nav on the admin plugin list or not.
+  // This will be removed when all plugins follow the new "show route" pattern.
+  @tracked viewingPluginsList = false;
+
   get currentUserUsingAdminSidebar() {
     return this.currentUser?.use_admin_sidebar;
   }

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

@@ -1,25 +1,52 @@
-<DBreadcrumbsContainer />
+<div class="admin-plugins-list-container">
 
-<DBreadcrumbsItem @path="/admin" @label={{i18n "admin_title"}} />
-<DBreadcrumbsItem
+  <AdminPageHeader
+    @titleLabel="admin.plugins.installed"
+    @descriptionLabel="admin.plugins.description"
+    @learnMoreUrl="https://www.discourse.org/plugins"
+  >
+    <:breadcrumbs>
+      <DBreadcrumbsItem
         @path="/admin/plugins"
         @label={{i18n "admin.plugins.title"}}
-/>
+      />
+    </:breadcrumbs>
+    <:tabs>
+      <NavItem @route="adminPlugins.index" @label="admin.plugins.title" />
+      {{#each this.adminRoutes as |route|}}
+        {{#if route.use_new_show_route}}
+          <NavItem
+            @route={{route.full_location}}
+            @label={{route.label}}
+            @routeParam={{route.location}}
+            @class="admin-plugin-tab-nav-item"
+            data-plugin-nav-tab-id={{route.plugin_id}}
+          />
+        {{else}}
+          <NavItem
+            @route={{route.full_location}}
+            @label={{route.label}}
+            @class="admin-plugin-tab-nav-item"
+            data-plugin-nav-tab-id={{route.plugin_id}}
+          />
+        {{/if}}
+      {{/each}}
+    </:tabs>
+  </AdminPageHeader>
+
+  <div class="alert alert-info -top-margin admin-plugins-howto">
+    {{dIcon "info-circle"}}
+    <a href="https://meta.discourse.org/t/install-a-plugin/19157">
+      {{i18n "admin.plugins.howto"}}
+    </a>
+  </div>
 
-<div class="admin-plugins-list-container">
   {{#if this.model.length}}
-    <h2>{{i18n "admin.plugins.installed"}}</h2>
     <AdminPluginsList @plugins={{this.model}} />
   {{else}}
     <p>{{i18n "admin.plugins.none_installed"}}</p>
   {{/if}}
 
-  <p class="admin-plugins-howto">
-    <a href="https://meta.discourse.org/t/install-a-plugin/19157">
-      {{i18n "admin.plugins.howto"}}
-    </a>
-  </p>
-
   <span>
     <PluginOutlet
       @name="admin-below-plugins-index"

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

@@ -1,5 +1,12 @@
 {{#if this.showTopNav}}
-  <div class="admin-controls">
+  <div class="admin-page-header">
+    <DBreadcrumbsContainer />
+    <DBreadcrumbsItem @path="/admin" @label={{i18n "admin_title"}} />
+    <DBreadcrumbsItem
+      @path="/admin/plugins"
+      @label={{i18n "admin.plugins.title"}}
+    />
+    <div class="admin-nav-submenu">
       <HorizontalOverflowNav class="main-nav nav plugin-nav">
         <NavItem @route="adminPlugins.index" @label="admin.plugins.title" />
         {{#each this.adminRoutes as |route|}}
@@ -8,16 +15,24 @@
               @route={{route.full_location}}
               @label={{route.label}}
               @routeParam={{route.location}}
+              @class="admin-plugin-tab-nav-item"
+              data-plugin-nav-tab-id={{route.plugin_id}}
             />
           {{else}}
-          <NavItem @route={{route.full_location}} @label={{route.label}} />
+            <NavItem
+              @route={{route.full_location}}
+              @label={{route.label}}
+              @class="admin-plugin-tab-nav-item"
+              data-plugin-nav-tab-id={{route.plugin_id}}
+            />
           {{/if}}
         {{/each}}
       </HorizontalOverflowNav>
     </div>
+  </div>
 {{/if}}
 
-<div class="admin-container">
+<div class="admin-container -no-header">
   {{#each this.brokenAdminRoutes as |route|}}
     <div class="alert alert-error">
       {{i18n "admin.plugins.broken_route" name=(i18n route.label)}}

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

@@ -453,6 +453,10 @@ $mobile-breakpoint: 700px;
     margin-bottom: 1em;
   }
 
+  &.-no-header {
+    margin-top: 1em;
+  }
+
   .username {
     input {
       min-width: 15em;

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

@@ -162,6 +162,10 @@
 
 .admin-plugins .admin-container {
   margin-top: 0;
+
+  &.-no-header {
+    margin-top: 1em;
+  }
 }
 
 .admin-plugin-filtered-site-settings {

app/assets/stylesheets/common/base/alert.scss

@@ -36,6 +36,10 @@
       z-index: z("base");
     }
   }
+
+  &.-top-margin {
+    margin-top: 1em;
+  }
 }
 
 a.alert.clickable {

config/locales/client.en.yml

@@ -5648,7 +5648,8 @@ en:
             move_down: "Move down"
       plugins:
         title: "Plugins"
-        installed: "Installed Plugins"
+        installed: "Installed plugins"
+        description: "Any Discourse plugins that you have installed, or plugins that come preinstalled with Discourse hosting, will appear in this list."
         name: "Name"
         none_installed: "You don't have any plugins installed."
         version: "Version"

plugins/automation/spec/system/admin_plugins_list_spec.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+# NOTE: This spec covers core functionality, but it is much easier
+# to test plugin related things inside an actual plugin.
+describe "Admin Plugins List", type: :system, js: true do
+  fab!(:current_user) { Fabricate(:admin) }
+  let(:admin_plugins_list_page) { PageObjects::Pages::AdminPluginsList.new }
+
+  before do
+    sign_in(current_user)
+    SiteSetting.discourse_automation_enabled = true
+  end
+
+  let(:automation_plugin) do
+    Plugin::Instance.parse_from_source(File.join(Rails.root, "plugins", "automation", "plugin.rb"))
+  end
+
+  it "shows the list of plugins" do
+    admin_plugins_list_page.visit
+
+    expect(admin_plugins_list_page.find_plugin("automation")).to have_css(
+      ".admin-plugins-list__name-with-badges .admin-plugins-list__name",
+      text: "Automation",
+    )
+    expect(admin_plugins_list_page.find_plugin("automation")).to have_css(
+      ".admin-plugins-list__author",
+      text: I18n.t("admin_js.admin.plugins.author", { author: "Discourse" }),
+    )
+    expect(admin_plugins_list_page.find_plugin("automation")).to have_css(
+      ".admin-plugins-list__about",
+      text: automation_plugin.metadata.about,
+    )
+  end
+
+  it "can toggle whether a plugin is enabled" do
+    admin_plugins_list_page.visit
+    toggle_switch =
+      PageObjects::Components::DToggleSwitch.new(
+        admin_plugins_list_page.plugin_row_selector("automation") +
+          " .admin-plugins-list__enabled .d-toggle-switch",
+      )
+    toggle_switch.toggle
+    expect(toggle_switch).to be_unchecked
+    expect(SiteSetting.discourse_automation_enabled).to eq(false)
+    toggle_switch.toggle
+    expect(toggle_switch).to be_checked
+    expect(SiteSetting.discourse_automation_enabled).to eq(true)
+  end
+
+  it "shows a navigation tab for each plugin that needs it" do
+    admin_plugins_list_page.visit
+    expect(admin_plugins_list_page).to have_plugin_tab("automation")
+  end
+end

spec/system/admin_plugins_list_spec.rb

@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-describe "Admin Plugins List", type: :system, js: true do
-  fab!(:current_user) { Fabricate(:admin) }
-
-  before do
-    sign_in(current_user)
-    Discourse.stubs(:visible_plugins).returns([spoiler_alert_plugin])
-  end
-
-  let(:spoiler_alert_plugin) do
-    path = File.join(Rails.root, "plugins", "spoiler-alert", "plugin.rb")
-    Plugin::Instance.parse_from_source(path)
-  end
-
-  it "shows the list of plugins" do
-    visit "/admin/plugins"
-
-    plugin_row =
-      find(
-        ".admin-plugins-list tr[data-plugin-name=\"spoiler-alert\"] td.admin-plugins-list__name-details",
-      )
-    expect(plugin_row).to have_css(
-      ".admin-plugins-list__name-with-badges .admin-plugins-list__name",
-      text: "Spoiler Alert",
-    )
-    expect(plugin_row).to have_css(
-      ".admin-plugins-list__author",
-      text: I18n.t("admin_js.admin.plugins.author", { author: "Discourse" }),
-    )
-    expect(plugin_row).to have_css(
-      ".admin-plugins-list__about",
-      text: spoiler_alert_plugin.metadata.about,
-    )
-  end
-end

spec/system/page_objects/components/d_toggle_switch.rb

@@ -17,6 +17,17 @@ module PageObjects
         actionbuilder = page.driver.browser.action # workaround zero height button
         actionbuilder.click(component).perform
       end
+
+      def checked?
+        find(@context).has_css?(".d-toggle-switch__checkbox[aria-checked=\"true\"]", visible: false)
+      end
+
+      def unchecked?
+        find(@context).has_css?(
+          ".d-toggle-switch__checkbox[aria-checked=\"false\"]",
+          visible: false,
+        )
+      end
     end
   end
 end

spec/system/page_objects/pages/admin_plugins_list.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class AdminPluginsList < PageObjects::Pages::Base
+      def visit
+        page.visit("/admin/plugins")
+        self
+      end
+
+      def find_plugin(plugin)
+        find(plugin_row_selector(plugin))
+      end
+
+      def plugin_row_selector(plugin)
+        ".admin-plugins-list .admin-plugins-list__row[data-plugin-name=\"#{plugin}\"]"
+      end
+
+      def has_plugin_tab?(plugin)
+        page.has_css?(plugin_nav_tab_selector(plugin))
+      end
+
+      def plugin_nav_tab_selector(plugin)
+        ".admin-nav-submenu__tabs .admin-plugin-tab-nav-item[data-plugin-nav-tab-id=\"#{plugin}\"]"
+      end
+    end
+  end
+end