FIX: Appropriately handle uninstalled problem checks (#28771)

When running checks, we look to the existing problem check trackers and try to grab their ProblemCheck classes.

In some cases this is no longer in the problem check repository, e.g. when the check was part of a plugin that has been uninstalled.

In the case where the check was scheduled, this would lead to an error in one of the jobs

app/jobs/scheduled/run_problem_checks.rb

@@ -12,12 +12,19 @@ module Jobs
     def execute(_args)
       scheduled_checks =
         ProblemCheckTracker.all.filter_map do |tracker|
-          tracker.check if tracker.check.scheduled? && tracker.ready_to_run?
+          tracker.check if eligible_for_this_run?(tracker)
         end
 
       scheduled_checks.each do |check|
         Jobs.enqueue(:run_problem_check, check_identifier: check.identifier.to_s)
       end
     end
+
+    private
+
+    def eligible_for_this_run?(tracker)
+      tracker.check.present? && tracker.check.enabled? && tracker.check.scheduled? &&
+        tracker.ready_to_run?
+    end
   end
 end

app/models/problem_check_tracker.rb

@@ -43,7 +43,14 @@ class ProblemCheckTracker < ActiveRecord::Base
   end
 
   def check
-    ProblemCheck[identifier]
+    check = ProblemCheck[identifier]
+
+    return check if check.present?
+
+    silence_the_alarm
+    destroy
+
+    nil
   end
 
   private

spec/jobs/run_problem_checks_spec.rb

@@ -11,18 +11,24 @@ RSpec.describe Jobs::RunProblemChecks do
 
     ProblemCheck::NonScheduledCheck = Class.new(ProblemCheck) { def call = [] }
 
+    ProblemCheck::DisabledCheck =
+      Class.new(ProblemCheck) do
+        self.perform_every = 30.minutes
+        self.enabled = false
+
+        def call = []
+      end
+
     stub_const(
       ProblemCheck,
       "CORE_PROBLEM_CHECKS",
-      [ProblemCheck::ScheduledCheck, ProblemCheck::NonScheduledCheck],
+      [ProblemCheck::ScheduledCheck, ProblemCheck::NonScheduledCheck, ProblemCheck::DisabledCheck],
       &example
     )
 
-    Discourse.redis.flushdb
-    AdminDashboardData.reset_problem_checks
-
     ProblemCheck.send(:remove_const, "ScheduledCheck")
     ProblemCheck.send(:remove_const, "NonScheduledCheck")
+    ProblemCheck.send(:remove_const, "DisabledCheck")
   end
 
   context "when the tracker determines the check is ready to run" do
@@ -68,12 +74,29 @@ RSpec.describe Jobs::RunProblemChecks do
     end
   end
 
-  it "does not schedule non-scheduled checks" do
-    expect_not_enqueued_with(
-      job: :run_problem_check,
-      args: {
-        check_identifier: "non_scheduled_check",
-      },
-    ) { described_class.new.execute([]) }
+  context "when dealing with a disabled check" do
+    before { ProblemCheckTracker.create!(identifier: "disabled_check", next_run_at: nil) }
+
+    it "does not schedule any check" do
+      expect_not_enqueued_with(
+        job: :run_problem_check,
+        args: {
+          check_identifier: "disabled_check",
+        },
+      ) { described_class.new.execute([]) }
+    end
+  end
+
+  context "when dealing with an uninstalled check" do
+    before { ProblemCheckTracker.create!(identifier: "uninstalled_check", next_run_at: nil) }
+
+    it "does not schedule any check" do
+      expect_not_enqueued_with(
+        job: :run_problem_check,
+        args: {
+          check_identifier: "uninstalled_check",
+        },
+      ) { described_class.new.execute([]) }
+    end
   end
 end

spec/models/problem_check_tracker_spec.rb

@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 RSpec.describe ProblemCheckTracker do
-  before { described_class.any_instance.stubs(:check).returns(stub(max_blips: 1, priority: "low")) }
-
   describe "validations" do
     let(:record) { described_class.new(identifier: "twitter_login") }
 
@@ -24,6 +22,23 @@ RSpec.describe ProblemCheckTracker do
     end
   end
 
+  describe "#check" do
+    before do
+      Fabricate(:problem_check_tracker, identifier: "twitter_login")
+      Fabricate(:problem_check_tracker, identifier: "missing_check")
+    end
+
+    context "when the tracker has a corresponding check" do
+      it { expect(described_class[:twitter_login].check.new).to be_a(ProblemCheck) }
+    end
+
+    context "when the checking logic of the tracker has been removed or renamed" do
+      it do
+        expect { described_class[:missing_check].check }.to change { described_class.count }.by(-1)
+      end
+    end
+  end
+
   describe "#ready_to_run?" do
     let(:problem_tracker) { described_class.new(next_run_at:) }
 
@@ -169,7 +184,10 @@ RSpec.describe ProblemCheckTracker do
     context "when there are still blips to go" do
       let(:blips) { 0 }
 
+      before { ProblemCheck::TwitterLogin.stubs(:max_blips).returns(1) }
+
       it "does not sound the alarm" do
+        puts ProblemCheck::TwitterLogin.max_blips
         expect { problem_tracker.problem!(next_run_at: 24.hours.from_now) }.not_to change {
           AdminNotice.problem.count
         }