diff --git a/lib/discourse_hub.rb b/lib/discourse_hub.rb index 38e5c101a17..a4b9db04006 100644 --- a/lib/discourse_hub.rb +++ b/lib/discourse_hub.rb @@ -62,17 +62,25 @@ module DiscourseHub }.merge(connect_opts) ) - if response.status != 200 - Rails.logger.warn("Discourse Hub (#{hub_base_url}#{rel_url}) returned a bad status #{response.status}.") + if (status = response.status) != 200 + Rails.logger.warn(response_status_log_message(rel_url, status)) end begin JSON.parse(response.body) rescue JSON::ParserError - Rails.logger.error("Discourse Hub returned a bad response body: " + response.body) + Rails.logger.error(response_body_log_message(response.body)) end end + def self.response_status_log_message(rel_url, status) + "Discourse Hub (#{hub_base_url}#{rel_url}) returned a bad status #{status}." + end + + def self.response_body_log_message(body) + "Discourse Hub returned a bad response body: #{body}" + end + def self.connect_opts(params = {}) params.delete(:connect_opts)&.except(:body, :headers, :query) || {} end diff --git a/spec/components/discourse_hub_spec.rb b/spec/components/discourse_hub_spec.rb index a42255b508c..31b0c9651ba 100644 --- a/spec/components/discourse_hub_spec.rb +++ b/spec/components/discourse_hub_spec.rb @@ -77,23 +77,28 @@ describe DiscourseHub do end describe '.collection_action' do - - it 'should log a warning if status is not 200' do - stub_request(:get, (ENV['HUB_BASE_URL'] || "http://local.hub:3000/api")). - to_return(status: 500, body: "", headers: {}) - - Rails.logger.expects(:warn) - - DiscourseHub.collection_action(:get, "") + before do + @orig_logger = Rails.logger + Rails.logger = @fake_logger = FakeLogger.new end - it 'should log an error if response is invalid JSON' do - stub_request(:get, (ENV['HUB_BASE_URL'] || "http://local.hub:3000/api")). - to_return(status: 200, body: "this is not valid JSON", headers: {}) + after do + Rails.logger = @orig_logger + end - Rails.logger.expects(:error) + it 'should log correctly on error' do + stub_request(:get, (ENV['HUB_BASE_URL'] || "http://local.hub:3000/api/test")). + to_return(status: 500, body: "", headers: {}) - DiscourseHub.collection_action(:get, "") + DiscourseHub.collection_action(:get, '/test') + + expect(Rails.logger.warnings).to eq([ + DiscourseHub.response_status_log_message('/test', 500), + ]) + + expect(Rails.logger.errors).to eq([ + DiscourseHub.response_body_log_message("") + ]) end end end diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb index 0d0e5df3742..4a084c2d442 100644 --- a/spec/components/discourse_spec.rb +++ b/spec/components/discourse_spec.rb @@ -275,15 +275,6 @@ describe Discourse do end context '#deprecate' do - - class FakeLogger - attr_reader :warnings - def warn(m) - @warnings ||= [] - @warnings << m - end - end - def old_method(m) Discourse.deprecate(m) end @@ -307,7 +298,7 @@ describe Discourse do expect(old_method_caller(k)).to include("discourse_spec") expect(old_method_caller(k)).to include(k) - expect(@fake_logger.warnings).to eq([old_method_caller(k)]) + expect(Rails.logger.warnings).to eq([old_method_caller(k)]) end end diff --git a/spec/components/email/processor_spec.rb b/spec/components/email/processor_spec.rb index c953400f524..c31a70fa7b2 100644 --- a/spec/components/email/processor_spec.rb +++ b/spec/components/email/processor_spec.rb @@ -103,14 +103,27 @@ describe Email::Processor do let(:mail2) { "From: #{from}\nTo: foo@foo.com\nSubject: BAR BAR\n\nBar bar bar bar?" } it "sends a rejection email on an unrecognized error" do - Email::Processor.any_instance.stubs(:can_send_rejection_email?).returns(true) - Email::Receiver.any_instance.stubs(:process_internal).raises("boom") - Rails.logger.expects(:error) + begin + @orig_logger = Rails.logger + Rails.logger = @fake_logger = FakeLogger.new - Email::Processor.process!(mail) - expect(IncomingEmail.last.error).to eq("boom") - expect(IncomingEmail.last.rejection_message).to be_present - expect(EmailLog.last.email_type).to eq("email_reject_unrecognized_error") + Email::Processor.any_instance.stubs(:can_send_rejection_email?).returns(true) + Email::Receiver.any_instance.stubs(:process_internal).raises("boom") + + Email::Processor.process!(mail) + + errors = Rails.logger.errors + expect(errors.size).to eq(1) + expect(errors.first).to include("boom") + + incoming_email = IncomingEmail.last + expect(incoming_email.error).to eq("boom") + expect(incoming_email.rejection_message).to be_present + + expect(EmailLog.last.email_type).to eq("email_reject_unrecognized_error") + ensure + Rails.logger = @orig_logger + end end it "sends more than one rejection email per day" do diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index 57d2279789d..7fb350a7194 100644 --- a/spec/models/report_spec.rb +++ b/spec/models/report_spec.rb @@ -814,18 +814,27 @@ describe Report do end describe "unexpected error on report initialization" do + before do + @orig_logger = Rails.logger + Rails.logger = @fake_logger = FakeLogger.new + end + + after do + Rails.logger = @orig_logger + end + it "returns no report" do class ReportInitError < StandardError; end Report.stubs(:new).raises(ReportInitError.new("x")) - Rails.logger.expects(:error) - .with('Couldn’t create report `signups`: ') - .once - report = Report.find('signups') expect(report).to be_nil + + expect(Rails.logger.errors).to eq([ + 'Couldn’t create report `signups`: ' + ]) end end diff --git a/spec/support/fake_logger.rb b/spec/support/fake_logger.rb new file mode 100644 index 00000000000..416de400dcd --- /dev/null +++ b/spec/support/fake_logger.rb @@ -0,0 +1,16 @@ +class FakeLogger + attr_reader :warnings, :errors + + def initialize + @warnings = [] + @errors = [] + end + + def warn(message) + @warnings << message + end + + def error(message) + @errors << message + end +end