From 74a8b3038afc4e777083e4601ab65fb273edab7f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 27 May 2021 12:55:52 +0200 Subject: [PATCH] Split ConfirmOrderJob to avoid blocking a worker This unties this two email notifications so that they are picked up by a DJ worker independently. This should avoid the blocking the worker experiences (remember we still have a single one in all instances) when waiting between the two deliveries. See the flamegraph: https://app.datadoghq.com/apm/traces?end=1622015605459&paused=true&query=env%3Aproduction%20service%3Adelayed_job%20operation_name%3Adelayed_job%20resource_name%3AConfirmOrderJob%20%40duration%3A%3E%3D5s&start=1622009898303&streamTraces=true&trace=AQAAAXmngbg_woqc_QAAAABBWG1uZ2IwVkFBRHVDbWJkN25QTUVuY28&traceID=2916038355421570548&spanID=2005781139590273685. Overall, both operations may take longer but other jobs can be processed in between. Also, if any of the two fails, the other won't be affected. --- app/jobs/confirm_order_job.rb | 8 ------ app/models/spree/order.rb | 3 ++- .../consumer/shopping/checkout_spec.rb | 10 ++++--- spec/jobs/confirm_order_job_spec.rb | 18 ------------- spec/jobs/subscription_confirm_job_spec.rb | 4 ++- spec/jobs/subscription_placement_job_spec.rb | 4 ++- spec/models/spree/order_spec.rb | 26 ++++++++++++------- 7 files changed, 31 insertions(+), 42 deletions(-) delete mode 100644 app/jobs/confirm_order_job.rb delete mode 100644 spec/jobs/confirm_order_job_spec.rb diff --git a/app/jobs/confirm_order_job.rb b/app/jobs/confirm_order_job.rb deleted file mode 100644 index fe4596d589..0000000000 --- a/app/jobs/confirm_order_job.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -class ConfirmOrderJob < ActiveJob::Base - def perform(order_id) - Spree::OrderMailer.confirm_email_for_customer(order_id).deliver_now - Spree::OrderMailer.confirm_email_for_shop(order_id).deliver_now - end -end diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 0a3042c418..1c6516a6f2 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -395,7 +395,8 @@ module Spree def deliver_order_confirmation_email return if subscription.present? - ConfirmOrderJob.perform_later(id) + Spree::OrderMailer.confirm_email_for_customer(id).deliver_later + Spree::OrderMailer.confirm_email_for_shop(id).deliver_later end # Helper methods for checkout steps diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index d0f4c03241..241f63d4c1 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -251,10 +251,14 @@ feature "As a consumer I want to check out my cart", js: true do choose shipping_with_fee.name choose check_without_fee.name - expect do + perform_enqueued_jobs do place_order + expect(page).to have_content "Your order has been processed successfully" - end.to enqueue_job ConfirmOrderJob + + expect(ActionMailer::Base.deliveries.first.subject).to match(/Order Confirmation/) + expect(ActionMailer::Base.deliveries.second.subject).to match(/Order Confirmation/) + end order = Spree::Order.complete.last expect(order.payment_state).to eq "balance_due" @@ -390,7 +394,7 @@ feature "As a consumer I want to check out my cart", js: true do expect do place_order expect(page).to have_content "Your order has been processed successfully" - end.to enqueue_job ConfirmOrderJob + end.to have_enqueued_mail(Spree::OrderMailer, :confirm_email_for_customer) # And the order's special instructions should be set order = Spree::Order.complete.last diff --git a/spec/jobs/confirm_order_job_spec.rb b/spec/jobs/confirm_order_job_spec.rb deleted file mode 100644 index 19e9b8ac96..0000000000 --- a/spec/jobs/confirm_order_job_spec.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe ConfirmOrderJob do - let(:order) { create(:order) } - - it "sends confirmation emails to both the user and the shop owner" do - customer_confirm_fake = double(:confirm_email_for_customer) - shop_confirm_fake = double(:confirm_email_for_shop) - expect(Spree::OrderMailer).to receive(:confirm_email_for_customer).and_return customer_confirm_fake - expect(Spree::OrderMailer).to receive(:confirm_email_for_shop).and_return shop_confirm_fake - expect(customer_confirm_fake).to receive :deliver_now - expect(shop_confirm_fake).to receive :deliver_now - - ConfirmOrderJob.perform_now order.id - end -end diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb index a8373553b8..d25a7560cc 100644 --- a/spec/jobs/subscription_confirm_job_spec.rb +++ b/spec/jobs/subscription_confirm_job_spec.rb @@ -237,7 +237,9 @@ describe SubscriptionConfirmJob do end it "sends only a subscription confirm email, no regular confirmation emails" do - expect{ job.send(:confirm_order!, order) }.to_not enqueue_job ConfirmOrderJob + expect{ job.send(:confirm_order!, order) } + .to_not have_enqueued_mail(Spree::OrderMailer, :confirm_email_for_customer) + expect(job).to have_received(:send_confirmation_email).once end end diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb index 0f43d81781..2e8273894f 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -202,7 +202,9 @@ describe SubscriptionPlacementJob do end it "does not enqueue confirmation emails" do - expect{ job.send(:place_order, order) }.to_not enqueue_job ConfirmOrderJob + expect{ job.send(:place_order, order) } + .to_not have_enqueued_mail(Spree::OrderMailer, :confirm_email_for_customer) + expect(job).to have_received(:send_placement_email).with(order, anything).once end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index e97ac29062..92b1e4e8d0 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -169,10 +169,13 @@ describe Spree::Order do expect(order.shipment_state).to eq 'ready' end - it "should send an order confirmation email" do - expect do - order.finalize! - end.to enqueue_job ConfirmOrderJob + it "sends confirmation emails to both the user and the shop owner" do + mailer = double(:mailer, deliver_later: true) + + expect(Spree::OrderMailer).to receive(:confirm_email_for_customer).and_return(mailer) + expect(Spree::OrderMailer).to receive(:confirm_email_for_shop).and_return(mailer) + + order.finalize! end it "should freeze all adjustments" do @@ -901,17 +904,20 @@ describe Spree::Order do let!(:order) { create(:order, distributor: distributor) } it "sends confirmation emails" do - expect do - order.deliver_order_confirmation_email - end.to enqueue_job ConfirmOrderJob + mailer = double(:mailer, deliver_later: true) + expect(Spree::OrderMailer).to receive(:confirm_email_for_customer).and_return(mailer) + expect(Spree::OrderMailer).to receive(:confirm_email_for_shop).and_return(mailer) + + order.deliver_order_confirmation_email end it "does not send confirmation emails when the order belongs to a subscription" do create(:proxy_order, order: order) - expect do - order.deliver_order_confirmation_email - end.to_not enqueue_job ConfirmOrderJob + expect(Spree::OrderMailer).not_to receive(:confirm_email_for_customer) + expect(Spree::OrderMailer).not_to receive(:confirm_email_for_shop) + + order.deliver_order_confirmation_email end end