From 2d68e5d942fa9312655d3d5abacf15d8a9fca948 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 20 Dec 2021 09:59:11 +1000 Subject: [PATCH] FEATURE: Scheduled problem checks for admin dashboard (#15327) This commit introduces scheduled problem checks for the admin dashboard, which are long running or otherwise cumbersome problem checks that will be run every 10 minutes rather than every time the dashboard is loaded. If these scheduled checks add a problem, the problem will remain until it is cleared or until the scheduled job runs again. An example of a check that should be scheduled is validating credentials against an external provider. This commit also introduces the concept of a `priority` to the problems generated by `AdminDashboardData` and the scheduled checks. This is `low` by default, and can be set to `high`, but this commit does not change any part of the UI with this information, only adds a CSS class. I will be making a follow up PR to check group SMTP credentials. --- .../components/dashboard-problems.hbs | 2 +- .../discourse/tests/fixtures/problems.js | 2 +- app/jobs/scheduled/problem_checks.rb | 17 ++ app/models/admin_dashboard_data.rb | 227 +++++++++++++----- .../pop3_polling_enabled_setting_validator.rb | 1 - spec/jobs/problem_checks_spec.rb | 76 ++++++ ...m_spec.rb => admin_dashboard_data_spec.rb} | 151 ++++++++---- 7 files changed, 372 insertions(+), 104 deletions(-) create mode 100644 app/jobs/scheduled/problem_checks.rb create mode 100644 spec/jobs/problem_checks_spec.rb rename spec/models/{admin_dashboard_problem_spec.rb => admin_dashboard_data_spec.rb} (69%) diff --git a/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs b/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs index 1160a252133..cd74918a73a 100644 --- a/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs +++ b/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs @@ -12,7 +12,7 @@
diff --git a/app/assets/javascripts/discourse/tests/fixtures/problems.js b/app/assets/javascripts/discourse/tests/fixtures/problems.js index 7e188ee5830..4800fd7d743 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/problems.js +++ b/app/assets/javascripts/discourse/tests/fixtures/problems.js @@ -1,5 +1,5 @@ export default { "/admin/dashboard/problems.json": { - problems: ["Houston..."] + problems: [{ message: "Houston...", priority: "low" }] } }; diff --git a/app/jobs/scheduled/problem_checks.rb b/app/jobs/scheduled/problem_checks.rb new file mode 100644 index 00000000000..5135e20f027 --- /dev/null +++ b/app/jobs/scheduled/problem_checks.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Jobs + # This job runs all of the scheduled problem checks for the admin dashboard + # on a regular basis. To add a problem check for this scheduled job run + # call AdminDashboardData.add_scheduled_problem_check + class ProblemChecks < ::Jobs::Scheduled + every 10.minutes + + def execute(_args) + # This way if the problems have been solved in the meantime, then they will + # not be re-added by the relevant checker, and will be cleared. + AdminDashboardData.clear_found_scheduled_check_problems + AdminDashboardData.execute_scheduled_checks + end + end +end diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index 7e21f4b91e6..84748dcbcb9 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -3,17 +3,47 @@ class AdminDashboardData include StatsCacheable + cattr_reader :problem_syms, + :problem_blocks, + :problem_messages, + :problem_scheduled_check_blocks + + class Problem + VALID_PRIORITIES = ["low", "high"].freeze + + attr_reader :message, :priority, :identifier + + def initialize(message, priority: "low", identifier: nil) + @message = message + @priority = VALID_PRIORITIES.include?(priority) ? priority : "low" + @identifier = identifier + end + + def to_s + @message + end + + def to_h + { message: message, priority: priority, identifier: identifier } + end + + def self.from_h(h) + h = h.with_indifferent_access + return if h[:message].blank? + new(h[:message], priority: h[:priority], identifier: h[:identifier]) + end + end + # kept for backward compatibility GLOBAL_REPORTS ||= [] + PROBLEM_MESSAGE_PREFIX = "admin-problem:" + SCHEDULED_PROBLEM_STORAGE_KEY = "admin-found-scheduled-problems" + def initialize(opts = {}) @opts = opts end - def self.fetch_stats - new.as_json - end - def get_json {} end @@ -22,31 +52,21 @@ class AdminDashboardData @json ||= get_json end - def self.reports(source) - source.map { |type| Report.find(type).as_json } - end - - def self.stats_cache_key - "dashboard-data-#{Report::SCHEMA_VERSION}" - end - - def self.add_problem_check(*syms, &blk) - @problem_syms.push(*syms) if syms - @problem_blocks << blk if blk - end - class << self; attr_reader :problem_syms, :problem_blocks, :problem_messages; end - def problems problems = [] - AdminDashboardData.problem_syms.each do |sym| - problems << public_send(sym) + self.class.problem_syms.each do |sym| + message = public_send(sym) + problems << Problem.new(message) if message.present? end - AdminDashboardData.problem_blocks.each do |blk| - problems << instance_exec(&blk) + self.class.problem_blocks.each do |blk| + message = instance_exec(&blk) + problems << Problem.new(message) if message.present? end - AdminDashboardData.problem_messages.each do |i18n_key| - problems << AdminDashboardData.problem_message_check(i18n_key) + self.class.problem_messages.each do |i18n_key| + message = self.class.problem_message_check(i18n_key) + problems << Problem.new(message) if message.present? end + problems += self.class.load_found_scheduled_check_problems problems.compact! if problems.empty? @@ -58,6 +78,126 @@ class AdminDashboardData problems end + def self.add_problem_check(*syms, &blk) + @@problem_syms.push(*syms) if syms + @@problem_blocks << blk if blk + end + + def self.add_scheduled_problem_check(check_identifier, &blk) + @@problem_scheduled_check_blocks[check_identifier] = blk + end + + def self.add_found_scheduled_check_problem(problem) + problems = load_found_scheduled_check_problems + if problem.identifier.present? + return if problems.find { |p| p.identifier == problem.identifier } + end + problems << problem + set_found_scheduled_check_problems(problems) + end + + def self.set_found_scheduled_check_problems(problems) + Discourse.redis.setex(SCHEDULED_PROBLEM_STORAGE_KEY, 300, JSON.dump(problems.map(&:to_h))) + end + + def self.clear_found_scheduled_check_problems + Discourse.redis.del(SCHEDULED_PROBLEM_STORAGE_KEY) + end + + def self.clear_found_problem(identifier) + problems = load_found_scheduled_check_problems + problems.reject! { |p| p.identifier == identifier } + set_found_scheduled_check_problems(problems) + end + + def self.load_found_scheduled_check_problems + found_problems_json = Discourse.redis.get(SCHEDULED_PROBLEM_STORAGE_KEY) + return [] if found_problems_json.blank? + begin + JSON.parse(found_problems_json).map do |problem| + Problem.from_h(problem) + end + rescue JSON::ParserError => err + Discourse.warn_exception(err, message: "Error parsing found problem JSON in admin dashboard: #{found_problems_json}") + [] + end + end + + def self.register_default_scheduled_problem_checks + # TODO (martin) Add group SMTP check here + end + + def self.execute_scheduled_checks + found_problems = [] + problem_scheduled_check_blocks.each do |check_identifier, blk| + problems = nil + + begin + problems = instance_exec(&blk) + rescue StandardError => err + Discourse.warn_exception(err, message: "A scheduled admin dashboard problem check (#{check_identifier}) errored.") + # we don't want to hold up other checks because this one errored + next + end + + found_problems += Array.wrap(problems) + end + + found_problems.compact.each do |problem| + next if !problem.is_a?(Problem) + add_found_scheduled_check_problem(problem) + end + end + + ## + # We call this method in the class definition below + # so all of the problem checks in this class are registered on + # boot. These problem checks are run when the problems are loaded in + # the admin dashboard controller. + # + # This method also can be used in testing to reset checks between + # tests. It will also fire multiple times in development mode because + # classes are not cached. + def self.reset_problem_checks + @@problem_syms = [] + @@problem_blocks = [] + @@problem_scheduled_check_blocks = {} + + @@problem_messages = [ + 'dashboard.bad_favicon_url', + 'dashboard.poll_pop3_timeout', + 'dashboard.poll_pop3_auth_error', + ] + + add_problem_check :rails_env_check, :host_names_check, :force_https_check, + :ram_check, :google_oauth2_config_check, + :facebook_config_check, :twitter_config_check, + :github_config_check, :s3_config_check, :s3_cdn_check, + :image_magick_check, :failing_emails_check, + :subfolder_ends_in_slash_check, + :email_polling_errored_recently, + :out_of_date_themes, :unreachable_themes, :watched_words_check + + register_default_scheduled_problem_checks + + add_problem_check do + sidekiq_check || queue_size_check + end + end + reset_problem_checks + + def self.fetch_stats + new.as_json + end + + def self.reports(source) + source.map { |type| Report.find(type).as_json } + end + + def self.stats_cache_key + "dashboard-data-#{Report::SCHEMA_VERSION}" + end + def self.problems_started_key 'dash-problems-started-at' end @@ -76,40 +216,19 @@ class AdminDashboardData s ? Time.zone.parse(s) : nil end - # used for testing - def self.reset_problem_checks - @problem_syms = [] - @problem_blocks = [] - - @problem_messages = [ - 'dashboard.bad_favicon_url', - 'dashboard.poll_pop3_timeout', - 'dashboard.poll_pop3_auth_error', - ] - - add_problem_check :rails_env_check, :host_names_check, :force_https_check, - :ram_check, :google_oauth2_config_check, - :facebook_config_check, :twitter_config_check, - :github_config_check, :s3_config_check, :s3_cdn_check, - :image_magick_check, :failing_emails_check, - :subfolder_ends_in_slash_check, - :pop3_polling_configuration, :email_polling_errored_recently, - :out_of_date_themes, :unreachable_themes, :watched_words_check - - add_problem_check do - sidekiq_check || queue_size_check - end - end - reset_problem_checks - def self.fetch_problems(opts = {}) - AdminDashboardData.new(opts).problems + new(opts).problems end def self.problem_message_check(i18n_key) Discourse.redis.get(problem_message_key(i18n_key)) ? I18n.t(i18n_key, base_path: Discourse.base_path) : nil end + ## + # Arbitrary messages cannot be added here, they must already be defined + # in the @problem_messages array which is defined in reset_problem_checks. + # The array is iterated over and each key that exists in redis will be added + # to the final problems output in #problems. def self.add_problem_message(i18n_key, expire_seconds = nil) if expire_seconds.to_i > 0 Discourse.redis.setex problem_message_key(i18n_key), expire_seconds.to_i, 1 @@ -123,7 +242,7 @@ class AdminDashboardData end def self.problem_message_key(i18n_key) - "admin-problem:#{i18n_key}" + "#{PROBLEM_MESSAGE_PREFIX}#{i18n_key}" end def rails_env_check @@ -207,10 +326,6 @@ class AdminDashboardData I18n.t('dashboard.subfolder_ends_in_slash') if Discourse.base_path =~ /\/$/ end - def pop3_polling_configuration - POP3PollingEnabledSettingValidator.new.error_message if SiteSetting.pop3_polling_enabled - end - def email_polling_errored_recently errors = Jobs::PollMailbox.errors_in_past_24_hours I18n.t('dashboard.email_polling_errored_recently', count: errors, base_path: Discourse.base_path) if errors > 0 diff --git a/lib/validators/pop3_polling_enabled_setting_validator.rb b/lib/validators/pop3_polling_enabled_setting_validator.rb index ed53a5cc9b7..cee2cfec85b 100644 --- a/lib/validators/pop3_polling_enabled_setting_validator.rb +++ b/lib/validators/pop3_polling_enabled_setting_validator.rb @@ -33,7 +33,6 @@ class POP3PollingEnabledSettingValidator private def authentication_works? - # TODO (martin, post-2.7 release) Change to use EmailSettingsValidator # EmailSettingsValidator.validate_pop3( # host: SiteSetting.pop3_polling_host, diff --git a/spec/jobs/problem_checks_spec.rb b/spec/jobs/problem_checks_spec.rb new file mode 100644 index 00000000000..2f8e89db926 --- /dev/null +++ b/spec/jobs/problem_checks_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Jobs::ProblemChecks do + after do + Discourse.redis.flushdb + AdminDashboardData.reset_problem_checks + end + + it "runs the scheduled problem check that has been added and adds the messages to the load_found_scheduled_check_problems array" do + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do + AdminDashboardData::Problem.new("big problem") + end + + described_class.new.execute(nil) + problems = AdminDashboardData.load_found_scheduled_check_problems + expect(problems.count).to eq(1) + expect(problems.first).to be_a(AdminDashboardData::Problem) + expect(problems.first.to_s).to eq("big problem") + end + + it "can handle the problem check returning multiple problems" do + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do + [ + AdminDashboardData::Problem.new("big problem"), + AdminDashboardData::Problem.new("yuge problem", priority: "high", identifier: "config_is_a_mess") + ] + end + + described_class.new.execute(nil) + problems = AdminDashboardData.load_found_scheduled_check_problems + expect(problems.map(&:to_s)).to match_array(["big problem", "yuge problem"]) + end + + it "does not add the same problem twice if the identifier already exists" do + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do + [ + AdminDashboardData::Problem.new("yuge problem", priority: "high", identifier: "config_is_a_mess"), + AdminDashboardData::Problem.new("nasty problem", priority: "high", identifier: "config_is_a_mess") + ] + end + + described_class.new.execute(nil) + problems = AdminDashboardData.load_found_scheduled_check_problems + expect(problems.map(&:to_s)).to match_array(["yuge problem"]) + end + + it "starts with a blank slate every time the checks are run to avoid duplicate problems and to clear no longer firing problems" do + problem_should_fire = true + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do + if problem_should_fire + problem_should_fire = false + AdminDashboardData::Problem.new("yuge problem", priority: "high") + end + end + + described_class.new.execute(nil) + expect(AdminDashboardData.load_found_scheduled_check_problems.count).to eq(1) + described_class.new.execute(nil) + expect(AdminDashboardData.load_found_scheduled_check_problems.count).to eq(0) + end + + it "handles errors from a troublesome check and proceeds with the rest" do + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do + raise StandardError.new("something went wrong") + AdminDashboardData::Problem.new("polling issue") + end + AdminDashboardData.add_scheduled_problem_check(:test_identifier_2) do + AdminDashboardData::Problem.new("yuge problem", priority: "high") + end + + described_class.new.execute(nil) + expect(AdminDashboardData.load_found_scheduled_check_problems.count).to eq(1) + end +end diff --git a/spec/models/admin_dashboard_problem_spec.rb b/spec/models/admin_dashboard_data_spec.rb similarity index 69% rename from spec/models/admin_dashboard_problem_spec.rb rename to spec/models/admin_dashboard_data_spec.rb index b2d3a1c0ff8..eb279c3a9bc 100644 --- a/spec/models/admin_dashboard_problem_spec.rb +++ b/spec/models/admin_dashboard_data_spec.rb @@ -3,37 +3,124 @@ require 'rails_helper' describe AdminDashboardData do + after do + AdminDashboardData.reset_problem_checks + Discourse.redis.flushdb + end - describe "adding new checks" do - after do - AdminDashboardData.reset_problem_checks + describe "#fetch_problems" do + describe "adding problem messages" do + it "adds the message and returns it when the problems are fetched" do + AdminDashboardData.add_problem_message("dashboard.bad_favicon_url") + problems = AdminDashboardData.fetch_problems.map(&:to_s) + expect(problems).to include(I18n.t("dashboard.bad_favicon_url", { base_path: Discourse.base_path })) + end + + it "does not allow adding of arbitrary problem messages, they must exist in AdminDashboardData.problem_messages" do + AdminDashboardData.add_problem_message("errors.messages.invalid") + problems = AdminDashboardData.fetch_problems.map(&:to_s) + expect(problems).not_to include(I18n.t("errors.messages.invalid")) + end end - it 'calls the passed block' do + describe "adding new checks" do + it 'calls the passed block' do + AdminDashboardData.add_problem_check do + "a problem was found" + end + + problems = AdminDashboardData.fetch_problems + expect(problems.map(&:to_s)).to include("a problem was found") + end + + it 'calls the passed method' do + klass = Class.new(AdminDashboardData) do + def my_test_method + "a problem was found" + end + end + + klass.add_problem_check :my_test_method + + problems = klass.fetch_problems + expect(problems.map(&:to_s)).to include("a problem was found") + end + end + end + + describe "adding scheduled checks" do + it "adds the passed block to the scheduled checks" do called = false - AdminDashboardData.add_problem_check do + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do called = true end - AdminDashboardData.fetch_problems - expect(called).to eq(true) - - AdminDashboardData.fetch_problems(check_force_https: true) + AdminDashboardData.execute_scheduled_checks expect(called).to eq(true) end - it 'calls the passed method' do - $test_AdminDashboardData_global = false - class AdminDashboardData - def my_test_method - $test_AdminDashboardData_global = true - end + it "adds a found problem from a scheduled check" do + AdminDashboardData.add_scheduled_problem_check(:test_identifier) do + AdminDashboardData::Problem.new("test problem") end - AdminDashboardData.add_problem_check :my_test_method - AdminDashboardData.fetch_problems - expect($test_AdminDashboardData_global).to eq(true) - $test_AdminDashboardData_global = nil + AdminDashboardData.execute_scheduled_checks + problems = AdminDashboardData.load_found_scheduled_check_problems + expect(problems.first).to be_a(AdminDashboardData::Problem) + expect(problems.first.message).to eq("test problem") + end + + it "does not add duplicate problems with the same identifier" do + prob1 = AdminDashboardData::Problem.new("test problem", identifier: "test") + prob2 = AdminDashboardData::Problem.new("test problem 2", identifier: "test") + AdminDashboardData.add_found_scheduled_check_problem(prob1) + AdminDashboardData.add_found_scheduled_check_problem(prob2) + expect(AdminDashboardData.load_found_scheduled_check_problems.map(&:to_s)).to eq(["test problem"]) + end + + it "does not error when loading malformed problems saved in redis" do + Discourse.redis.set(AdminDashboardData::SCHEDULED_PROBLEM_STORAGE_KEY, "{ 'badjson") + expect(AdminDashboardData.load_found_scheduled_check_problems).to eq([]) + end + + it "clears a specific problem by identifier" do + prob1 = AdminDashboardData::Problem.new("test problem 1", identifier: "test") + AdminDashboardData.add_found_scheduled_check_problem(prob1) + AdminDashboardData.clear_found_problem("test") + expect(AdminDashboardData.load_found_scheduled_check_problems).to eq([]) + end + + it "defaults to low priority, and uses low priority if an invalid priority is passed" do + prob1 = AdminDashboardData::Problem.new("test problem 1") + prob2 = AdminDashboardData::Problem.new("test problem 2", priority: "superbad") + expect(prob1.priority).to eq("low") + expect(prob2.priority).to eq("low") + end + end + + describe 'stats cache' do + include_examples 'stats cacheable' + end + + describe '#problem_message_check' do + let(:key) { AdminDashboardData.problem_messages.first } + + after do + described_class.clear_problem_message(key) + end + + it 'returns nil if message has not been added' do + expect(described_class.problem_message_check(key)).to be_nil + end + + it 'returns a message if it was added' do + described_class.add_problem_message(key) + expect(described_class.problem_message_check(key)).to eq(I18n.t(key, base_path: Discourse.base_path)) + end + + it 'returns a message if it was added with an expiry' do + described_class.add_problem_message(key, 300) + expect(described_class.problem_message_check(key)).to eq(I18n.t(key, base_path: Discourse.base_path)) end end @@ -220,32 +307,6 @@ describe AdminDashboardData do end end - describe 'stats cache' do - include_examples 'stats cacheable' - end - - describe '#problem_message_check' do - let(:key) { AdminDashboardData.problem_messages.first } - - before do - described_class.clear_problem_message(key) - end - - it 'returns nil if message has not been added' do - expect(described_class.problem_message_check(key)).to be_nil - end - - it 'returns a message if it was added' do - described_class.add_problem_message(key) - expect(described_class.problem_message_check(key)).to eq(I18n.t(key, base_path: Discourse.base_path)) - end - - it 'returns a message if it was added with an expiry' do - described_class.add_problem_message(key, 300) - expect(described_class.problem_message_check(key)).to eq(I18n.t(key, base_path: Discourse.base_path)) - end - end - describe '#out_of_date_themes' do let(:remote) { RemoteTheme.create!(remote_url: "https://github.com/org/testtheme") } let!(:theme) { Fabricate(:theme, remote_theme: remote, name: "Test< Theme") }