From 914f93b896a71a87b07ac269a836ad16fd1fabf2 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 2 May 2024 11:36:46 +1000
Subject: [PATCH] DEV: Add more structure for admin plugin config nav (#26707)

* Simplify config nav link generation to always inject the Settings
  tab
* Auto-redirect to the first non-settings config link (if there is one)
  when the user lands on /admin/plugins/:plugin_id
* Add `extras` to admin plugin serializer so plugins can add more
  data on first load
* Add PikadayCalendar page object for system specs, extracted from the
CalendarDateTimePicker to make it more generic.
---
 .../admin-plugin-config-metadata.gjs          | 22 +++++
 .../components/admin-plugin-config-page.gjs   | 57 ++----------
 .../admin-plugin-config-top-nav.gjs           | 36 ++++++++
 .../admin/addon/models/admin-plugin.js        |  1 +
 .../addon/routes/admin-plugins-show-index.js  | 20 +++++
 .../services/admin-plugin-nav-manager.js      | 39 +++++++-
 .../app/lib/sidebar/admin-sidebar.js          |  2 +-
 .../admin-plugin-config-area-test.js          | 10 +--
 app/serializers/admin_plugin_serializer.rb    |  2 +-
 .../components/calendar_date_time_picker.rb   | 14 +--
 .../components/pikaday_calendar.rb            | 88 +++++++++++++++++++
 11 files changed, 221 insertions(+), 70 deletions(-)
 create mode 100644 app/assets/javascripts/admin/addon/components/admin-plugin-config-metadata.gjs
 create mode 100644 app/assets/javascripts/admin/addon/components/admin-plugin-config-top-nav.gjs
 create mode 100644 app/assets/javascripts/admin/addon/routes/admin-plugins-show-index.js
 create mode 100644 spec/system/page_objects/components/pikaday_calendar.rb

diff --git a/app/assets/javascripts/admin/addon/components/admin-plugin-config-metadata.gjs b/app/assets/javascripts/admin/addon/components/admin-plugin-config-metadata.gjs
new file mode 100644
index 00000000000..c27dcd4d555
--- /dev/null
+++ b/app/assets/javascripts/admin/addon/components/admin-plugin-config-metadata.gjs
@@ -0,0 +1,22 @@
+import i18n from "discourse-common/helpers/i18n";
+
+const AdminPluginConfigMetadata = <template>
+  <div class="admin-plugin-config-page__metadata">
+    <div class="admin-plugin-config-area__metadata-title">
+      <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>
+</template>;
+
+export default AdminPluginConfigMetadata;
diff --git a/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs b/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs
index d40cb9833b1..608d6e12b0c 100644
--- a/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs
+++ b/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs
@@ -1,11 +1,10 @@
 import Component from "@glimmer/component";
 import { inject as service } from "@ember/service";
-import HorizontalOverflowNav from "discourse/components/horizontal-overflow-nav";
-import NavItem from "discourse/components/nav-item";
-import i18n from "discourse-common/helpers/i18n";
 import AdminPluginConfigArea from "./admin-plugin-config-area";
+import AdminPluginConfigMetadata from "./admin-plugin-config-metadata";
+import AdminPluginConfigTopNav from "./admin-plugin-config-top-nav";
 
-export default class extends Component {
+export default class AdminPluginConfigPage extends Component {
   @service currentUser;
   @service adminPluginNavManager;
 
@@ -21,58 +20,14 @@ export default class extends Component {
     return classes.join(" ");
   }
 
-  linkText(navLink) {
-    if (navLink.label) {
-      return i18n(navLink.label);
-    } else {
-      return navLink.text;
-    }
-  }
-
   <template>
     <div class="admin-plugin-config-page">
       {{#if this.adminPluginNavManager.isTopMode}}
-        <div class="admin-controls">
-          <HorizontalOverflowNav
-            class="nav-pills action-list main-nav nav plugin-nav"
-          >
-            {{#each
-              this.adminPluginNavManager.currentConfigNav.links
-              as |navLink|
-            }}
-              <NavItem
-                @route={{navLink.route}}
-                @i18nLabel={{this.linkText navLink}}
-                title={{this.linkText navLink}}
-                class="admin-plugin-config-page__top-nav-item"
-              >
-                {{this.linkText navLink}}
-              </NavItem>
-            {{/each}}
-          </HorizontalOverflowNav>
-        </div>
+        <AdminPluginConfigTopNav />
       {{/if}}
 
-      <div class="admin-plugin-config-page__metadata">
-        <div class="admin-plugin-config-area__metadata-title">
-          <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>
+      <AdminPluginConfigMetadata @plugin={{@plugin}} />
+
       <div class="admin-plugin-config-page__content">
         <div class={{this.mainAreaClasses}}>
           <AdminPluginConfigArea>
diff --git a/app/assets/javascripts/admin/addon/components/admin-plugin-config-top-nav.gjs b/app/assets/javascripts/admin/addon/components/admin-plugin-config-top-nav.gjs
new file mode 100644
index 00000000000..b221be845c1
--- /dev/null
+++ b/app/assets/javascripts/admin/addon/components/admin-plugin-config-top-nav.gjs
@@ -0,0 +1,36 @@
+import Component from "@glimmer/component";
+import { inject as service } from "@ember/service";
+import HorizontalOverflowNav from "discourse/components/horizontal-overflow-nav";
+import NavItem from "discourse/components/nav-item";
+import i18n from "discourse-common/helpers/i18n";
+
+export default class AdminPluginConfigTopNav extends Component {
+  @service adminPluginNavManager;
+
+  linkText(navLink) {
+    if (navLink.label) {
+      return i18n(navLink.label);
+    } else {
+      return navLink.text;
+    }
+  }
+
+  <template>
+    <div class="admin-controls">
+      <HorizontalOverflowNav
+        class="nav-pills action-list main-nav nav plugin-nav"
+      >
+        {{#each this.adminPluginNavManager.currentConfigNav.links as |navLink|}}
+          <NavItem
+            @route={{navLink.route}}
+            @i18nLabel={{this.linkText navLink}}
+            title={{this.linkText navLink}}
+            class="admin-plugin-config-page__top-nav-item"
+          >
+            {{this.linkText navLink}}
+          </NavItem>
+        {{/each}}
+      </HorizontalOverflowNav>
+    </div>
+  </template>
+}
diff --git a/app/assets/javascripts/admin/addon/models/admin-plugin.js b/app/assets/javascripts/admin/addon/models/admin-plugin.js
index 5adfe58abb3..e799509d25b 100644
--- a/app/assets/javascripts/admin/addon/models/admin-plugin.js
+++ b/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.extras = args.extras;
   }
 
   get snakeCaseName() {
diff --git a/app/assets/javascripts/admin/addon/routes/admin-plugins-show-index.js b/app/assets/javascripts/admin/addon/routes/admin-plugins-show-index.js
new file mode 100644
index 00000000000..e3cba45f04f
--- /dev/null
+++ b/app/assets/javascripts/admin/addon/routes/admin-plugins-show-index.js
@@ -0,0 +1,20 @@
+import Route from "@ember/routing/route";
+import { inject as service } from "@ember/service";
+
+export default class AdminPluginsShowIndexRoute extends Route {
+  @service router;
+  @service adminPluginNavManager;
+
+  model() {
+    return this.modelFor("adminPlugins.show");
+  }
+
+  afterModel(model) {
+    if (this.adminPluginNavManager.currentPluginDefaultRoute) {
+      this.router.replaceWith(
+        this.adminPluginNavManager.currentPluginDefaultRoute,
+        model.id
+      );
+    }
+  }
+}
diff --git a/app/assets/javascripts/admin/addon/services/admin-plugin-nav-manager.js b/app/assets/javascripts/admin/addon/services/admin-plugin-nav-manager.js
index 8ca213388b2..e3aa99d8567 100644
--- a/app/assets/javascripts/admin/addon/services/admin-plugin-nav-manager.js
+++ b/app/assets/javascripts/admin/addon/services/admin-plugin-nav-manager.js
@@ -15,7 +15,44 @@ export default class AdminPluginNavManager extends Service {
   }
 
   get currentConfigNav() {
-    return configNavForPlugin(this.currentPlugin.id);
+    const configNav = configNavForPlugin(this.currentPlugin.id);
+    const settingsNav = {
+      mode: PLUGIN_NAV_MODE_TOP,
+      links: [
+        {
+          label: "admin.plugins.change_settings_short",
+          route: "adminPlugins.show.settings",
+        },
+      ],
+    };
+
+    // Not all plugins have a more complex config UI and navigation,
+    // in that case only the settings route will be available.
+    if (!configNav) {
+      return settingsNav;
+    }
+
+    // Automatically inject the settings link.
+    if (
+      !configNav.links.mapBy("route").includes("adminPlugins.show.settings")
+    ) {
+      configNav.links.unshift(settingsNav.links[0]);
+    }
+    return configNav;
+  }
+
+  get currentPluginDefaultRoute() {
+    const currentConfigNavLinks = this.currentConfigNav.links;
+    const linksExceptSettings = currentConfigNavLinks.reject(
+      (link) => link.route === "adminPlugins.show.settings"
+    );
+
+    // Some plugins only have the Settings route, if so it's fine to use it as default.
+    if (linksExceptSettings.length === 0) {
+      return currentConfigNavLinks[0].route;
+    }
+
+    return linksExceptSettings[0].route;
   }
 
   get isSidebarMode() {
diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js b/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js
index 1af4038e18f..408fdfbd127 100644
--- a/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js
+++ b/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js
@@ -229,7 +229,7 @@ function pluginAdminRouteLinks() {
       return {
         name: `admin_plugin_${plugin.admin_route.location}`,
         route: plugin.admin_route.use_new_show_route
-          ? `adminPlugins.show.${plugin.admin_route.location}`
+          ? `adminPlugins.show`
           : `adminPlugins.${plugin.admin_route.location}`,
         routeModels: plugin.admin_route.use_new_show_route
           ? [plugin.admin_route.location]
diff --git a/app/assets/javascripts/discourse/tests/integration/components/admin-plugin-config-area-test.js b/app/assets/javascripts/discourse/tests/integration/components/admin-plugin-config-area-test.js
index 18ec94e2e44..a07b4091bc1 100644
--- a/app/assets/javascripts/discourse/tests/integration/components/admin-plugin-config-area-test.js
+++ b/app/assets/javascripts/discourse/tests/integration/components/admin-plugin-config-area-test.js
@@ -13,7 +13,7 @@ import AdminPlugin from "admin/models/admin-plugin";
 module("Integration | Component | admin-plugin-config-area", function (hooks) {
   setupRenderingTest(hooks);
 
-  test("it renders the plugin config nav and content in the sidebar mode", async function (assert) {
+  test("it renders the plugin config nav and content in the sidebar mode but not along the top", async function (assert) {
     registerAdminPluginConfigNav(
       "discourse-test-plugin",
       PLUGIN_NAV_MODE_SIDEBAR,
@@ -39,8 +39,8 @@ module("Integration | Component | admin-plugin-config-area", function (hooks) {
 
     assert.strictEqual(
       document.querySelectorAll(".admin-plugin-inner-sidebar-nav__item").length,
-      2,
-      "it renders the correct number of nav items"
+      3,
+      "it renders the correct number of sidebar nav items (including always adding a Settings link)"
     );
 
     assert.strictEqual(
@@ -50,7 +50,7 @@ module("Integration | Component | admin-plugin-config-area", function (hooks) {
     );
   });
 
-  test("it does not render the nav items in the sidebar when using top mode", async function (assert) {
+  test("it does not render the nav items in the sidebar when using top mode but it does along the top", async function (assert) {
     registerAdminPluginConfigNav("discourse-test-plugin", PLUGIN_NAV_MODE_TOP, [
       {
         route: "adminPlugins.show.discourse-test-plugin.one",
@@ -73,7 +73,7 @@ module("Integration | Component | admin-plugin-config-area", function (hooks) {
     assert.strictEqual(
       document.querySelectorAll(".admin-plugin-inner-sidebar-nav__item").length,
       0,
-      "it renders the correct number of nav items"
+      "it renders the correct number of sidebar nav items"
     );
 
     assert.strictEqual(
diff --git a/app/serializers/admin_plugin_serializer.rb b/app/serializers/admin_plugin_serializer.rb
index 43023657684..dc487f34506 100644
--- a/app/serializers/admin_plugin_serializer.rb
+++ b/app/serializers/admin_plugin_serializer.rb
@@ -68,7 +68,7 @@ class AdminPluginSerializer < ApplicationSerializer
 
     ret = route.slice(:location, :label)
     if route[:use_new_show_route]
-      ret[:full_location] = "adminPlugins.show.#{ret[:location]}"
+      ret[:full_location] = "adminPlugins.show"
       ret[:use_new_show_route] = true
     else
       ret[:full_location] = "adminPlugins.#{ret[:location]}"
diff --git a/spec/system/page_objects/components/calendar_date_time_picker.rb b/spec/system/page_objects/components/calendar_date_time_picker.rb
index 27b66d7a0b5..c3bbb638589 100644
--- a/spec/system/page_objects/components/calendar_date_time_picker.rb
+++ b/spec/system/page_objects/components/calendar_date_time_picker.rb
@@ -3,25 +3,17 @@
 module PageObjects
   module Components
     class CalendarDateTimePicker < PageObjects::Components::Base
+      delegate :select_day, :select_year, to: :@pikaday_calendar
+
       def initialize(context)
         @context = context
+        @pikaday_calendar = PageObjects::Components::PikadayCalendar.new(context)
       end
 
       def component
         find(@context)
       end
 
-      def select_day(day_number)
-        component.find("button.pika-button.pika-day[data-pika-day='#{day_number}']").click
-      end
-
-      def select_year(year)
-        component
-          .find(".pika-select-year", visible: false)
-          .find("option[value='#{year}']")
-          .select_option
-      end
-
       def fill_time(time)
         component.find(".time-picker").fill_in(with: time)
       end
diff --git a/spec/system/page_objects/components/pikaday_calendar.rb b/spec/system/page_objects/components/pikaday_calendar.rb
new file mode 100644
index 00000000000..3f8f3f1507d
--- /dev/null
+++ b/spec/system/page_objects/components/pikaday_calendar.rb
@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Components
+    class PikadayCalendar < PageObjects::Components::Base
+      attr_reader :context
+
+      def initialize(context)
+        @context = context
+      end
+
+      def component
+        find(@context)
+      end
+
+      def open_calendar
+        component.click
+      end
+
+      def visible_pikaday
+        find(".pika-single:not(.is-hidden)")
+      end
+
+      def hidden?
+        page.has_no_css?(".pika-single:not(.is-hidden)")
+      end
+
+      def select_date(year, month, day)
+        open_calendar
+        select_year(year)
+        select_month(month)
+        select_day(day)
+      end
+
+      def select_day(day_number)
+        find("button.pika-button.pika-day[data-pika-day='#{day_number}']:not(.is-disabled)").click
+      end
+
+      # The month is 0-based. Month name can be provided too.
+      def select_month(month)
+        parsed_month =
+          begin
+            Integer(month)
+          rescue StandardError
+            nil
+          end
+
+        if parsed_month.nil?
+          parsed_month =
+            {
+              "january" => 0,
+              "february" => 1,
+              "march" => 2,
+              "april" => 3,
+              "may" => 4,
+              "june" => 5,
+              "july" => 6,
+              "august" => 7,
+              "september" => 8,
+              "october" => 9,
+              "november" => 10,
+              "december" => 11,
+            }[
+              month.downcase
+            ]
+        end
+
+        # visible: false is here because pikaday sets the controls
+        # to opacity: 0 for some reason.
+        visible_pikaday
+          .find(".pika-select-month", visible: false)
+          .click
+          .find("option[value='#{parsed_month}']")
+          .click
+      end
+
+      def select_year(year)
+        # visible: false is here because pikaday sets the controls
+        # to opacity: 0 for some reason.
+        visible_pikaday
+          .find(".pika-select-year", visible: false)
+          .click
+          .find("option[value='#{year}']")
+          .click
+      end
+    end
+  end
+end