Skip to content

Commit

Permalink
DEV: Move non scheduled problem checks to classes (discourse#26122)
Browse files Browse the repository at this point in the history 
In AdminDashboardData we have a bunch of problem checks implemented as methods on that class. This PR absolves it of the responsibility by promoting each of those checks to a first class ProblemCheck. This way each of them can have their own priority and arbitrary functionality can be isolated in its own class.

Think "extract class" refactoring over and over. Since they were all moved we can also get rid of the @@problem_syms class variable which was basically the old version of the registry now replaced by ProblemCheck.realtime.

In addition AdminDashboardData::Problem value object has been entirely replaced with the new ProblemCheck::Problem (with compatible API).

Lastly, I added some RSpec matchers to simplify testing of problem checks and provide helpful error messages when assertions fail.
  • Loading branch information
@Drenmi
Drenmi authored Mar 14, 2024
1 parent 9afb0b2 commit ea5c3a3
Show file tree
Hide file tree
Showing 51 changed files with 1,447 additions and 477 deletions.
232 changes: 6 additions & 226 deletions app/models/admin_dashboard_data.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,7 @@
class AdminDashboardData
include StatsCacheable

cattr_reader :problem_syms, :problem_blocks, :problem_messages

class Problem
VALID_PRIORITIES = %w[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
cattr_reader :problem_blocks, :problem_messages

# kept for backward compatibility
GLOBAL_REPORTS ||= []
Expand All @@ -51,18 +25,16 @@ def as_json(_options = nil)

def problems
problems = []
self.class.problem_syms.each do |sym|
message = public_send(sym)
problems << Problem.new(message) if message.present?
end
self.class.problem_blocks.each do |blk|
message = instance_exec(&blk)
problems << Problem.new(message) if message.present?
problems << ProblemCheck::Problem.new(message) if message.present?
end
self.class.problem_messages.each do |i18n_key|
message = self.class.problem_message_check(i18n_key)
problems << Problem.new(message) if message.present?
problems << ProblemCheck::Problem.new(message) if message.present?
end
problems.concat(ProblemCheck.realtime.flat_map { |c| c.call(@opts).map(&:to_h) })

problems += self.class.load_found_scheduled_check_problems
problems.compact!

Expand All @@ -76,7 +48,6 @@ def problems
end

def self.add_problem_check(*syms, &blk)
@@problem_syms.push(*syms) if syms
@@problem_blocks << blk if blk
end

Expand Down Expand Up @@ -109,7 +80,7 @@ def self.load_found_scheduled_check_problems

found_problems.filter_map do |problem|
begin
Problem.from_h(JSON.parse(problem))
ProblemCheck::Problem.from_h(JSON.parse(problem))
rescue JSON::ParserError => err
Discourse.warn_exception(
err,
Expand All @@ -130,7 +101,6 @@ def self.load_found_scheduled_check_problems
# 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_messages = %w[
Expand All @@ -139,26 +109,6 @@ def self.reset_problem_checks
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,
:google_analytics_version_check,
:translation_overrides_check

add_problem_check { sidekiq_check || queue_size_check }
end
reset_problem_checks
Expand Down Expand Up @@ -226,16 +176,6 @@ def self.problem_message_key(i18n_key)
"#{PROBLEM_MESSAGE_PREFIX}#{i18n_key}"
end

def rails_env_check
I18n.t("dashboard.rails_env_warning", env: Rails.env) unless Rails.env.production?
end

def host_names_check
if %w[localhost production.localhost].include?(Discourse.current_hostname)
I18n.t("dashboard.host_names_warning")
end
end

def sidekiq_check
last_job_performed_at = Jobs.last_job_performed_at
if Jobs.queued > 0 && (last_job_performed_at.nil? || last_job_performed_at < 2.minutes.ago)
Expand All @@ -247,164 +187,4 @@ def queue_size_check
queue_size = Jobs.queued
I18n.t("dashboard.queue_size_warning", queue_size: queue_size) if queue_size >= 100_000
end

def ram_check
I18n.t("dashboard.memory_warning") if MemInfo.new.mem_total && MemInfo.new.mem_total < 950_000
end

def google_oauth2_config_check
if SiteSetting.enable_google_oauth2_logins &&
(
SiteSetting.google_oauth2_client_id.blank? ||
SiteSetting.google_oauth2_client_secret.blank?
)
I18n.t("dashboard.google_oauth2_config_warning", base_path: Discourse.base_path)
end
end

def facebook_config_check
if SiteSetting.enable_facebook_logins &&
(SiteSetting.facebook_app_id.blank? || SiteSetting.facebook_app_secret.blank?)
I18n.t("dashboard.facebook_config_warning", base_path: Discourse.base_path)
end
end

def twitter_config_check
if SiteSetting.enable_twitter_logins &&
(SiteSetting.twitter_consumer_key.blank? || SiteSetting.twitter_consumer_secret.blank?)
I18n.t("dashboard.twitter_config_warning", base_path: Discourse.base_path)
end
end

def github_config_check
if SiteSetting.enable_github_logins &&
(SiteSetting.github_client_id.blank? || SiteSetting.github_client_secret.blank?)
I18n.t("dashboard.github_config_warning", base_path: Discourse.base_path)
end
end

def s3_config_check
# if set via global setting it is validated during the `use_s3?` call
if !GlobalSetting.use_s3?
bad_keys =
(SiteSetting.s3_access_key_id.blank? || SiteSetting.s3_secret_access_key.blank?) &&
!SiteSetting.s3_use_iam_profile

if SiteSetting.enable_s3_uploads && (bad_keys || SiteSetting.s3_upload_bucket.blank?)
return I18n.t("dashboard.s3_config_warning", base_path: Discourse.base_path)
end

if SiteSetting.backup_location == BackupLocationSiteSetting::S3 &&
(bad_keys || SiteSetting.s3_backup_bucket.blank?)
return I18n.t("dashboard.s3_backup_config_warning", base_path: Discourse.base_path)
end
end
nil
end

def s3_cdn_check
if (GlobalSetting.use_s3? || SiteSetting.enable_s3_uploads) &&
SiteSetting.Upload.s3_cdn_url.blank?
I18n.t("dashboard.s3_cdn_warning")
end
end

def translation_overrides_check
if TranslationOverride.exists?(status: %i[outdated invalid_interpolation_keys])
I18n.t("dashboard.outdated_translations_warning", base_path: Discourse.base_path)
end
end

def image_magick_check
if SiteSetting.create_thumbnails && !system("command -v convert >/dev/null;")
I18n.t("dashboard.image_magick_warning")
end
end

def failing_emails_check
num_failed_jobs = Jobs.num_email_retry_jobs
if num_failed_jobs > 0
I18n.t(
"dashboard.failing_emails_warning",
num_failed_jobs: num_failed_jobs,
base_path: Discourse.base_path,
)
end
end

def subfolder_ends_in_slash_check
I18n.t("dashboard.subfolder_ends_in_slash") if Discourse.base_path =~ %r{/\z}
end

def google_analytics_version_check
I18n.t("dashboard.v3_analytics_deprecated") if SiteSetting.ga_version == "v3_analytics"
end

def email_polling_errored_recently
errors = Jobs::PollMailbox.errors_in_past_24_hours
if errors > 0
I18n.t(
"dashboard.email_polling_errored_recently",
count: errors,
base_path: Discourse.base_path,
)
end
end

def missing_mailgun_api_key
return unless SiteSetting.reply_by_email_enabled
return unless ActionMailer::Base.smtp_settings[:address]["smtp.mailgun.org"]
return unless SiteSetting.mailgun_api_key.blank?
I18n.t("dashboard.missing_mailgun_api_key")
end

def force_https_check
return unless @opts[:check_force_https]
unless SiteSetting.force_https
I18n.t("dashboard.force_https_warning", base_path: Discourse.base_path)
end
end

def watched_words_check
WatchedWord.actions.keys.each do |action|
begin
WordWatcher.compiled_regexps_for_action(action, raise_errors: true)
rescue RegexpError => e
translated_action = I18n.t("admin_js.admin.watched_words.actions.#{action}")
I18n.t(
"dashboard.watched_word_regexp_error",
base_path: Discourse.base_path,
action: translated_action,
)
end
end
nil
end

def out_of_date_themes
old_themes = RemoteTheme.out_of_date_themes
return unless old_themes.present?

themes_html_format(old_themes, "dashboard.out_of_date_themes")
end

def unreachable_themes
themes = RemoteTheme.unreachable_themes
return unless themes.present?

themes_html_format(themes, "dashboard.unreachable_themes")
end

private

def themes_html_format(themes, i18n_key)
html =
themes
.map do |name, id|
"<li><a href=\"/admin/customize/themes/#{id}\">#{CGI.escapeHTML(name)}</a></li>"
end
.join("\n")

"#{I18n.t(i18n_key)}<ul>#{html}</ul>"
end
end
36 changes: 32 additions & 4 deletions app/models/problem_check.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ def self.scheduled
checks.select(&:scheduled?)
end

def self.realtime
checks.reject(&:scheduled?)
end

def self.identifier
name.demodulize.underscore.to_sym
end
Expand All @@ -45,20 +49,36 @@ def self.scheduled?
end
delegate :scheduled?, to: :class

def self.call
new.call
def self.realtime?
!scheduled?
end
delegate :realtime?, to: :class

def self.call(data = {})
new(data).call
end

def initialize(data = {})
@data = OpenStruct.new(data)
end

attr_reader :data

def call
raise NotImplementedError
end

private

def problem
def problem(override_key = nil)
[
Problem.new(
I18n.t(translation_key, base_path: Discourse.base_path),
message ||
I18n.t(
override_key || translation_key,
base_path: Discourse.base_path,
**translation_data.symbolize_keys,
),
priority: self.config.priority,
identifier:,
),
Expand All @@ -69,8 +89,16 @@ def no_problem
[]
end

def message
nil
end

def translation_key
# TODO: Infer a default based on class name, then move translations in locale file.
raise NotImplementedError
end

def translation_data
{}
end
end
25 changes: 25 additions & 0 deletions app/services/problem_check/email_polling_errored_recently.rb
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

class ProblemCheck::EmailPollingErroredRecently < ProblemCheck
self.priority = "low"

def call
return no_problem if polling_error_count.to_i == 0

problem
end

private

def polling_error_count
@polling_error_count ||= Jobs::PollMailbox.errors_in_past_24_hours
end

def translation_key
"dashboard.email_polling_errored_recently"
end

def translation_data
{ count: polling_error_count }
end
end
Loading

0 comments on commit ea5c3a3

Please sign in to comment.