From d8102cb532c4c125acdf19efec5a404452715f23 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 29 Jan 2025 09:44:20 +1000
Subject: [PATCH] FIX: Update AdminNotice details when problem check tracker
 changes (#31031)

We have many problem check trackers, and some of them
like `OutOfDateThemes` can have a message which has variable
data in it shown to admins. In this case, a list of themes
that need updating. Currently if you resolve one of these
out of date themes and refresh the list of problems, you
do not see any change.

This is happening because we are only updating the `details`
of the `ProblemCheckTracker` record, not the corresponding
`AdminNotice` record which is what is displayed to the admins
on their dashboard. This commit fixes the issue by updating the
details of the notice at the same time as the problem check
tracker details.
---
 app/models/problem_check_tracker.rb       | 18 ++++++++++--
 spec/models/problem_check_tracker_spec.rb | 36 +++++++++++++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/app/models/problem_check_tracker.rb b/app/models/problem_check_tracker.rb
index 20b74f52fac..4afc92fb9bf 100644
--- a/app/models/problem_check_tracker.rb
+++ b/app/models/problem_check_tracker.rb
@@ -28,6 +28,8 @@ class ProblemCheckTracker < ActiveRecord::Base
 
     update!(blips: blips + 1, details:, last_run_at: now, last_problem_at: now, next_run_at:)
 
+    update_notice_details(details)
+
     sound_the_alarm if sound_the_alarm?
   end
 
@@ -55,6 +57,18 @@ class ProblemCheckTracker < ActiveRecord::Base
 
   private
 
+  def resolved_target
+    self.target || ProblemCheck::NO_TARGET
+  end
+
+  def details_with_target(details)
+    details.merge(target: resolved_target)
+  end
+
+  def update_notice_details(details)
+    admin_notice.where(identifier:).update_all(details: details_with_target(details))
+  end
+
   def sound_the_alarm?
     failing? && blips > check.max_blips
   end
@@ -62,7 +76,7 @@ class ProblemCheckTracker < ActiveRecord::Base
   def sound_the_alarm
     admin_notice.create_with(
       priority: check.priority,
-      details: details.merge(target: target || ProblemCheck::NO_TARGET),
+      details: details_with_target(self.details),
     ).find_or_create_by(identifier:)
   end
 
@@ -71,7 +85,7 @@ class ProblemCheckTracker < ActiveRecord::Base
   end
 
   def admin_notice
-    AdminNotice.problem.where("details->>'target' = ?", target || ProblemCheck::NO_TARGET)
+    AdminNotice.problem.where("details->>'target' = ?", resolved_target)
   end
 end
 
diff --git a/spec/models/problem_check_tracker_spec.rb b/spec/models/problem_check_tracker_spec.rb
index dadeca7bb6f..d58495b3509 100644
--- a/spec/models/problem_check_tracker_spec.rb
+++ b/spec/models/problem_check_tracker_spec.rb
@@ -146,6 +146,42 @@ RSpec.describe ProblemCheckTracker do
       end
     end
 
+    context "when the details of the problem change but the problem remains" do
+      let(:blips) { 1 }
+
+      it "updates the notice" do
+        original_details = {
+          themes_list:
+            "<ul><li><a href=\"/admin/customize/themes/13\">discourse-blank-theme</a></li> <li><a href=\"/admin/customize/themes/31\">Simple Theme</a></li></ul>",
+          base_path: "",
+        }
+
+        expect do problem_tracker.problem!(details: original_details) end.to change {
+          AdminNotice.problem.count
+        }.by(1)
+
+        admin_notice = AdminNotice.problem.find_by(identifier: "twitter_login")
+
+        expect(
+          admin_notice.details.merge(target: problem_tracker.target).with_indifferent_access,
+        ).to eq(original_details.merge(target: problem_tracker.target).with_indifferent_access)
+
+        new_details = {
+          themes_list: "<ul><li><a href=\"/admin/customize/themes/31\">Simple Theme</a></li></ul>",
+          base_path: "",
+        }
+        expect do problem_tracker.problem!(details: new_details) end.not_to change {
+          AdminNotice.problem.count
+        }
+
+        admin_notice.reload
+
+        expect(
+          admin_notice.details.merge(target: problem_tracker.target).with_indifferent_access,
+        ).to eq(new_details.merge(target: problem_tracker.target).with_indifferent_access)
+      end
+    end
+
     context "when there's an alarm sounding for multi-target trackers" do
       let(:blips) { 1 }