From be33363f131947ad9e92965fa2d31c859ebe6966 Mon Sep 17 00:00:00 2001
From: Ted Johansson <ted@discourse.org>
Date: Tue, 17 Sep 2024 14:43:34 +0800
Subject: [PATCH] FEATURE: Add ability to dismiss admin notices (#28916)

his is a new feature that lets admins dismiss notices from the dashboard. This helps with self-service in cases where a notice is "stuck", while we work on provisions to prevent "sticking" in the first place.
---
 .../admin/addon/components/admin-notice.gjs   | 25 ++++++++
 .../addon/components/dashboard-problems.gjs   | 63 ++++++++++---------
 .../addon/controllers/admin-dashboard.js      | 21 +------
 .../admin/addon/templates/dashboard.hbs       |  4 +-
 .../tests/acceptance/dashboard-test.js        |  8 ---
 .../stylesheets/common/admin/dashboard.scss   | 15 +++--
 .../admin/admin_notices_controller.rb         | 10 +++
 app/models/problem_check_tracker.rb           |  7 ++-
 app/serializers/admin_notice_serializer.rb    |  2 +-
 app/services/admin_notices/dismiss.rb         | 32 ++++++++++
 config/locales/client.en.yml                  |  1 +
 config/routes.rb                              |  2 +
 spec/fabricators/admin_notice_fabricator.rb   |  1 +
 spec/models/problem_check_tracker_spec.rb     | 26 ++++++++
 spec/services/admin_notices/dismiss_spec.rb   | 32 ++++++++++
 spec/system/admin_notices_spec.rb             | 25 ++++++++
 .../page_objects/pages/admin_dashboard.rb     | 24 +++++++
 17 files changed, 231 insertions(+), 67 deletions(-)
 create mode 100644 app/assets/javascripts/admin/addon/components/admin-notice.gjs
 create mode 100644 app/controllers/admin/admin_notices_controller.rb
 create mode 100644 app/services/admin_notices/dismiss.rb
 create mode 100644 spec/services/admin_notices/dismiss_spec.rb
 create mode 100644 spec/system/admin_notices_spec.rb
 create mode 100644 spec/system/page_objects/pages/admin_dashboard.rb

diff --git a/app/assets/javascripts/admin/addon/components/admin-notice.gjs b/app/assets/javascripts/admin/addon/components/admin-notice.gjs
new file mode 100644
index 00000000000..07138307c7d
--- /dev/null
+++ b/app/assets/javascripts/admin/addon/components/admin-notice.gjs
@@ -0,0 +1,25 @@
+import Component from "@glimmer/component";
+import { action } from "@ember/object";
+import { htmlSafe } from "@ember/template";
+import DButton from "discourse/components/d-button";
+import icon from "discourse-common/helpers/d-icon";
+
+export default class AdminNotice extends Component {
+  @action
+  dismiss() {
+    this.args.dismissCallback(this.args.problem);
+  }
+
+  <template>
+    <div class="notice">
+      <div class="message">
+        {{if @icon (icon @icon)}}
+        {{htmlSafe @problem.message}}
+      </div>
+      <DButton
+        @action={{this.dismiss}}
+        @label="admin.dashboard.dismiss_notice"
+      />
+    </div>
+  </template>
+}
diff --git a/app/assets/javascripts/admin/addon/components/dashboard-problems.gjs b/app/assets/javascripts/admin/addon/components/dashboard-problems.gjs
index 7f34df878b4..882d65501d6 100644
--- a/app/assets/javascripts/admin/addon/components/dashboard-problems.gjs
+++ b/app/assets/javascripts/admin/addon/components/dashboard-problems.gjs
@@ -1,15 +1,33 @@
 import Component from "@glimmer/component";
-import { htmlSafe } from "@ember/template";
+import { concat } from "@ember/helper";
+import { action } from "@ember/object";
+import { eq } from "truth-helpers";
 import ConditionalLoadingSection from "discourse/components/conditional-loading-section";
 import DButton from "discourse/components/d-button";
 import concatClass from "discourse/helpers/concat-class";
+import { ajax } from "discourse/lib/ajax";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 import icon from "discourse-common/helpers/d-icon";
 import i18n from "discourse-common/helpers/i18n";
+import AdminNotice from "admin/components/admin-notice";
 
-// eslint-disable-next-line ember/no-empty-glimmer-component-classes
 export default class DashboardProblems extends Component {
+  @action
+  async dismissProblem(problem) {
+    try {
+      await ajax(`/admin/admin_notices/${problem.id}`, { type: "DELETE" });
+      this.args.problems.removeObject(problem);
+    } catch (error) {
+      popupAjaxError(error);
+    }
+  }
+
+  get problems() {
+    return this.args.problems.sortBy("priority");
+  }
+
   <template>
-    {{#if @foundProblems}}
+    {{#if @problems.length}}
       <div class="section dashboard-problems">
         <div class="section-title">
           <h2>
@@ -20,35 +38,24 @@ export default class DashboardProblems extends Component {
 
         <div class="section-body">
           <ConditionalLoadingSection @isLoading={{@loadingProblems}}>
-            {{#if @highPriorityProblems.length}}
-              <div class="problem-messages priority-high">
-                <ul>
-                  {{#each @highPriorityProblems as |problem|}}
-                    <li
-                      class={{concatClass
-                        "dashboard-problem "
-                        "priority-"
-                        problem.priority
-                      }}
-                    >
-                      {{icon "triangle-exclamation"}}
-                      {{htmlSafe problem.message}}
-                    </li>
-                  {{/each}}
-                </ul>
-              </div>
-            {{/if}}
-
-            <div class="problem-messages priority-low">
+            <div class="problem-messages">
               <ul>
-                {{#each @lowPriorityProblems as |problem|}}
+                {{#each this.problems as |problem|}}
                   <li
                     class={{concatClass
-                      "dashboard-problem "
-                      "priority-"
-                      problem.priority
+                      "dashboard-problem"
+                      (concat "priority-" problem.priority)
                     }}
-                  >{{htmlSafe problem.message}}</li>
+                  >
+                    <AdminNotice
+                      @icon={{if
+                        (eq problem.priority "high")
+                        "triangle-exclamation"
+                      }}
+                      @problem={{problem}}
+                      @dismissCallback={{this.dismissProblem}}
+                    />
+                  </li>
                 {{/each}}
               </ul>
             </div>
diff --git a/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js b/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js
index 89a6a52ad19..8f60607eba4 100644
--- a/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js
+++ b/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js
@@ -18,16 +18,6 @@ export default class AdminDashboardController extends Controller {
 
   @setting("version_checks") showVersionChecks;
 
-  @discourseComputed(
-    "lowPriorityProblems.length",
-    "highPriorityProblems.length"
-  )
-  foundProblems(lowPriorityProblemsLength, highPriorityProblemsLength) {
-    const problemsLength =
-      lowPriorityProblemsLength + highPriorityProblemsLength;
-    return this.currentUser.admin && problemsLength > 0;
-  }
-
   @computed("siteSettings.dashboard_visible_tabs")
   get visibleTabs() {
     return (this.siteSettings.dashboard_visible_tabs || "")
@@ -106,16 +96,7 @@ export default class AdminDashboardController extends Controller {
     });
 
     AdminDashboard.fetchProblems()
-      .then((model) => {
-        this.set(
-          "highPriorityProblems",
-          model.problems.filterBy("priority", "high")
-        );
-        this.set(
-          "lowPriorityProblems",
-          model.problems.filterBy("priority", "low")
-        );
-      })
+      .then((model) => this.set("problems", model.problems))
       .finally(() => this.set("loadingProblems", false));
   }
 
diff --git a/app/assets/javascripts/admin/addon/templates/dashboard.hbs b/app/assets/javascripts/admin/addon/templates/dashboard.hbs
index 2d0e81ff674..f5a07025ee1 100644
--- a/app/assets/javascripts/admin/addon/templates/dashboard.hbs
+++ b/app/assets/javascripts/admin/addon/templates/dashboard.hbs
@@ -18,9 +18,7 @@
 
 <DashboardProblems
   @loadingProblems={{this.loadingProblems}}
-  @foundProblems={{this.foundProblems}}
-  @lowPriorityProblems={{this.lowPriorityProblems}}
-  @highPriorityProblems={{this.highPriorityProblems}}
+  @problems={{this.problems}}
   @problemsTimestamp={{this.problemsTimestamp}}
   @refreshProblems={{action "refreshProblems"}}
 />
diff --git a/app/assets/javascripts/discourse/tests/acceptance/dashboard-test.js b/app/assets/javascripts/discourse/tests/acceptance/dashboard-test.js
index 270acddc74c..e494aa7f85b 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/dashboard-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/dashboard-test.js
@@ -4,7 +4,6 @@ import {
   acceptance,
   count,
   exists,
-  query,
 } from "discourse/tests/helpers/qunit-helpers";
 import selectKit from "discourse/tests/helpers/select-kit-helper";
 
@@ -60,13 +59,6 @@ acceptance("Dashboard", function (needs) {
       exists(".admin-report.new-contributors"),
       "new-contributors report"
     );
-    assert.strictEqual(
-      query(
-        ".section.dashboard-problems .problem-messages ul li:first-child"
-      ).innerHTML.trim(),
-      "Houston...",
-      "displays problems"
-    );
   });
 
   test("moderation tab", async function (assert) {
diff --git a/app/assets/stylesheets/common/admin/dashboard.scss b/app/assets/stylesheets/common/admin/dashboard.scss
index cc3130d08f6..1b61bd84d1e 100644
--- a/app/assets/stylesheets/common/admin/dashboard.scss
+++ b/app/assets/stylesheets/common/admin/dashboard.scss
@@ -246,16 +246,21 @@
     .problem-messages {
       margin-bottom: 1em;
 
-      &.priority-high {
-        background-color: var(--danger-low);
-        border: 1px solid var(--danger-medium);
-      }
-
       ul {
         margin: 0 0 0 1.25em;
 
         li.dashboard-problem {
           padding: 0.5em 0.5em;
+
+          .notice {
+            display: flex;
+            justify-content: space-between;
+            align-items: center;
+          }
+
+          .message {
+            margin-right: var(--space-4);
+          }
         }
       }
     }
diff --git a/app/controllers/admin/admin_notices_controller.rb b/app/controllers/admin/admin_notices_controller.rb
new file mode 100644
index 00000000000..d19f763f1bd
--- /dev/null
+++ b/app/controllers/admin/admin_notices_controller.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class Admin::AdminNoticesController < Admin::StaffController
+  def destroy
+    AdminNotices::Dismiss.call do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+    end
+  end
+end
diff --git a/app/models/problem_check_tracker.rb b/app/models/problem_check_tracker.rb
index 722491b0542..38ffb05bfdc 100644
--- a/app/models/problem_check_tracker.rb
+++ b/app/models/problem_check_tracker.rb
@@ -32,11 +32,14 @@ class ProblemCheckTracker < ActiveRecord::Base
   end
 
   def no_problem!(next_run_at: nil)
+    reset
+    silence_the_alarm
+  end
+
+  def reset(next_run_at: nil)
     now = Time.current
 
     update!(blips: 0, last_run_at: now, last_success_at: now, next_run_at:)
-
-    silence_the_alarm
   end
 
   def check
diff --git a/app/serializers/admin_notice_serializer.rb b/app/serializers/admin_notice_serializer.rb
index febcb511ae2..8f3da1134db 100644
--- a/app/serializers/admin_notice_serializer.rb
+++ b/app/serializers/admin_notice_serializer.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 class AdminNoticeSerializer < ApplicationSerializer
-  attributes :priority, :message, :identifier
+  attributes :id, :priority, :message, :identifier
 end
diff --git a/app/services/admin_notices/dismiss.rb b/app/services/admin_notices/dismiss.rb
new file mode 100644
index 00000000000..93e57d048d8
--- /dev/null
+++ b/app/services/admin_notices/dismiss.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+class AdminNotices::Dismiss
+  include Service::Base
+
+  model :admin_notice
+
+  policy :invalid_access
+
+  transaction do
+    step :destroy
+    step :reset_problem_check
+  end
+
+  private
+
+  def fetch_admin_notice(id:)
+    AdminNotice.find_by(id: id)
+  end
+
+  def invalid_access(guardian:)
+    guardian.is_admin?
+  end
+
+  def destroy(admin_notice:)
+    admin_notice.destroy!
+  end
+
+  def reset_problem_check(admin_notice:)
+    ProblemCheckTracker.find_by(identifier: admin_notice.identifier)&.reset
+  end
+end
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 9713403fcfa..6a6879e175b 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -5077,6 +5077,7 @@ en:
         installed_version: "Installed"
         latest_version: "Latest"
         problems_found: "Some advice based on your current site settings"
+        dismiss_notice: "Dismiss"
         new_features:
           title: "What's new"
           subtitle: "We are releasing new features and improvements all the time. This page covers the highlights, but you can click 'Learn more' to see extensive release notes."
diff --git a/config/routes.rb b/config/routes.rb
index a336a0d81a8..88ccaa708ba 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -401,6 +401,8 @@ Discourse::Application.routes.draw do
           collection { put "/" => "about#update" }
         end
       end
+
+      resources :admin_notices, only: %i[destroy], constraints: AdminConstraint.new
     end # admin namespace
 
     get "email/unsubscribe/:key" => "email#unsubscribe", :as => "email_unsubscribe"
diff --git a/spec/fabricators/admin_notice_fabricator.rb b/spec/fabricators/admin_notice_fabricator.rb
index fa0e23e0d83..dbb3478ca11 100644
--- a/spec/fabricators/admin_notice_fabricator.rb
+++ b/spec/fabricators/admin_notice_fabricator.rb
@@ -3,4 +3,5 @@
 Fabricator(:admin_notice) do
   priority { "low" }
   identifier { "test_notice" }
+  subject { "problem" }
 end
diff --git a/spec/models/problem_check_tracker_spec.rb b/spec/models/problem_check_tracker_spec.rb
index 60b5f9f4e0e..b0d93a20786 100644
--- a/spec/models/problem_check_tracker_spec.rb
+++ b/spec/models/problem_check_tracker_spec.rb
@@ -212,4 +212,30 @@ RSpec.describe ProblemCheckTracker do
       end
     end
   end
+
+  describe "#reset" do
+    let(:problem_tracker) do
+      Fabricate(:problem_check_tracker, identifier: "twitter_login", **original_attributes)
+    end
+
+    let(:original_attributes) do
+      {
+        blips: 0,
+        last_problem_at: 1.week.ago,
+        last_success_at: Time.current,
+        last_run_at: 24.hours.ago,
+        next_run_at: nil,
+      }
+    end
+
+    let(:updated_attributes) { { blips: 0 } }
+
+    it do
+      freeze_time
+
+      expect { problem_tracker.reset(next_run_at: 24.hours.from_now) }.to change {
+        problem_tracker.attributes
+      }.to(hash_including(updated_attributes))
+    end
+  end
 end
diff --git a/spec/services/admin_notices/dismiss_spec.rb b/spec/services/admin_notices/dismiss_spec.rb
new file mode 100644
index 00000000000..758aba9d1ca
--- /dev/null
+++ b/spec/services/admin_notices/dismiss_spec.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+RSpec.describe(AdminNotices::Dismiss) do
+  subject(:result) { described_class.call(id: admin_notice.id, guardian: current_user.guardian) }
+
+  let!(:admin_notice) { Fabricate(:admin_notice, identifier: "problem.test") }
+  let!(:problem_check) { Fabricate(:problem_check_tracker, identifier: "problem.test", blips: 3) }
+
+  context "when user is not allowed to perform the action" do
+    fab!(:current_user) { Fabricate(:user) }
+
+    it { is_expected.to fail_a_policy(:invalid_access) }
+  end
+
+  context "when user is allowed to perform the action" do
+    fab!(:current_user) { Fabricate(:admin) }
+
+    it { is_expected.to run_successfully }
+
+    it "sets the service result as successful" do
+      expect(result).to be_a_success
+    end
+
+    it "destroys the admin notice" do
+      expect { result }.to change { AdminNotice.count }.from(1).to(0)
+    end
+
+    it "resets any associated problem check" do
+      expect { result }.to change { problem_check.reload.blips }.from(3).to(0)
+    end
+  end
+end
diff --git a/spec/system/admin_notices_spec.rb b/spec/system/admin_notices_spec.rb
new file mode 100644
index 00000000000..6653c11e56d
--- /dev/null
+++ b/spec/system/admin_notices_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+describe "Admin Notices", type: :system do
+  fab!(:admin)
+
+  let(:admin_dashboard) { PageObjects::Pages::AdminDashboard.new }
+
+  before do
+    Fabricate(:admin_notice)
+
+    I18n.backend.store_translations(:en, dashboard: { problem: { test_notice: "Houston" } })
+
+    sign_in(admin)
+  end
+
+  it "supports dismissing admin notices" do
+    admin_dashboard.visit
+
+    expect(admin_dashboard).to have_admin_notice(I18n.t("dashboard.problem.test_notice"))
+
+    admin_dashboard.dismiss_notice(I18n.t("dashboard.problem.test_notice"))
+
+    expect(admin_dashboard).to have_no_admin_notice(I18n.t("dashboard.problem.test_notice"))
+  end
+end
diff --git a/spec/system/page_objects/pages/admin_dashboard.rb b/spec/system/page_objects/pages/admin_dashboard.rb
new file mode 100644
index 00000000000..6d6b1c940de
--- /dev/null
+++ b/spec/system/page_objects/pages/admin_dashboard.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class AdminDashboard < PageObjects::Pages::Base
+      def visit
+        page.visit("/admin")
+        self
+      end
+
+      def has_admin_notice?(message)
+        has_css?(".dashboard-problem", text: message)
+      end
+
+      def has_no_admin_notice?(message)
+        has_no_css?(".dashboard-problem", text: message)
+      end
+
+      def dismiss_notice(message)
+        find(".dashboard-problem", text: message).find(".btn").click
+      end
+    end
+  end
+end