From 5648b2e281f5bfb753855072ce7c83f0bce95674 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 8 Jul 2020 18:32:40 +0100 Subject: [PATCH 1/5] Add rescue statements to subs jobs so that when an order placement or confirmation fails, there's a bugsnag alert for it and the job continues processing the rest of the orders --- app/jobs/subscription_confirm_job.rb | 2 ++ app/jobs/subscription_placement_job.rb | 13 ++++++++++--- .../order_management/subscriptions/summarizer.rb | 7 +++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index 9ed8223d0e..8735bf41ca 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -50,6 +50,8 @@ class SubscriptionConfirmJob else send_failed_payment_email(order) end + rescue StandardError => e + Bugsnag.notify(e, order: order) end def process_payment!(order) diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index 8b5df2b159..9d68dae2ea 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -29,10 +29,16 @@ class SubscriptionPlacementJob def place_order_for(proxy_order) JobLogger.logger.info("Placing Order for Proxy Order #{proxy_order.id}") - proxy_order.initialise_order! + initialise_order(proxy_order) place_order(proxy_order.order) end + def initialise_order(proxy_order) + proxy_order.initialise_order! + rescue StandardError => e + Bugsnag.notify(e, subscription: proxy_order.subscription, proxy_order: proxy_order) + end + def place_order(order) record_order(order) return record_issue(:complete, order) if order.completed? @@ -42,8 +48,9 @@ class SubscriptionPlacementJob move_to_completion(order) send_placement_email(order, changes) - rescue StateMachine::InvalidTransition - record_and_log_error(:processing, order) + rescue StandardError => e + record_and_log_error(:processing, order, e.message) + Bugsnag.notify(e, order: order) end def cap_quantity_and_store_changes(order) diff --git a/engines/order_management/app/services/order_management/subscriptions/summarizer.rb b/engines/order_management/app/services/order_management/subscriptions/summarizer.rb index 8029716338..f591295d1c 100644 --- a/engines/order_management/app/services/order_management/subscriptions/summarizer.rb +++ b/engines/order_management/app/services/order_management/subscriptions/summarizer.rb @@ -22,12 +22,15 @@ module OrderManagement summary_for(order).record_issue(type, order, message) end - def record_and_log_error(type, order) + def record_and_log_error(type, order, error_message = nil) return record_issue(type, order) unless order.errors.any? error = "Subscription#{type.to_s.camelize}Error" line1 = "#{error}: Cannot process order #{order.number} due to errors" - line2 = "Errors: #{order.errors.full_messages.join(', ')}" + + error_message ||= order.errors.full_messages.join(', ') + line2 = "Errors: #{error_message}" + JobLogger.logger.info("#{line1}\n#{line2}") record_issue(type, order, line2) end From 7a9f9a562489988d2a1ab31da0a48076d3abe501 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Thu, 9 Jul 2020 20:08:38 +0100 Subject: [PATCH 2/5] Log bugsnag and still send failed payment email when any exception is caught during the confirmation process --- app/jobs/subscription_confirm_job.rb | 5 +++-- spec/jobs/subscription_confirm_job_spec.rb | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index 8735bf41ca..ef2fd30ca8 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -52,6 +52,7 @@ class SubscriptionConfirmJob end rescue StandardError => e Bugsnag.notify(e, order: order) + send_failed_payment_email(order, e.message) end def process_payment!(order) @@ -89,9 +90,9 @@ class SubscriptionConfirmJob SubscriptionMailer.confirmation_email(order).deliver end - def send_failed_payment_email(order) + def send_failed_payment_email(order, error_message = nil) order.update! - record_and_log_error(:failed_payment, order) + record_and_log_error(:failed_payment, order, error_message) SubscriptionMailer.failed_payment_email(order).deliver end end diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb index 603da10a10..b6384ac099 100644 --- a/spec/jobs/subscription_confirm_job_spec.rb +++ b/spec/jobs/subscription_confirm_job_spec.rb @@ -216,7 +216,7 @@ describe SubscriptionConfirmJob do it "records and logs an error and sends the email" do expect(order).to receive(:update!) - expect(job).to receive(:record_and_log_error).with(:failed_payment, order).once + expect(job).to receive(:record_and_log_error).with(:failed_payment, order, nil).once job.send(:send_failed_payment_email, order) expect(SubscriptionMailer).to have_received(:failed_payment_email).with(order) expect(mail_mock).to have_received(:deliver) From 01ab974a3ba9f055c4c2506e0ae0a1c955c547b8 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 10 Jul 2020 10:16:20 +0100 Subject: [PATCH 3/5] Add rescue statment to failed payment email so that the processing of other orders continues if there is a problem while sending the email --- app/jobs/subscription_confirm_job.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index ef2fd30ca8..be43bc32ec 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -94,5 +94,7 @@ class SubscriptionConfirmJob order.update! record_and_log_error(:failed_payment, order, error_message) SubscriptionMailer.failed_payment_email(order).deliver + rescue StandardError => e + Bugsnag.notify(e, order: order, error_message: error_message) end end From 6389fdb16e00b0a0bf2c3059a39a9a16a55749a9 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 10 Jul 2020 12:40:36 +0200 Subject: [PATCH 4/5] Simplify code related to error handling --- app/jobs/subscription_confirm_job.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index be43bc32ec..c7275f00d6 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -45,30 +45,30 @@ class SubscriptionConfirmJob def confirm_order!(order) record_order(order) - if process_payment!(order) - send_confirmation_email(order) - else - send_failed_payment_email(order) - end + process_payment!(order) + send_confirmation_email(order) rescue StandardError => e - Bugsnag.notify(e, order: order) - send_failed_payment_email(order, e.message) + if order.errors.any? + send_failed_payment_email(order) + else + Bugsnag.notify(e, order: order) + send_failed_payment_email(order, e.message) + end end + # Process the order payment and raise if it's not successful def process_payment!(order) - return false if order.errors.present? - return true unless order.payment_required? + raise if order.errors.present? + return unless order.payment_required? setup_payment!(order) - return false if order.errors.any? + raise if order.errors.any? authorize_payment!(order) - return false if order.errors.any? + raise if order.errors.any? order.process_payments! - return false if order.errors.any? - - true + raise if order.errors.any? end def setup_payment!(order) From 5afb862ce174fa2eb6819650b3436116fcec5d57 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 10 Jul 2020 12:44:55 +0100 Subject: [PATCH 5/5] Extract setup and authorize to a new method called prepare_for_payment to fix rubocop ABCsize issue It looks like this rubocop rule weights a raise over a return... --- app/jobs/subscription_confirm_job.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index c7275f00d6..acf241e4e0 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -61,14 +61,17 @@ class SubscriptionConfirmJob raise if order.errors.present? return unless order.payment_required? + prepare_for_payment!(order) + order.process_payments! + raise if order.errors.any? + end + + def prepare_for_payment!(order) setup_payment!(order) raise if order.errors.any? authorize_payment!(order) raise if order.errors.any? - - order.process_payments! - raise if order.errors.any? end def setup_payment!(order)