From 9d30f007a9795323185476fadd49c25f21f944ba Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 12:36:00 +1100 Subject: [PATCH 1/9] Format spec file --- .../dfc_provider/spec/services/dfc_request_spec.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/engines/dfc_provider/spec/services/dfc_request_spec.rb b/engines/dfc_provider/spec/services/dfc_request_spec.rb index 0e6fbfb8d3..47c37a6b83 100644 --- a/engines/dfc_provider/spec/services/dfc_request_spec.rb +++ b/engines/dfc_provider/spec/services/dfc_request_spec.rb @@ -38,15 +38,10 @@ RSpec.describe DfcRequest do account.refresh_token = ENV.fetch("OPENID_REFRESH_TOKEN") account.updated_at = 1.day.ago - expect { - api.call("http://example.net/api") - } + expect { api.call("http://example.net/api") } .to raise_error(Faraday::UnauthorizedError) - .and change { - account.token - }.and change { - account.refresh_token - } + .and change { account.token } + .and change { account.refresh_token } end it "doesn't try to refresh the token when it's still fresh" do From 710e1654d011a7ac69b5a3b6c928c8c7261032c4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 13:01:05 +1100 Subject: [PATCH 2/9] Report DFC server errors to Bugsnag --- engines/dfc_provider/app/services/dfc_request.rb | 3 +++ engines/dfc_provider/spec/services/dfc_request_spec.rb | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/engines/dfc_provider/app/services/dfc_request.rb b/engines/dfc_provider/app/services/dfc_request.rb index c976a69787..541505f8e3 100644 --- a/engines/dfc_provider/app/services/dfc_request.rb +++ b/engines/dfc_provider/app/services/dfc_request.rb @@ -22,6 +22,9 @@ class DfcRequest # If access was denied and our token is stale then refresh and retry: refresh_access_token! response = request(url, data, method:) + rescue Faraday::ServerError => e + Alert.raise(e, { dfc_request: { data: } }) + raise end response.body diff --git a/engines/dfc_provider/spec/services/dfc_request_spec.rb b/engines/dfc_provider/spec/services/dfc_request_spec.rb index 47c37a6b83..173367f410 100644 --- a/engines/dfc_provider/spec/services/dfc_request_spec.rb +++ b/engines/dfc_provider/spec/services/dfc_request_spec.rb @@ -81,4 +81,13 @@ RSpec.describe DfcRequest do products = graph.select { |s| s.semanticType == "dfc-b:SuppliedProduct" } expect(products).to be_present end + + it "reports and raises server errors" do + stub_request(:get, "http://example.net/api").to_return(status: 500) + + expect(Bugsnag).to receive(:notify) + + expect { api.call("http://example.net/api") } + .to raise_error(Faraday::ServerError) + end end From b50e711757870d2eecc6bb256099187f396aeaae Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 13:32:51 +1100 Subject: [PATCH 3/9] Add deployed version to Bugsnag --- config/initializers/bugsnag.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/initializers/bugsnag.rb b/config/initializers/bugsnag.rb index 572681f1db..c67725fcf6 100644 --- a/config/initializers/bugsnag.rb +++ b/config/initializers/bugsnag.rb @@ -1,6 +1,8 @@ Bugsnag.configure do |config| config.api_key = ENV['BUGSNAG_API_KEY'] config.release_stage = Rails.env + config.app_version = Rails.application.config.x.git_version + # Avoid missing API key warning without changing the Rails log level. if Rails.env.development? config.logger = Logger.new(STDOUT) From cc6a3f4e5b0ed261c67de7b6d682e07463672c1e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 14:33:17 +1100 Subject: [PATCH 4/9] Use Bugsnag key with valid format in specs That's why Bugsnag wasn't active in CI. --- spec/services/alert_spec.rb | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/spec/services/alert_spec.rb b/spec/services/alert_spec.rb index 2e8723fd8f..bc31d7686c 100644 --- a/spec/services/alert_spec.rb +++ b/spec/services/alert_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Alert do original_config = nil Bugsnag.configure do |config| original_config = config.dup - config.api_key ||= "dummy-key" + config.api_key ||= "00000000000000000000000000000000" config.notify_release_stages = ["test"] config.delivery_method = :synchronous end @@ -15,7 +15,7 @@ RSpec.describe Alert do example.run Bugsnag.configure do |config| - config.api_key ||= original_config.api_key + config.api_key = original_config.api_key config.notify_release_stages = original_config.notify_release_stages config.delivery_method = original_config.delivery_method end @@ -28,8 +28,6 @@ RSpec.describe Alert do end it "adds context" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( :order, { number: "ABC123" } ) @@ -44,8 +42,6 @@ RSpec.describe Alert do end it "is compatible with Bugsnag API" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( :order, { number: "ABC123" } ) @@ -56,8 +52,6 @@ RSpec.describe Alert do end it "sends ActiveRecord objects" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - order = Spree::Order.new(number: "ABC123") expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( @@ -68,8 +62,6 @@ RSpec.describe Alert do end it "notifies Bugsnag when ActiveRecord object is missing" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( "NilClass", { record_was_nil: true } ) From c90472ebf90afbcbee7f3fe217b118ecb2f1e343 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 14:57:46 +1100 Subject: [PATCH 5/9] Update Bugsnag config naming --- config/initializers/bugsnag.rb | 2 +- spec/services/alert_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/initializers/bugsnag.rb b/config/initializers/bugsnag.rb index c67725fcf6..89dae2853d 100644 --- a/config/initializers/bugsnag.rb +++ b/config/initializers/bugsnag.rb @@ -12,5 +12,5 @@ Bugsnag.configure do |config| # If you want to notify Bugsnag in dev or test then set the env var: # spring stop # BUGSNAG=true ./bin/rails console - config.notify_release_stages = %w(production staging) unless ENV["BUGSNAG"] + config.enabled_release_stages = %w(production staging) unless ENV["BUGSNAG"] end diff --git a/spec/services/alert_spec.rb b/spec/services/alert_spec.rb index bc31d7686c..16a73721e5 100644 --- a/spec/services/alert_spec.rb +++ b/spec/services/alert_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Alert do Bugsnag.configure do |config| original_config = config.dup config.api_key ||= "00000000000000000000000000000000" - config.notify_release_stages = ["test"] + config.enabled_release_stages = ["test"] config.delivery_method = :synchronous end @@ -16,7 +16,7 @@ RSpec.describe Alert do Bugsnag.configure do |config| config.api_key = original_config.api_key - config.notify_release_stages = original_config.notify_release_stages + config.enabled_release_stages = original_config.notify_release_stages config.delivery_method = original_config.delivery_method end end From b23eac1004ae0812f68ed8feb63a2788fa973406 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 14:59:01 +1100 Subject: [PATCH 6/9] Omit setting Bugsnag release stage to default value --- config/initializers/bugsnag.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/config/initializers/bugsnag.rb b/config/initializers/bugsnag.rb index 89dae2853d..2eb0ddbe2a 100644 --- a/config/initializers/bugsnag.rb +++ b/config/initializers/bugsnag.rb @@ -1,6 +1,5 @@ Bugsnag.configure do |config| config.api_key = ENV['BUGSNAG_API_KEY'] - config.release_stage = Rails.env config.app_version = Rails.application.config.x.git_version # Avoid missing API key warning without changing the Rails log level. From fa7edbb073924f8111e520ab7981c7c6c991a3e9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 15:27:58 +1100 Subject: [PATCH 7/9] Alert ensures to pass metadata correctly --- app/services/alert.rb | 2 ++ spec/services/alert_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/app/services/alert.rb b/app/services/alert.rb index d030554298..7327cf45b0 100644 --- a/app/services/alert.rb +++ b/app/services/alert.rb @@ -23,6 +23,8 @@ class Alert def self.raise(error, metadata = {}, &block) Bugsnag.notify(error) do |payload| metadata.each do |name, data| + # Bugsnag only reports metadata when given a Hash. + data = { data: } unless data.is_a?(Hash) payload.add_metadata(name, data) end block.call(payload) diff --git a/spec/services/alert_spec.rb b/spec/services/alert_spec.rb index 16a73721e5..946db70684 100644 --- a/spec/services/alert_spec.rb +++ b/spec/services/alert_spec.rb @@ -41,6 +41,14 @@ RSpec.describe Alert do ) end + it "adds context given as keyword argument" do + expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( + :data, { data: "ABC123" } + ) + + Alert.raise("hey", data: "ABC123") + end + it "is compatible with Bugsnag API" do expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( :order, { number: "ABC123" } From 079d09b8b801d4487703a1474787b35dd2329b19 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 11 Feb 2025 15:36:07 +1100 Subject: [PATCH 8/9] Report simple values easily --- app/services/alert.rb | 5 +++++ spec/services/alert_spec.rb | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/app/services/alert.rb b/app/services/alert.rb index 7327cf45b0..a8ce1fbec0 100644 --- a/app/services/alert.rb +++ b/app/services/alert.rb @@ -22,11 +22,16 @@ class Alert # ) def self.raise(error, metadata = {}, &block) Bugsnag.notify(error) do |payload| + unless metadata.respond_to?(:each) + metadata = { metadata: { data: metadata } } + end + metadata.each do |name, data| # Bugsnag only reports metadata when given a Hash. data = { data: } unless data.is_a?(Hash) payload.add_metadata(name, data) end + block.call(payload) end end diff --git a/spec/services/alert_spec.rb b/spec/services/alert_spec.rb index 946db70684..143ecdb1d6 100644 --- a/spec/services/alert_spec.rb +++ b/spec/services/alert_spec.rb @@ -49,6 +49,14 @@ RSpec.describe Alert do Alert.raise("hey", data: "ABC123") end + it "adds simple values as context" do + expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( + :metadata, { data: "ABC123" } + ) + + Alert.raise("hey", "ABC123") + end + it "is compatible with Bugsnag API" do expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( :order, { number: "ABC123" } From d9cb1e8e74786a28db2459275c572479caa7a99e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 12 Feb 2025 15:33:33 +1100 Subject: [PATCH 9/9] Improve var naming, thanks Gaetan --- spec/services/alert_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/alert_spec.rb b/spec/services/alert_spec.rb index 143ecdb1d6..c899228e6a 100644 --- a/spec/services/alert_spec.rb +++ b/spec/services/alert_spec.rb @@ -43,10 +43,10 @@ RSpec.describe Alert do it "adds context given as keyword argument" do expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( - :data, { data: "ABC123" } + :thing, { data: "ABC123" } ) - Alert.raise("hey", data: "ABC123") + Alert.raise("hey", thing: "ABC123") end it "adds simple values as context" do