DEV: Make problem check registration more explicit (#26413)

Previously the problem check registry simply looked at the subclasses of ProblemCheck. This was causing some confusion in environments where eager loading is not enabled, as the registry would appear empty as a result of the classes never being referenced (and thus never loaded.)

This PR changes the approach to a more explicit one. I followed other implementations (bookmarkable and hashtag autocomplete.) As a bonus, this now has a neat plugin entry point as well.

app/models/problem_check.rb

@@ -28,6 +28,37 @@ class ProblemCheck
   #
   config_accessor :max_blips, default: 0, instance_writer: false
 
+  # Problem check classes need to be registered here in order to be enabled.
+  #
+  # Note: This list must come after the `config_accessor` declarations.
+  #
+  CORE_PROBLEM_CHECKS = [
+    ProblemCheck::EmailPollingErroredRecently,
+    ProblemCheck::FacebookConfig,
+    ProblemCheck::FailingEmails,
+    ProblemCheck::ForceHttps,
+    ProblemCheck::GithubConfig,
+    ProblemCheck::GoogleAnalyticsVersion,
+    ProblemCheck::GoogleOauth2Config,
+    ProblemCheck::GroupEmailCredentials,
+    ProblemCheck::HostNames,
+    ProblemCheck::ImageMagick,
+    ProblemCheck::MissingMailgunApiKey,
+    ProblemCheck::OutOfDateThemes,
+    ProblemCheck::RailsEnv,
+    ProblemCheck::Ram,
+    ProblemCheck::S3BackupConfig,
+    ProblemCheck::S3Cdn,
+    ProblemCheck::S3UploadConfig,
+    ProblemCheck::SidekiqCheck,
+    ProblemCheck::SubfolderEndsInSlash,
+    ProblemCheck::TranslationOverrides,
+    ProblemCheck::TwitterConfig,
+    ProblemCheck::TwitterLogin,
+    ProblemCheck::UnreachableThemes,
+    ProblemCheck::WatchedWords,
+  ].freeze
+
   def self.[](key)
     key = key.to_sym
 
@@ -35,7 +66,7 @@ class ProblemCheck
   end
 
   def self.checks
-    descendants
+    CORE_PROBLEM_CHECKS | DiscoursePluginRegistry.problem_checks
   end
 
   def self.scheduled

lib/discourse_plugin_registry.rb

@@ -121,6 +121,8 @@ class DiscoursePluginRegistry
 
   define_filtered_register :post_strippers
 
+  define_filtered_register :problem_checks
+
   def self.register_auth_provider(auth_provider)
     self.auth_providers << auth_provider
   end

lib/plugin/instance.rb

@@ -317,6 +317,10 @@ class Plugin::Instance
     Site.preloaded_category_custom_fields << field
   end
 
+  def register_problem_check(klass)
+    DiscoursePluginRegistry.register_problem_check(klass, self)
+  end
+
   def custom_avatar_column(column)
     reloadable_patch do |plugin|
       UserLookup.lookup_columns << column

spec/jobs/run_problem_check_spec.rb

@@ -1,14 +1,10 @@
 # frozen_string_literal: true
 
 RSpec.describe Jobs::RunProblemCheck do
-  after do
-    Discourse.redis.flushdb
-
-    ProblemCheck.send(:remove_const, "TestCheck")
-  end
+  after { Discourse.redis.flushdb }
 
   context "when there are problems" do
-    before do
+    around do |example|
       ProblemCheck::TestCheck =
         Class.new(ProblemCheck) do
           self.perform_every = 30.minutes
@@ -25,6 +21,10 @@ RSpec.describe Jobs::RunProblemCheck do
             ]
           end
         end
+
+      stub_const(ProblemCheck, "CORE_PROBLEM_CHECKS", [ProblemCheck::TestCheck], &example)
+
+      ProblemCheck.send(:remove_const, "TestCheck")
     end
 
     it "adds the messages to the Redis problems array" do
@@ -37,7 +37,7 @@ RSpec.describe Jobs::RunProblemCheck do
   end
 
   context "with multiple problems with the same identifier" do
-    before do
+    around do |example|
       ProblemCheck::TestCheck =
         Class.new(ProblemCheck) do
           self.perform_every = 30.minutes
@@ -58,6 +58,10 @@ RSpec.describe Jobs::RunProblemCheck do
             ]
           end
         end
+
+      stub_const(ProblemCheck, "CORE_PROBLEM_CHECKS", [ProblemCheck::TestCheck], &example)
+
+      ProblemCheck.send(:remove_const, "TestCheck")
     end
 
     it "does not add the same problem twice" do
