From bbdbf387b7bd2904fb2440acd207b67a47ed8ffb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 17:13:16 +0000 Subject: [PATCH 1/6] Combine SendAuthorizationEmails and StripeScaPaymentAuthorize --- app/jobs/subscription_confirm_job.rb | 4 +--- .../order/send_authorization_emails.rb | 16 ------------- .../order/stripe_sca_payment_authorize.rb | 12 ++++++++-- .../stripe_sca_payment_authorize_spec.rb | 24 ++++++++++++------- 4 files changed, 26 insertions(+), 30 deletions(-) delete mode 100644 engines/order_management/app/services/order_management/order/send_authorization_emails.rb diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index e492036220..d0db6636e6 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -88,9 +88,7 @@ class SubscriptionConfirmJob < ActiveJob::Base def authorize_payment!(order) return if order.subscription.payment_method.class != Spree::Gateway::StripeSCA - OrderManagement::Order::StripeScaPaymentAuthorize.new(order). - extend(OrderManagement::Order::SendAuthorizationEmails). - call! + OrderManagement::Order::StripeScaPaymentAuthorize.new(order, off_session: true).call! end def send_confirmation_email(order) diff --git a/engines/order_management/app/services/order_management/order/send_authorization_emails.rb b/engines/order_management/app/services/order_management/order/send_authorization_emails.rb deleted file mode 100644 index 5b4fc46924..0000000000 --- a/engines/order_management/app/services/order_management/order/send_authorization_emails.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -module OrderManagement - module Order - module SendAuthorizationEmails - def call!(redirect_url = full_order_path(@order)) - super(redirect_url) - - return unless @payment.requires_authorization? - - PaymentMailer.authorize_payment(@payment).deliver_now - PaymentMailer.authorization_required(@payment).deliver_now - end - end - end -end diff --git a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb index 80f5364490..0f802f92b6 100644 --- a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb +++ b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb @@ -5,9 +5,10 @@ module OrderManagement class StripeScaPaymentAuthorize include FullUrlHelper - def initialize(order) + def initialize(order, off_session: false) @order = order @payment = OrderPaymentFinder.new(@order).last_pending_payment + @off_session = off_session end def call!(redirect_url = full_order_path(@order)) @@ -19,8 +20,15 @@ module OrderManagement @order.errors.add(:base, I18n.t('authorization_failure')) end - @payment + return @payment unless @payment.requires_authorization? && off_session + + PaymentMailer.authorize_payment(@payment).deliver_now + PaymentMailer.authorization_required(@payment).deliver_now end + + private + + attr_reader :off_session end end end diff --git a/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb b/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb index 00447c6d78..9cfb512cfa 100644 --- a/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb +++ b/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb @@ -70,15 +70,6 @@ module OrderManagement } end - it "sends an email requesting authorization and an email notifying the shop owner when requested" do - payment_authorize.extend(OrderManagement::Order::SendAuthorizationEmails).call! - - expect(order.errors.size).to eq 0 - expect(PaymentMailer).to have_received(:authorize_payment) - expect(PaymentMailer).to have_received(:authorization_required) - expect(mail_mock).to have_received(:deliver_now).twice - end - it "doesn't send emails by default" do payment_authorize.call! @@ -87,6 +78,21 @@ module OrderManagement expect(PaymentMailer).to_not have_received(:authorization_required) expect(mail_mock).to_not have_received(:deliver_now) end + + context "when the processing is off-session (via backoffice/subscription)" do + let(:payment_authorize) { + OrderManagement::Order::StripeScaPaymentAuthorize.new(order, off_session: true) + } + + it "sends an email requesting authorization and an email notifying the shop owner when requested" do + payment_authorize.call! + + expect(order.errors.size).to eq 0 + expect(PaymentMailer).to have_received(:authorize_payment) + expect(PaymentMailer).to have_received(:authorization_required) + expect(mail_mock).to have_received(:deliver_now).twice + end + end end end end From 6b683d600f707828bf4f16875a430031585eb95f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 17:20:00 +0000 Subject: [PATCH 2/6] Break up #call! with some readable comment-methods --- .../order/stripe_sca_payment_authorize.rb | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb index 0f802f92b6..99e0854243 100644 --- a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb +++ b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb @@ -7,28 +7,37 @@ module OrderManagement def initialize(order, off_session: false) @order = order - @payment = OrderPaymentFinder.new(@order).last_pending_payment + @payment = OrderPaymentFinder.new(order).last_pending_payment @off_session = off_session end - def call!(redirect_url = full_order_path(@order)) - return unless @payment&.checkout? + def call!(redirect_url = full_order_path(order)) + return unless payment&.checkout? - @payment.authorize!(redirect_url) + payment.authorize!(redirect_url) - unless @payment.pending? || @payment.requires_authorization? - @order.errors.add(:base, I18n.t('authorization_failure')) - end + order.errors.add(:base, I18n.t('authorization_failure')) unless successfully_processed? + send_auth_emails if requires_authorization_emails? - return @payment unless @payment.requires_authorization? && off_session - - PaymentMailer.authorize_payment(@payment).deliver_now - PaymentMailer.authorization_required(@payment).deliver_now + payment end private - attr_reader :off_session + attr_reader :order, :payment, :off_session + + def successfully_processed? + payment.pending? || payment.requires_authorization? + end + + def requires_authorization_emails? + payment.requires_authorization? && off_session + end + + def send_auth_emails + PaymentMailer.authorize_payment(payment).deliver_now + PaymentMailer.authorization_required(payment).deliver_now + end end end end From dbe4d61e57e846d031bbea8a06ab704385ada94d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 17:57:53 +0000 Subject: [PATCH 3/6] Add explanatory note for "off-session" payment processing --- .../order_management/order/stripe_sca_payment_authorize.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb index 99e0854243..8e135c2fc3 100644 --- a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb +++ b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb @@ -1,5 +1,10 @@ # frozen_string_literal: true +# Note: "off-session" processing happens when a payment is placed on behalf of a user when +# they are currently offline. This can happen with backoffice orders or subscriptions. +# In that case; if the payment requires authorization in Stripe, we send an email so the user +# can authorize it later (asynchronously). + module OrderManagement module Order class StripeScaPaymentAuthorize From 542e3ad1aabaac92772f77f7072e282483b53e91 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 16 Dec 2021 20:28:43 +0000 Subject: [PATCH 4/6] Reuse StripeScaPaymentAuthorize in Admin::PaymentsController --- app/controllers/spree/admin/payments_controller.rb | 9 ++++----- .../order/stripe_sca_payment_authorize.rb | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index 543ec7d707..566701ea66 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -174,15 +174,14 @@ module Spree def authorize_stripe_sca_payment return unless @payment.payment_method.instance_of?(Spree::Gateway::StripeSCA) - @payment.authorize!(full_order_path(@payment.order)) + OrderManagement::Order::StripeScaPaymentAuthorize. + new(@order, payment: @payment, off_session: true). + call!(full_order_path(@order)) - unless @payment.pending? || @payment.requires_authorization? - raise Spree::Core::GatewayError, I18n.t('authorization_failure') - end + raise Spree::Core::GatewayError, I18n.t('authorization_failure') if @order.errors.any? return unless @payment.requires_authorization? - PaymentMailer.authorize_payment(@payment).deliver_later raise Spree::Core::GatewayError, I18n.t('action_required') end diff --git a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb index 8e135c2fc3..f16c8ce57c 100644 --- a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb +++ b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb @@ -10,9 +10,9 @@ module OrderManagement class StripeScaPaymentAuthorize include FullUrlHelper - def initialize(order, off_session: false) + def initialize(order, payment: nil, off_session: false) @order = order - @payment = OrderPaymentFinder.new(order).last_pending_payment + @payment = payment || OrderPaymentFinder.new(order).last_pending_payment @off_session = off_session end From 438c205d879d7b36fb12b26312c81044aee4c549 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 17 Dec 2021 12:38:09 +0000 Subject: [PATCH 5/6] Add basic test to SubscriptionConfirmJob spec --- spec/jobs/subscription_confirm_job_spec.rb | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb index 8c2ac1a2cb..897625b7f6 100644 --- a/spec/jobs/subscription_confirm_job_spec.rb +++ b/spec/jobs/subscription_confirm_job_spec.rb @@ -170,10 +170,10 @@ describe SubscriptionConfirmJob do let(:provider) { double } before do - allow_any_instance_of(Stripe::CreditCardCloner).to receive(:find_or_clone) { - ["cus_123", "pm_1234"] - } + allow_any_instance_of(Stripe::CreditCardCloner). + to receive(:find_or_clone) { ["cus_123", "pm_1234"] } allow(order).to receive(:pending_payments) { [stripe_sca_payment] } + allow(order).to receive_message_chain(:subscription, :payment_method) { stripe_sca_payment_method } allow(stripe_sca_payment_method).to receive(:provider) { provider } allow(stripe_sca_payment_method.provider).to receive(:purchase) { true } allow(stripe_sca_payment_method.provider).to receive(:capture) { true } @@ -189,6 +189,13 @@ describe SubscriptionConfirmJob do expect(stripe_sca_payment_method.provider).to receive(:capture) job.send(:confirm_order!, order) end + + it "authorizes the payment with Stripe" do + expect(OrderManagement::Order::StripeScaPaymentAuthorize). + to receive_message_chain(:new, :call!) { true } + + job.send(:confirm_order!, order) + end end context "Stripe Connect" do From 68af9b6e80fd53f940a64c0360995f22667035af Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 17 Dec 2021 12:46:11 +0000 Subject: [PATCH 6/6] Only email the hub about authorization required when necessary This is needed in subscriptions, but in the backoffice the hub manager gets a notification in the UI, so the email is not needed. --- app/jobs/subscription_confirm_job.rb | 6 ++++- .../order/stripe_sca_payment_authorize.rb | 7 +++--- .../stripe_sca_payment_authorize_spec.rb | 24 ++++++++++++++++--- spec/jobs/subscription_confirm_job_spec.rb | 3 ++- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/app/jobs/subscription_confirm_job.rb b/app/jobs/subscription_confirm_job.rb index d0db6636e6..0ad025d9f6 100644 --- a/app/jobs/subscription_confirm_job.rb +++ b/app/jobs/subscription_confirm_job.rb @@ -88,7 +88,11 @@ class SubscriptionConfirmJob < ActiveJob::Base def authorize_payment!(order) return if order.subscription.payment_method.class != Spree::Gateway::StripeSCA - OrderManagement::Order::StripeScaPaymentAuthorize.new(order, off_session: true).call! + OrderManagement::Order::StripeScaPaymentAuthorize.new( + order, + off_session: true, + notify_hub: true + ).call! end def send_confirmation_email(order) diff --git a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb index f16c8ce57c..4a7839f614 100644 --- a/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb +++ b/engines/order_management/app/services/order_management/order/stripe_sca_payment_authorize.rb @@ -10,10 +10,11 @@ module OrderManagement class StripeScaPaymentAuthorize include FullUrlHelper - def initialize(order, payment: nil, off_session: false) + def initialize(order, payment: nil, off_session: false, notify_hub: false) @order = order @payment = payment || OrderPaymentFinder.new(order).last_pending_payment @off_session = off_session + @notify_hub = notify_hub end def call!(redirect_url = full_order_path(order)) @@ -29,7 +30,7 @@ module OrderManagement private - attr_reader :order, :payment, :off_session + attr_reader :order, :payment, :off_session, :notify_hub def successfully_processed? payment.pending? || payment.requires_authorization? @@ -41,7 +42,7 @@ module OrderManagement def send_auth_emails PaymentMailer.authorize_payment(payment).deliver_now - PaymentMailer.authorization_required(payment).deliver_now + PaymentMailer.authorization_required(payment).deliver_now if notify_hub end end end diff --git a/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb b/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb index 9cfb512cfa..9dd15e8a8b 100644 --- a/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb +++ b/engines/order_management/spec/services/order_management/order/stripe_sca_payment_authorize_spec.rb @@ -84,13 +84,31 @@ module OrderManagement OrderManagement::Order::StripeScaPaymentAuthorize.new(order, off_session: true) } - it "sends an email requesting authorization and an email notifying the shop owner when requested" do + it "notifies the customer" do payment_authorize.call! expect(order.errors.size).to eq 0 expect(PaymentMailer).to have_received(:authorize_payment) - expect(PaymentMailer).to have_received(:authorization_required) - expect(mail_mock).to have_received(:deliver_now).twice + expect(mail_mock).to have_received(:deliver_now).once + end + + context "with an additional notification for the hub manager" do + let(:payment_authorize) { + OrderManagement::Order::StripeScaPaymentAuthorize.new( + order, + off_session: true, + notify_hub: true + ) + } + + it "notifies both the customer and the hub" do + payment_authorize.call! + + expect(order.errors.size).to eq 0 + expect(PaymentMailer).to have_received(:authorize_payment) + expect(PaymentMailer).to have_received(:authorization_required) + expect(mail_mock).to have_received(:deliver_now).twice + end end end end diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb index 897625b7f6..3d61d569ec 100644 --- a/spec/jobs/subscription_confirm_job_spec.rb +++ b/spec/jobs/subscription_confirm_job_spec.rb @@ -173,7 +173,6 @@ describe SubscriptionConfirmJob do allow_any_instance_of(Stripe::CreditCardCloner). to receive(:find_or_clone) { ["cus_123", "pm_1234"] } allow(order).to receive(:pending_payments) { [stripe_sca_payment] } - allow(order).to receive_message_chain(:subscription, :payment_method) { stripe_sca_payment_method } allow(stripe_sca_payment_method).to receive(:provider) { provider } allow(stripe_sca_payment_method.provider).to receive(:purchase) { true } allow(stripe_sca_payment_method.provider).to receive(:capture) { true } @@ -191,6 +190,8 @@ describe SubscriptionConfirmJob do end it "authorizes the payment with Stripe" do + allow(order).to receive_message_chain(:subscription, :payment_method) { stripe_sca_payment_method } + expect(OrderManagement::Order::StripeScaPaymentAuthorize). to receive_message_chain(:new, :call!) { true }