FIX: Fix scheduled targeted problem checks (#35696)

Scheduled problem checks with multiple targets are not honouring the
`run_every` configuration.

For checks with multiple targets, all targets are checked in a single
instance of the problem check. However, we have one problem check
tracker per target.

This mismatch results in the `#ready_to_run?` method always creating a
tracker with no target when being checked.

This commit fixes that by:

**Expect checks to operate on a single target.**

This change makes it so that instances of a `ProblemCheck` class are
initialized with a target. So instead of 1-N we now have an N-N
relationship between checks and trackers.

Each instance can access their `target` through an attribute of the same
name.

This also means problem checks are back to returning a singular
`Problem` or `nil`, instead of `[Problem]` or `[]`.

For scheduled checks, this means that `ScheduleProblemChecks` now
enqueues `N` jobs (where `N` is the number of targets) per check instead
of `1` job per check.

**Update existing targeted checks to operate on a single target.**

This is essentially just removing the loop inside the check.
This commit is contained in:
Ted Johansson
2025-11-10 10:09:14 +08:00
committed by GitHub
parent 177dcd8977
commit d446cc7318
19 changed files with 404 additions and 374 deletions
+6 -3
View File
@@ -12,12 +12,15 @@ module Jobs
def execute(args)
retry_count = args[:retry_count].to_i
identifier = args[:check_identifier].to_sym
target = args[:target].to_s
return if target.blank?
check = ProblemCheck[identifier]
check.run do |problems|
raise RetrySignal if problems.present? && retry_count < check.max_retries
end
check
.new(target)
.run { |problem| raise RetrySignal if problem.present? && retry_count < check.max_retries }
rescue RetrySignal
Jobs.enqueue_in(
check.retry_after,
+11 -9
View File
@@ -10,17 +10,19 @@ module Jobs
every 10.minutes
def execute(_args)
ProblemCheck.scheduled.filter_map do |check|
if eligible_for_this_run?(check)
Jobs.enqueue(:run_problem_check, check_identifier: check.identifier.to_s)
ProblemCheck.scheduled.filter_map do |scheduled_check|
scheduled_check.each_target do |target|
check = scheduled_check.new(target)
if check.enabled? && check.ready_to_run?
Jobs.enqueue(
:run_problem_check,
check_identifier: check.identifier.to_s,
target: target.to_s,
)
end
end
end
end
private
def eligible_for_this_run?(check)
check.enabled? && check.scheduled? && check.ready_to_run?
end
end
end
+48 -56
View File
@@ -13,7 +13,7 @@ class ProblemCheck
end
def run_all
select(&:enabled?).each(&:run)
select(&:enabled?).each { |check| check.each_target { |t| check.new(t).run } }
end
private
@@ -56,6 +56,11 @@ class ProblemCheck
#
config_accessor :inline, default: false, instance_writer: false
# Used to set up multiple targets for the check. For example, a check that
# operates on groups may need to specify which groups to work on.
#
config_accessor :targets, default: -> { [NO_TARGET] }, 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.
@@ -114,11 +119,6 @@ class ProblemCheck
Collection.new(checks.select(&:realtime?))
end
def self.tracker(target = NO_TARGET)
ProblemCheckTracker[identifier, target]
end
delegate :tracker, to: :class
def self.identifier
name.demodulize.underscore.to_sym
end
@@ -144,85 +144,77 @@ class ProblemCheck
end
delegate :inline?, to: :class
def self.ready_to_run?
tracker.ready_to_run?
def self.targeted?
targets.call != [ProblemCheck::NO_TARGET]
end
delegate :ready_to_run?, to: :class
delegate :targeted?, to: :class
def self.call(data = {})
new(data).call
def self.each_target(&)
targets.call.each(&)
end
def self.run(data = {}, &)
new(data).run(&)
def initialize(target = NO_TARGET)
@target = target
end
def initialize(data = {})
@data = OpenStruct.new(data)
end
attr_reader :data
attr_reader :target
def call
raise NotImplementedError
end
def run
problems = call
if targeted? && (target == NO_TARGET || targets.call.exclude?(target))
tracker.destroy
return
end
yield(problems) if block_given?
problem = call
yield(problem) if block_given?
next_run_at = perform_every&.from_now
if problems.empty?
targets.each { |t| tracker(t).no_problem!(next_run_at:) }
if problem.blank?
tracker.no_problem!(next_run_at:)
else
problems
.uniq(&:target)
.each do |problem|
problem_translation_data =
problem.target.present? ? translation_data(problem.target) : translation_data
tracker(problem.target).problem!(
next_run_at:,
details:
problem_translation_data.merge(problem.details).merge(base_path: Discourse.base_path),
)
end
tracker.problem!(
next_run_at:,
details: translation_data.merge(problem.details).merge(base_path: Discourse.base_path),
)
end
end
problems
def tracker
ProblemCheckTracker[identifier, target]
end
def ready_to_run?
tracker.ready_to_run?
end
private
def targets
[NO_TARGET]
end
def problem(target = nil, override_key: nil, override_data: {}, details: {})
target_identifier = target.kind_of?(ActiveRecord::Base) ? target.id : target
problem =
Problem.new(
I18n.t(
override_key || translation_key,
base_path: Discourse.base_path,
**override_data.merge(
target.present? ? translation_data(target) : translation_data,
).symbolize_keys,
),
priority: self.config.priority,
identifier:,
target: target_identifier,
details:,
)
target.present? ? problem : [problem]
Problem.new(
I18n.t(
override_key || translation_key,
base_path: Discourse.base_path,
**override_data.merge(
target.present? ? translation_data(target) : translation_data,
).symbolize_keys,
),
priority: self.config.priority,
identifier:,
target: target_identifier,
details:,
)
end
def no_problem
[]
nil
end
def translation_key
@@ -5,7 +5,6 @@ class ProblemCheck::ForceHttps < ProblemCheck
def call
return no_problem if SiteSetting.force_https
return no_problem if !data.check_force_https
problem
end
@@ -9,55 +9,54 @@
class ProblemCheck::GroupEmailCredentials < ProblemCheck
self.priority = "high"
self.perform_every = 30.minutes
self.targets = -> do
[*Group.with_smtp_configured.pluck(:name), *Group.with_imap_configured.pluck(:name)]
end
def call
[*smtp_errors, *imap_errors]
if group = Group.with_smtp_configured.find_by(name: target)
return no_problem if !SiteSetting.enable_smtp
return(
try_validate(group) do
EmailSettingsValidator.validate_smtp(
host: group.smtp_server,
port: group.smtp_port,
username: group.email_username,
password: group.email_password,
)
end
)
end
if group = Group.with_imap_configured.find_by(name: target)
return no_problem if !SiteSetting.enable_imap
return(
try_validate(group) do
EmailSettingsValidator.validate_imap(
host: group.imap_server,
port: group.imap_port,
username: group.email_username,
password: group.email_password,
)
end
)
end
no_problem
end
private
def targets
[*Group.with_smtp_configured.pluck(:name), *Group.with_imap_configured.pluck(:name)]
end
def translation_data(group)
{ group_name: group.name, group_full_name: group.full_name }
end
def smtp_errors
return [] if !SiteSetting.enable_smtp
Group.with_smtp_configured.find_each.filter_map do |group|
try_validate(group) do
EmailSettingsValidator.validate_smtp(
host: group.smtp_server,
port: group.smtp_port,
username: group.email_username,
password: group.email_password,
)
end
end
end
def imap_errors
return [] if !SiteSetting.enable_imap
Group.with_imap_configured.find_each.filter_map do |group|
try_validate(group) do
EmailSettingsValidator.validate_imap(
host: group.imap_server,
port: group.imap_port,
username: group.email_username,
password: group.email_password,
)
end
end
end
def try_validate(group, &blk)
begin
blk.call
nil
no_problem
rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS => err
error_message =
EmailSettingsExceptionHandler.friendly_exception_message(err, group.smtp_server)
@@ -69,7 +68,7 @@ class ProblemCheck::GroupEmailCredentials < ProblemCheck
message:
"Unexpected error when checking SMTP credentials for group #{group.id} (#{group.name}).",
)
nil
no_problem
end
end
end
@@ -5,7 +5,19 @@ class ProblemCheck::UpcomingChangeStableOptedOut < ProblemCheck
def call
return no_problem if !SiteSetting.enable_upcoming_changes
status_errors
# If the site setting is enabled, then the change is opted in, either
# manually or automatically, so we skip it.
return no_problem if SiteSetting.send(target)
# Don't care about any changes that are not yet stable, admins can opt
# in and out of these without worry.
return no_problem if UpcomingChanges.not_yet_stable?(target)
# At this point, we have an upcoming change that is stable or permanent,
# and the site is opted out of it. Admins need to know that the change
# will either become permanent or be removed soon.
problem(target)
end
private
@@ -17,23 +29,4 @@ class ProblemCheck::UpcomingChangeStableOptedOut < ProblemCheck
def targets
SiteSetting.upcoming_change_site_settings
end
def status_errors
targets
.map do |upcoming_change|
# If the site setting is enabled, then the change is opted in, either
# manually or automatically, so we skip it.
next if SiteSetting.send(upcoming_change)
# Don't care about any changes that are not yet stable, admins can opt
# in and out of these without worry.
next if UpcomingChanges.not_yet_stable?(upcoming_change)
# At this point, we have an upcoming change that is stable or permanent,
# and the site is opted out of it. Admins need to know that the change
# will either become permanent or be removed soon.
problem(upcoming_change)
end
.compact
end
end
@@ -3,32 +3,26 @@
class ProblemCheck::AiCreditHardLimit < ProblemCheck
self.priority = "high"
self.perform_every = 1.hour
self.targets = -> do
LlmModel.joins(:llm_credit_allocation).where("llm_models.id < 0").pluck("llm_models.id")
end
def call
return [] if !SiteSetting.discourse_ai_enabled
return no_problem if !SiteSetting.discourse_ai_enabled
problems = []
model = LlmModel.where("id < 0").includes(:llm_credit_allocation).find_by(id: target)
LlmModel
.where("id < 0")
.includes(:llm_credit_allocation)
.find_each do |model|
next unless model.llm_credit_allocation
return no_problem if model.llm_credit_allocation.blank?
allocation = model.llm_credit_allocation
allocation = model.llm_credit_allocation
problems << hard_limit_problem(model, allocation) if allocation.hard_limit_reached?
end
return no_problem if !allocation.hard_limit_reached?
problems.compact
hard_limit_problem(model, allocation)
end
private
def targets
LlmModel.joins(:llm_credit_allocation).where("llm_models.id < 0").pluck("llm_models.id")
end
def hard_limit_problem(model, allocation)
override_data = {
model_id: model.id,
@@ -3,34 +3,27 @@
class ProblemCheck::AiCreditSoftLimit < ProblemCheck
self.priority = "low"
self.perform_every = 1.hour
self.targets = -> do
LlmModel.joins(:llm_credit_allocation).where("llm_models.id < 0").pluck("llm_models.id")
end
def call
return [] if !SiteSetting.discourse_ai_enabled
return no_problem if !SiteSetting.discourse_ai_enabled
problems = []
model = LlmModel.where("id < 0").includes(:llm_credit_allocation).find_by(id: target)
LlmModel
.where("id < 0")
.includes(:llm_credit_allocation)
.find_each do |model|
next unless model.llm_credit_allocation
return no_problem if model.llm_credit_allocation.blank?
allocation = model.llm_credit_allocation
allocation = model.llm_credit_allocation
if allocation.soft_limit_reached? && !allocation.hard_limit_reached?
problems << soft_limit_problem(model, allocation)
end
end
return no_problem if !allocation.soft_limit_reached?
return no_problem if allocation.hard_limit_reached?
problems.compact
soft_limit_problem(model, allocation)
end
private
def targets
LlmModel.joins(:llm_credit_allocation).where("llm_models.id < 0").pluck("llm_models.id")
end
def soft_limit_problem(model, allocation)
override_data = {
model_id: model.id,
@@ -6,31 +6,25 @@ class ProblemCheck::AiLlmStatus < ProblemCheck
self.max_retries = 2
self.retry_after = 1.minute
self.max_blips = 2
self.targets = -> { LlmModel.in_use.pluck(:id) }
def call
return [] if !SiteSetting.discourse_ai_enabled
return no_problem if !SiteSetting.discourse_ai_enabled
llm_errors
model = LlmModel.in_use.find_by(id: target)
return no_problem if model.blank?
return no_problem if model.seeded?
try_validate(model) { validator.run_test(model) }
end
private
def targets
LlmModel.in_use.pluck(:id)
end
def llm_errors
return [] if !SiteSetting.discourse_ai_enabled
LlmModel.in_use.find_each.filter_map do |model|
next if model.seeded?
try_validate(model) { validator.run_test(model) }
end
end
def try_validate(model, &blk)
begin
blk.call
nil
no_problem
rescue => e
# Skip problem reporting for rate limiting and temporary service issues
# These are expected to resolve on their own
@@ -38,7 +32,7 @@ class ProblemCheck::AiLlmStatus < ProblemCheck
Rails.logger.info(
"AI LLM Status Check: Rate limit detected for model #{model.display_name} (#{model.id}), skipping problem report",
)
return nil
return no_problem
end
# Log transient errors but still return a problem
@@ -1,15 +1,17 @@
# frozen_string_literal: true
RSpec.describe ProblemCheck::AiCreditHardLimit do
subject(:check) { described_class.new(target) }
fab!(:llm_model) { Fabricate(:llm_model, id: -1) }
before { SiteSetting.discourse_ai_enabled = true }
describe "#call" do
it "returns no problems when no credit allocations exist" do
problems = described_class.new.call
let(:target) { llm_model.id }
expect(problems).to be_empty
it "returns no problems when no credit allocations exist" do
expect(check).to be_chill_about_it
end
it "returns no problems when credits are not exhausted" do
@@ -21,9 +23,7 @@ RSpec.describe ProblemCheck::AiCreditHardLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
it "returns hard limit problem when credits are exhausted" do
@@ -35,12 +35,7 @@ RSpec.describe ProblemCheck::AiCreditHardLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems.size).to eq(1)
expect(problems.first.identifier).to eq(:ai_credit_hard_limit)
expect(problems.first.priority).to eq("high")
expect(problems.first.target).to eq(llm_model.id)
expect(check).to have_a_problem.with_priority("high").with_target(llm_model.id)
end
it "returns hard limit problem when credits are over-exhausted" do
@@ -52,10 +47,7 @@ RSpec.describe ProblemCheck::AiCreditHardLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems.size).to eq(1)
expect(problems.first.identifier).to eq(:ai_credit_hard_limit)
expect(check).to have_a_problem.with_priority("high").with_target(llm_model.id)
end
it "does not report problem when previous month exceeded limit but current month is new" do
@@ -70,9 +62,9 @@ RSpec.describe ProblemCheck::AiCreditHardLimit do
)
freeze_time(Time.zone.parse("2025-11-05 10:00:00"))
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
allocation.reload
expect(allocation.monthly_used).to eq(0)
end
@@ -86,9 +78,7 @@ RSpec.describe ProblemCheck::AiCreditHardLimit do
monthly_used: 1000,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
it "returns no problems when discourse_ai is disabled" do
@@ -100,9 +90,7 @@ RSpec.describe ProblemCheck::AiCreditHardLimit do
monthly_used: 1000,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
end
end
@@ -1,15 +1,17 @@
# frozen_string_literal: true
RSpec.describe ProblemCheck::AiCreditSoftLimit do
subject(:check) { described_class.new(target) }
fab!(:llm_model) { Fabricate(:llm_model, id: -1) }
before { SiteSetting.discourse_ai_enabled = true }
describe "#call" do
it "returns no problems when no credit allocations exist" do
problems = described_class.new.call
let(:target) { llm_model.id }
expect(problems).to be_empty
it "returns no problems when no credit allocations exist" do
expect(check).to be_chill_about_it
end
it "returns no problems when credits are not at soft limit" do
@@ -21,9 +23,7 @@ RSpec.describe ProblemCheck::AiCreditSoftLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
it "returns soft limit problem when soft limit is reached" do
@@ -35,12 +35,7 @@ RSpec.describe ProblemCheck::AiCreditSoftLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems.size).to eq(1)
expect(problems.first.identifier).to eq(:ai_credit_soft_limit)
expect(problems.first.priority).to eq("low")
expect(problems.first.target).to eq(llm_model.id)
expect(check).to have_a_problem.with_priority("low").with_target(llm_model.id)
end
it "does not return soft limit problem when hard limit is reached" do
@@ -52,9 +47,7 @@ RSpec.describe ProblemCheck::AiCreditSoftLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
it "does not report problem when previous month exceeded limit but current month is new" do
@@ -69,9 +62,9 @@ RSpec.describe ProblemCheck::AiCreditSoftLimit do
)
freeze_time(Time.zone.parse("2025-11-05 10:00:00"))
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
allocation.reload
expect(allocation.monthly_used).to eq(0)
end
@@ -86,9 +79,7 @@ RSpec.describe ProblemCheck::AiCreditSoftLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
it "returns no problems when discourse_ai is disabled" do
@@ -101,9 +92,7 @@ RSpec.describe ProblemCheck::AiCreditSoftLimit do
soft_limit_percentage: 80,
)
problems = described_class.new.call
expect(problems).to be_empty
expect(check).to be_chill_about_it
end
end
end
@@ -1,7 +1,7 @@
# frozen_string_literal: true
RSpec.describe ProblemCheck::AiLlmStatus do
subject(:check) { described_class.new }
subject(:check) { described_class.new(target) }
fab!(:llm_model)
fab!(:ai_persona) { Fabricate(:ai_persona, default_llm_id: llm_model.id) }
@@ -23,6 +23,8 @@ RSpec.describe ProblemCheck::AiLlmStatus do
{ message: "API key error! Please check you have supplied the correct API key." }.to_json
end
let(:target) { llm_model.id }
before do
stub_request(:post, post_url).to_return(status: 200, body: success_response, headers: {})
assign_fake_provider_to(:ai_default_llm_model)
@@ -50,15 +52,10 @@ RSpec.describe ProblemCheck::AiLlmStatus do
},
)
expect(described_class.new.call.first).to have_attributes(
identifier: :ai_llm_status,
target: llm_model.id,
priority: "high",
message: message,
details: {
error: JSON.parse(error_response)["message"],
},
)
expect(check).to have_a_problem
.with_priority("high")
.with_target(llm_model.id)
.with_message(message)
end
it "does not return a problem if the LLM models are working" do
@@ -105,25 +102,13 @@ RSpec.describe ProblemCheck::AiLlmStatus do
it "reports problem for network timeout errors" do
stub_request(:post, post_url).to_timeout
problems = described_class.new.call
expect(problems.length).to eq(1)
expect(problems.first).to have_attributes(
identifier: :ai_llm_status,
target: llm_model.id,
priority: "high",
)
expect(check).to have_a_problem.with_priority("high").with_target(llm_model.id)
end
it "reports problem for authentication errors" do
stub_request(:post, post_url).to_return(status: 401, body: error_response, headers: {})
problems = described_class.new.call
expect(problems.length).to eq(1)
expect(problems.first).to have_attributes(
identifier: :ai_llm_status,
target: llm_model.id,
priority: "high",
)
expect(check).to have_a_problem.with_priority("high").with_target(llm_model.id)
end
end
end
+27 -15
View File
@@ -11,14 +11,7 @@ RSpec.describe Jobs::RunProblemCheck do
self.max_retries = 0
def call
[
ProblemCheck::Problem.new("Big problem"),
ProblemCheck::Problem.new(
"Yuge problem",
priority: "high",
identifier: "config_is_a_mess",
),
]
ProblemCheck::Problem.new("Big problem")
end
end
@@ -29,7 +22,11 @@ RSpec.describe Jobs::RunProblemCheck do
it "updates the problem check tracker" do
expect {
described_class.new.execute(check_identifier: "test_check", retry_count: 0)
described_class.new.execute(
check_identifier: "test_check",
retry_count: 0,
target: ProblemCheck::NO_TARGET,
)
}.to change { ProblemCheckTracker.failing.count }.by(1)
end
end
@@ -42,7 +39,7 @@ RSpec.describe Jobs::RunProblemCheck do
self.max_retries = 2
def call
[ProblemCheck::Problem.new("Yuge problem")]
ProblemCheck::Problem.new("Yuge problem")
end
end
@@ -53,7 +50,11 @@ RSpec.describe Jobs::RunProblemCheck do
it "does not yet update the problem check tracker" do
expect {
described_class.new.execute(check_identifier: "test_check", retry_count: 1)
described_class.new.execute(
check_identifier: "test_check",
retry_count: 1,
target: ProblemCheck::NO_TARGET,
)
}.not_to change { ProblemCheckTracker.where("blips > ?", 0).count }
end
@@ -63,8 +64,11 @@ RSpec.describe Jobs::RunProblemCheck do
args: {
check_identifier: "test_check",
retry_count: 1,
target: ProblemCheck::NO_TARGET,
},
) { described_class.new.execute(check_identifier: "test_check") }
) do
described_class.new.execute(check_identifier: "test_check", target: ProblemCheck::NO_TARGET)
end
end
end
@@ -76,7 +80,7 @@ RSpec.describe Jobs::RunProblemCheck do
self.max_retries = 1
def call
[ProblemCheck::Problem.new("Yuge problem")]
ProblemCheck::Problem.new("Yuge problem")
end
end
@@ -87,13 +91,21 @@ RSpec.describe Jobs::RunProblemCheck do
it "updates the problem check tracker" do
expect {
described_class.new.execute(check_identifier: "test_check", retry_count: 1)
described_class.new.execute(
check_identifier: "test_check",
retry_count: 1,
target: ProblemCheck::NO_TARGET,
)
}.to change { ProblemCheckTracker.where("blips > ?", 0).count }.by(1)
end
it "does not schedule a retry" do
expect_not_enqueued_with(job: :run_problem_check) do
described_class.new.execute(check_identifier: "test_check", retry_count: 1)
described_class.new.execute(
check_identifier: "test_check",
retry_count: 1,
target: ProblemCheck::NO_TARGET,
)
end
end
end
+53 -1
View File
@@ -19,16 +19,28 @@ RSpec.describe Jobs::RunProblemChecks do
def call = []
end
ProblemCheck::MultiTargetCheck =
Class.new(ProblemCheck) do
self.perform_every = 30.minutes
self.targets = -> { %w[foo bar] }
end
stub_const(
ProblemCheck,
"CORE_PROBLEM_CHECKS",
[ProblemCheck::ScheduledCheck, ProblemCheck::NonScheduledCheck, ProblemCheck::DisabledCheck],
[
ProblemCheck::ScheduledCheck,
ProblemCheck::NonScheduledCheck,
ProblemCheck::DisabledCheck,
ProblemCheck::MultiTargetCheck,
],
&example
)
ProblemCheck.send(:remove_const, "ScheduledCheck")
ProblemCheck.send(:remove_const, "NonScheduledCheck")
ProblemCheck.send(:remove_const, "DisabledCheck")
ProblemCheck.send(:remove_const, "MultiTargetCheck")
end
context "when a tracker hasn't been created yet" do
@@ -37,6 +49,7 @@ RSpec.describe Jobs::RunProblemChecks do
job: :run_problem_check,
args: {
check_identifier: "scheduled_check",
target: ProblemCheck::NO_TARGET,
},
) { described_class.new.execute([]) }
end
@@ -52,6 +65,7 @@ RSpec.describe Jobs::RunProblemChecks do
job: :run_problem_check,
args: {
check_identifier: "scheduled_check",
target: ProblemCheck::NO_TARGET,
},
) { described_class.new.execute([]) }
end
@@ -110,4 +124,42 @@ RSpec.describe Jobs::RunProblemChecks do
) { described_class.new.execute([]) }
end
end
context "when dealing with a multi-target check" do
it "schedules one check per target" do
expect_enqueued_with(
job: :run_problem_check,
args: {
check_identifier: "multi_target_check",
target: "foo",
},
) { described_class.new.execute([]) }
expect_enqueued_with(
job: :run_problem_check,
args: {
check_identifier: "multi_target_check",
target: "bar",
},
) { described_class.new.execute([]) }
end
it "creates a problem tracker for each target" do
expect { described_class.new.execute([]) }.to change {
ProblemCheckTracker.where(
identifier: "multi_target_check",
target: ProblemCheck::MultiTargetCheck.targets.call,
).count
}.by(2)
end
it "does not create any no-target tracker" do
expect { described_class.new.execute([]) }.not_to change {
ProblemCheckTracker.where(
identifier: "multi_target_check",
target: ProblemCheck::NO_TARGET,
).count
}
end
end
end
@@ -1,14 +1,13 @@
# frozen_string_literal: true
RSpec.describe ProblemCheck::ForceHttps do
subject(:check) { described_class.new(data) }
subject(:check) { described_class.new }
describe ".call" do
before { SiteSetting.stubs(force_https: configured) }
context "when configured to force SSL" do
let(:configured) { true }
let(:data) { { check_force_https: true } }
it { expect(check).to be_chill_about_it }
end
@@ -16,20 +15,10 @@ RSpec.describe ProblemCheck::ForceHttps do
context "when not configured to force SSL" do
let(:configured) { false }
context "when the request is coming over HTTPS" do
let(:data) { { check_force_https: true } }
it do
expect(check).to have_a_problem.with_priority("low").with_message(
"Your website is using SSL. But `<a href='/admin/site_settings/category/all_results?filter=force_https'>force_https</a>` is not yet enabled in your site settings.",
)
end
end
context "when the request is coming over HTTP" do
let(:data) { { check_force_https: false } }
it { expect(check).to be_chill_about_it }
it do
expect(check).to have_a_problem.with_priority("low").with_message(
"Your website is using SSL. But `<a href='/admin/site_settings/category/all_results?filter=force_https'>force_https</a>` is not yet enabled in your site settings.",
)
end
end
end
@@ -4,104 +4,96 @@ require "net/smtp"
require "net/imap"
RSpec.describe ProblemCheck::GroupEmailCredentials do
subject(:check) { described_class.new }
subject(:check) { described_class.new(target) }
fab!(:group1, :group)
fab!(:group2, :smtp_group)
fab!(:group3, :imap_group)
fab!(:group1) { Fabricate(:group, smtp_enabled: false, imap_enabled: false) }
fab!(:smtp_group) { Fabricate(:smtp_group, name: "smtp_group", imap_enabled: true) }
fab!(:imap_group) { Fabricate(:imap_group, name: "imap_group", smtp_enabled: false) }
describe "#call" do
it "does nothing if SMTP is disabled for the site" do
expect_no_validate_any
SiteSetting.enable_smtp = false
expect(check).to be_chill_about_it
context "when SMTP is disabled for the site" do
before { SiteSetting.enable_smtp = false }
context "with an SMTP-enabled group" do
let(:target) { smtp_group.name }
it "does not report a problem" do
expect(check).to be_chill_about_it
end
end
end
context "with smtp and imap enabled for the site" do
context "when IMAP is disabled for the site" do
before { SiteSetting.enable_imap = false }
context "with an IMAP-enabled group" do
let(:target) { imap_group.name }
it "does not report a problem" do
expect(check).to be_chill_about_it
end
end
end
context "when SMTP and IMAP are enabled for the site" do
before do
SiteSetting.enable_smtp = true
SiteSetting.enable_imap = true
end
it "does nothing if no groups have smtp enabled" do
expect_no_validate_any
group2.update!(smtp_enabled: false)
group3.update!(smtp_enabled: false, imap_enabled: false)
expect(check).to be_chill_about_it
context "when the group has no SMTP or IMAP enabled" do
let(:target) { group1.name }
it "does not report a problem" do
expect(check).to be_chill_about_it
end
end
it "returns a problem with the group's SMTP settings error" do
EmailSettingsValidator
.expects(:validate_smtp)
.raises(Net::SMTPAuthenticationError.new("bad credentials"))
.then
.returns(true)
.at_least_once
EmailSettingsValidator.stubs(:validate_imap).returns(true)
context "when SMTP error check fails" do
let(:target) { smtp_group.name }
expect(described_class.new.call).to contain_exactly(
have_attributes(
identifier: :group_email_credentials,
target: group2.id,
priority: "high",
message:
I18n.t(
"dashboard.problem.group_email_credentials",
base_path: Discourse.base_path,
group_name: group2.name,
group_full_name: group2.full_name,
error:
I18n.t("email_settings.smtp_authentication_error", message: "bad credentials"),
),
),
)
it "registers a problem with the group's SMTP settings error" do
EmailSettingsValidator
.expects(:validate_smtp)
.raises(Net::SMTPAuthenticationError.new("bad credentials"))
.then
.returns(true)
.at_least_once
EmailSettingsValidator.stubs(:validate_imap).returns(true)
expect(check).to have_a_problem.with_priority("high").with_message(
I18n.t(
"dashboard.problem.group_email_credentials",
base_path: Discourse.base_path,
group_name: smtp_group.name,
group_full_name: smtp_group.full_name,
error: I18n.t("email_settings.smtp_authentication_error", message: "bad credentials"),
),
)
end
end
it "returns an error message and the group ID if the group's IMAP settings error" do
EmailSettingsValidator.stubs(:validate_smtp).returns(true)
EmailSettingsValidator
.expects(:validate_imap)
.raises(Net::IMAP::NoResponseError.new(stub(data: stub(text: "Invalid credentials"))))
.once
context "when IMAP error check fails" do
let(:target) { imap_group.name }
expect(described_class.new.call).to contain_exactly(
have_attributes(
identifier: :group_email_credentials,
target: group3.id,
priority: "high",
message:
I18n.t(
"dashboard.problem.group_email_credentials",
base_path: Discourse.base_path,
group_name: group3.name,
group_full_name: group3.full_name,
error:
I18n.t("email_settings.imap_authentication_error", message: "bad credentials"),
),
),
)
end
it "registers a problem with the group's IMAP settings error" do
EmailSettingsValidator.stubs(:validate_smtp).returns(true)
EmailSettingsValidator
.expects(:validate_imap)
.raises(Net::IMAP::NoResponseError.new(stub(data: stub(text: "Invalid credentials"))))
.once
it "returns no imap errors if imap is disabled for the site" do
SiteSetting.enable_imap = false
EmailSettingsValidator.stubs(:validate_smtp).returns(true)
EmailSettingsValidator.expects(:validate_imap).never
expect(described_class.new.call).to eq([])
expect(check).to have_a_problem.with_priority("high").with_message(
I18n.t(
"dashboard.problem.group_email_credentials",
base_path: Discourse.base_path,
group_name: imap_group.name,
group_full_name: imap_group.full_name,
error: I18n.t("email_settings.imap_authentication_error", message: "bad credentials"),
),
)
end
end
end
end
def expect_no_validate_imap
EmailSettingsValidator.expects(:validate_imap).never
end
def expect_no_validate_smtp
EmailSettingsValidator.expects(:validate_smtp).never
end
def expect_no_validate_any
expect_no_validate_smtp
expect_no_validate_imap
end
end
@@ -1,9 +1,11 @@
# frozen_string_literal: true
RSpec.describe ProblemCheck::UpcomingChangeStableOptedOut do
subject(:check) { described_class.new }
subject(:check) { described_class.new(target) }
describe ".call" do
let(:target) { "enable_upload_debug_mode" }
before do
mock_upcoming_change_metadata(
{
@@ -33,7 +35,11 @@ RSpec.describe ProblemCheck::UpcomingChangeStableOptedOut do
end
context "when upcoming change is stable and not opted in" do
it { expect(check).to have_a_problem }
it do
expect(check).to have_a_problem.with_priority("low").with_target(
"enable_upload_debug_mode",
)
end
end
context "when upcoming change is not yet stable and not opted in" do
@@ -65,7 +71,11 @@ RSpec.describe ProblemCheck::UpcomingChangeStableOptedOut do
)
end
it { expect(check).to have_a_problem }
it do
expect(check).to have_a_problem.with_priority("low").with_target(
"enable_upload_debug_mode",
)
end
end
end
end
+40 -3
View File
@@ -7,6 +7,7 @@ RSpec.describe ProblemCheck do
InlineCheck = Class.new(described_class) { self.inline = true }
PluginCheck = Class.new(described_class)
DisabledCheck = Class.new(described_class) { self.enabled = false }
MultiTargetCheck = Class.new(described_class) { self.targets = -> { %w[foo bar] } }
FailingCheck =
Class.new(described_class) do
def call
@@ -31,7 +32,15 @@ RSpec.describe ProblemCheck do
stub_const(
described_class,
"CORE_PROBLEM_CHECKS",
[ScheduledCheck, RealtimeCheck, InlineCheck, DisabledCheck, FailingCheck, PassingCheck],
[
ScheduledCheck,
RealtimeCheck,
InlineCheck,
DisabledCheck,
MultiTargetCheck,
FailingCheck,
PassingCheck,
],
&example
)
@@ -39,6 +48,7 @@ RSpec.describe ProblemCheck do
Object.send(:remove_const, RealtimeCheck.name)
Object.send(:remove_const, InlineCheck.name)
Object.send(:remove_const, DisabledCheck.name)
Object.send(:remove_const, MultiTargetCheck.name)
Object.send(:remove_const, PluginCheck.name)
Object.send(:remove_const, FailingCheck.name)
Object.send(:remove_const, PassingCheck.name)
@@ -49,6 +59,7 @@ RSpec.describe ProblemCheck do
let(:inline_check) { InlineCheck }
let(:enabled_check) { RealtimeCheck }
let(:disabled_check) { DisabledCheck }
let(:multi_target_check) { MultiTargetCheck }
let(:plugin_check) { PluginCheck }
let(:failing_check) { FailingCheck }
let(:passing_check) { PassingCheck }
@@ -101,6 +112,11 @@ RSpec.describe ProblemCheck do
it { expect(disabled_check).not_to be_enabled }
end
describe ".targeted?" do
it { expect(scheduled_check).not_to be_targeted }
it { expect(multi_target_check).to be_targeted }
end
describe "plugin problem check registration" do
before { DiscoursePluginRegistry.register_problem_check(PluginCheck, stub(enabled?: enabled)) }
@@ -121,11 +137,32 @@ RSpec.describe ProblemCheck do
describe "#run" do
context "when check is failing" do
it { expect { failing_check.run }.to change { ProblemCheckTracker.failing.count }.by(1) }
it { expect { failing_check.new.run }.to change { ProblemCheckTracker.failing.count }.by(1) }
end
context "when check is passing" do
it { expect { passing_check.run }.to change { ProblemCheckTracker.passing.count }.by(1) }
it { expect { passing_check.new.run }.to change { ProblemCheckTracker.passing.count }.by(1) }
end
context "when targeted check has a no-target tracker" do
before do
ProblemCheckTracker.create!(
identifier: "multi_target_check",
target: ProblemCheck::NO_TARGET,
)
end
it "deletes the tracker" do
expect { multi_target_check.new.run }.to change { ProblemCheckTracker.count }.by(-1)
end
end
context "when targeted check has an outdated target" do
before { ProblemCheckTracker.create!(identifier: "multi_target_check", target: "baz") }
it "deletes the tracker" do
expect { multi_target_check.new("baz").run }.to change { ProblemCheckTracker.count }.by(-1)
end
end
end
end
+18 -11
View File
@@ -1,7 +1,7 @@
# frozen_string_literal: true
RSpec::Matchers.define :be_chill_about_it do
match { |service| expect(service.call).to be_empty }
match { |service| expect(service.call).to be_blank }
end
RSpec::Matchers.define :have_a_problem do
@@ -13,24 +13,29 @@ RSpec::Matchers.define :have_a_problem do
@priority = priority
end
chain :with_target do |target|
@target = target
end
match do |service|
@result = service.call
aggregate_failures do
expect(@result).to include(be_a(ProblemCheck::Problem))
expect(@result.first.priority).to(eq(@priority.to_s)) if @priority.present?
expect(@result.first.message).to(eq(@message)) if @message.present?
expect(@result).to be_a(ProblemCheck::Problem)
expect(@result.priority).to(eq(@priority.to_s)) if @priority.present?
expect(@result.message).to(eq(@message)) if @message.present?
expect(@result.target).to(eq(@target)) if @target.present?
end
end
failure_message do |service|
if @result.empty?
if @result.blank?
"Expected check to have a problem, but it was chill about it."
elsif !@result.all?(ProblemCheck::Problem)
"Expected result to contain only instances of `Problem`."
elsif @priority.present? && @result.first.priority != @priority
"Expected problem to have priority `#{@priority}`, but got priority `#{@result.first.priority}`."
elsif @message.present? && @result.first.message != @message
elsif !@result.is_a?(ProblemCheck::Problem)
"Expected result to must be an instance of `Problem`."
elsif @priority.present? && @result.priority != @priority
"Expected problem to have priority `#{@priority}`, but got priority `#{@result.priority}`."
elsif @message.present? && @result.message != @message
<<~MESSAGE
Expected problem to have message:
@@ -38,8 +43,10 @@ RSpec::Matchers.define :have_a_problem do
but got message:
> #{@result.first.message}
> #{@result.message}
MESSAGE
elsif @target.present? && @result.target != @target
"Expected problem to have target `#{@target}`, but got target `#{@result.target}`."
end
end
end