From 782f43cc551e38042392f83a29be970905cb1536 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Mon, 7 Nov 2022 16:39:27 +0000
Subject: [PATCH] Improve route error handling in admin/plugins (#18911)

Previously if a specific plugin route was not available (e.g. there was an error loading the plugin's JS due to an ad blocker), the entire page would fail to load. This commit updates the behavior to catch this kind of issue and display a user-friendly message at the top of the screen.
---
 .../admin/addon/controllers/admin-plugins.js  | 33 +++++++++---
 .../admin/addon/templates/plugins.hbs         |  6 +++
 .../tests/acceptance/admin-plugins-test.js    | 51 +++++++++++++++++++
 app/assets/stylesheets/testem.scss            |  1 +
 config/locales/client.en.yml                  |  1 +
 5 files changed, 85 insertions(+), 7 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/tests/acceptance/admin-plugins-test.js

diff --git a/app/assets/javascripts/admin/addon/controllers/admin-plugins.js b/app/assets/javascripts/admin/addon/controllers/admin-plugins.js
index be663cadb15..34ffe45783d 100644
--- a/app/assets/javascripts/admin/addon/controllers/admin-plugins.js
+++ b/app/assets/javascripts/admin/addon/controllers/admin-plugins.js
@@ -1,17 +1,27 @@
 import { action } from "@ember/object";
 import Controller from "@ember/controller";
-import discourseComputed from "discourse-common/utils/decorators";
+import { inject as service } from "@ember/service";
 
 export default Controller.extend({
-  @discourseComputed
-  adminRoutes() {
+  router: service(),
+
+  get adminRoutes() {
+    return this.allAdminRoutes.filter((r) => this.routeExists(r.full_location));
+  },
+
+  get brokenAdminRoutes() {
+    return this.allAdminRoutes.filter(
+      (r) => !this.routeExists(r.full_location)
+    );
+  },
+
+  get allAdminRoutes() {
     return this.model
+      .filter((p) => p?.enabled)
       .map((p) => {
-        if (p.get("enabled")) {
-          return p.admin_route;
-        }
+        return p.admin_route;
       })
-      .compact();
+      .filter(Boolean);
   },
 
   @action
@@ -21,4 +31,13 @@ export default Controller.extend({
       adminDetail.classList.toggle(state);
     });
   },
+
+  routeExists(routeName) {
+    try {
+      this.router.urlFor(routeName);
+      return true;
+    } catch (e) {
+      return false;
+    }
+  },
 });
diff --git a/app/assets/javascripts/admin/addon/templates/plugins.hbs b/app/assets/javascripts/admin/addon/templates/plugins.hbs
index 1425cf479ef..1baaaddd2d7 100644
--- a/app/assets/javascripts/admin/addon/templates/plugins.hbs
+++ b/app/assets/javascripts/admin/addon/templates/plugins.hbs
@@ -19,5 +19,11 @@
 </div>
 
 <div class="admin-detail pull-left mobile-closed">
+  {{#each this.brokenAdminRoutes as |route|}}
+    <div class="alert alert-error">
+      {{i18n "admin.plugins.broken_route" name=(i18n route.label)}}
+    </div>
+  {{/each}}
+
   {{outlet}}
 </div>
diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-plugins-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-plugins-test.js
new file mode 100644
index 00000000000..61ca9fafc3c
--- /dev/null
+++ b/app/assets/javascripts/discourse/tests/acceptance/admin-plugins-test.js
@@ -0,0 +1,51 @@
+import {
+  acceptance,
+  exists,
+  query,
+} from "discourse/tests/helpers/qunit-helpers";
+import { visit } from "@ember/test-helpers";
+import { test } from "qunit";
+
+acceptance("Admin - Plugins", function (needs) {
+  needs.user();
+
+  needs.pretender((server, helper) => {
+    server.get("/admin/plugins", () =>
+      helper.response({
+        plugins: [
+          {
+            id: "some-test-plugin",
+            name: "some-test-plugin",
+            about: "Plugin description",
+            version: "0.1",
+            url: "https://example.com",
+            admin_route: {
+              location: "testlocation",
+              label: "test.plugin.label",
+              full_location: "adminPlugins.testlocation",
+            },
+            enabled: true,
+            enabled_setting: "testplugin_enabled",
+            has_settings: true,
+            is_official: true,
+          },
+        ],
+      })
+    );
+  });
+
+  test("shows plugin list", async function (assert) {
+    await visit("/admin/plugins");
+    const table = query("table.admin-plugins");
+    assert.strictEqual(
+      table.querySelector("tr .plugin-name .name").innerText,
+      "some-test-plugin",
+      "displays the plugin in the table"
+    );
+
+    assert.true(
+      exists(".admin-plugins .admin-detail .alert-error"),
+      "displays an error for unknown routes"
+    );
+  });
+});
diff --git a/app/assets/stylesheets/testem.scss b/app/assets/stylesheets/testem.scss
index cb117a88252..a1e4c5dad37 100644
--- a/app/assets/stylesheets/testem.scss
+++ b/app/assets/stylesheets/testem.scss
@@ -19,6 +19,7 @@ $love: #fa6c8d !default;
 @import "common/foundation/mixins";
 @import "desktop";
 @import "color_definitions";
+@import "admin";
 
 #ember-testing-container {
   box-sizing: border-box;
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index fb29c37f874..0cc78869985 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -4639,6 +4639,7 @@ en:
         change_settings_short: "Settings"
         howto: "How do I install plugins?"
         official: "Official Plugin"
+        broken_route: "Unable to configure link to '%{name}'. Ensure ad-blockers are disabled and try reloading the page."
 
       backups:
         title: "Backups"