@@ -70,7 +74,7 @@ RSpec.describe Jobs::RunProblemCheck do
   end
 
   context "when there are retries remaining" do
-    before do
+    around do |example|
       ProblemCheck::TestCheck =
         Class.new(ProblemCheck) do
           self.perform_every = 30.minutes
@@ -80,6 +84,10 @@ RSpec.describe Jobs::RunProblemCheck do
             [ProblemCheck::Problem.new("Yuge problem")]
           end
         end
+
+      stub_const(ProblemCheck, "CORE_PROBLEM_CHECKS", [ProblemCheck::TestCheck], &example)
+
+      ProblemCheck.send(:remove_const, "TestCheck")
     end
 
     it "does not yet update the problem check tracker" do
@@ -100,7 +108,7 @@ RSpec.describe Jobs::RunProblemCheck do
   end
 
   context "when there are no retries remaining" do
-    before do
+    around do |example|
       ProblemCheck::TestCheck =
         Class.new(ProblemCheck) do
           self.perform_every = 30.minutes
@@ -110,6 +118,10 @@ RSpec.describe Jobs::RunProblemCheck do
             [ProblemCheck::Problem.new("Yuge problem")]
           end
         end
+
+      stub_const(ProblemCheck, "CORE_PROBLEM_CHECKS", [ProblemCheck::TestCheck], &example)
+
+      ProblemCheck.send(:remove_const, "TestCheck")
     end
 
     it "updates the problem check tracker" do
@@ -126,7 +138,7 @@ RSpec.describe Jobs::RunProblemCheck do
   end
 
   context "when the check unexpectedly errors out" do
-    before do
+    around do |example|
       ProblemCheck::TestCheck =
         Class.new(ProblemCheck) do
           self.max_retries = 1
@@ -135,6 +147,10 @@ RSpec.describe Jobs::RunProblemCheck do
             raise StandardError.new("Something went wrong")
           end
         end
+
+      stub_const(ProblemCheck, "CORE_PROBLEM_CHECKS", [ProblemCheck::TestCheck], &example)
+
+      ProblemCheck.send(:remove_const, "TestCheck")
     end
 
     it "does not add a problem to the Redis array" do

spec/jobs/run_problem_checks_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 RSpec.describe Jobs::RunProblemChecks do
-  before do
+  around do |example|
     ProblemCheck::ScheduledCheck =
       Class.new(ProblemCheck) do
         self.perform_every = 30.minutes
@@ -10,9 +10,14 @@ RSpec.describe Jobs::RunProblemChecks do
       end
 
     ProblemCheck::NonScheduledCheck = Class.new(ProblemCheck) { def call = [] }
-  end
 
-  after do
+    stub_const(
+      ProblemCheck,
+      "CORE_PROBLEM_CHECKS",
+      [ProblemCheck::ScheduledCheck, ProblemCheck::NonScheduledCheck],
+      &example
+    )
+
     Discourse.redis.flushdb
     AdminDashboardData.reset_problem_checks
 

spec/models/problem_check_spec.rb

@@ -1,17 +1,17 @@
 # frozen_string_literal: true
 
 RSpec.describe ProblemCheck do
-  # rubocop:disable RSpec/BeforeAfterAll
-  before(:all) do
+  around do |example|
     ScheduledCheck = Class.new(described_class) { self.perform_every = 30.minutes }
     RealtimeCheck = Class.new(described_class)
-  end
+    PluginCheck = Class.new(described_class)
+
+    stub_const(described_class, "CORE_PROBLEM_CHECKS", [ScheduledCheck, RealtimeCheck], &example)
 
-  after(:all) do
     Object.send(:remove_const, ScheduledCheck.name)
     Object.send(:remove_const, RealtimeCheck.name)
+    Object.send(:remove_const, PluginCheck.name)
   end
-  # rubocop:enable RSpec/BeforeAfterAll
 
   let(:scheduled_check) { ScheduledCheck }
   let(:realtime_check) { RealtimeCheck }
@@ -48,4 +48,22 @@ RSpec.describe ProblemCheck do
     it { expect(realtime_check).to be_realtime }
     it { expect(scheduled_check).to_not be_realtime }
   end
+
+  describe "plugin problem check registration" do
+    before { DiscoursePluginRegistry.register_problem_check(PluginCheck, stub(enabled?: enabled)) }
+
+    after { DiscoursePluginRegistry.reset! }
+
+    context "when the plugin is enabled" do
+      let(:enabled) { true }
+
+      it { expect(described_class.checks).to include(PluginCheck) }
+    end
+
+    context "when the plugin is disabled" do
+      let(:enabled) { false }
+
+      it { expect(described_class.checks).not_to include(PluginCheck) }
+    end
+  end
 end