From fcb013eb2a8d443342ad2d8ed9d45ec6a2e0cb84 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Tue, 30 Mar 2021 12:05:52 +0200
Subject: [PATCH] Extract subs placement job logic into new service

While doing that we pass stock changes to the service but we
lazy-evaluate them. This way we don't fetch all this data from DB when
it might not be used due to an early return.

Also, this makes it possible to save the stock-related logic for later.

Finally, when changing things to rely on `#initialize_order`'s boolean
return value I noticed though, that we were evaluating
`proxy_order.order` too early.  When instantiating the service object it
won't exist yet.
---
 app/jobs/subscription_placement_job.rb       |  59 +------
 app/services/place_order.rb                  |  82 ++++++++++
 spec/jobs/subscription_placement_job_spec.rb | 152 ++++++++-----------
 spec/services/place_order_spec.rb            | 132 ++++++++++++++++
 4 files changed, 287 insertions(+), 138 deletions(-)
 create mode 100644 app/services/place_order.rb
 create mode 100644 spec/services/place_order_spec.rb

diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb
index 45a8de23cb..674b6ae2f4 100644
--- a/app/jobs/subscription_placement_job.rb
+++ b/app/jobs/subscription_placement_job.rb
@@ -9,14 +9,11 @@ class SubscriptionPlacementJob < ActiveJob::Base
       place_order_for(proxy_order)
     end
 
-    send_placement_summary_emails
+    summarizer.send_placement_summary_emails
   end
 
   private
 
-  delegate :record_success, :record_issue, :record_subscription_issue, to: :summarizer
-  delegate :record_order, :record_and_log_error, :send_placement_summary_emails, to: :summarizer
-
   def summarizer
     @summarizer ||= OrderManagement::Subscriptions::Summarizer.new
   end
@@ -29,33 +26,14 @@ class SubscriptionPlacementJob < ActiveJob::Base
   end
 
   def place_order_for(proxy_order)
-    JobLogger.logger.info("Placing Order for Proxy Order #{proxy_order.id}")
-    initialise_order(proxy_order)
-    return unless proxy_order.order.present?
+    place_order = PlaceOrder.new(
+      proxy_order,
+      summarizer,
+      JobLogger.logger,
+      lambda { cap_quantity_and_store_changes(proxy_order.order) }
+    )
 
-    proxy_order.update_column(:placed_at, Time.zone.now)
-    place_order(proxy_order.order)
-  end
-
-  def initialise_order(proxy_order)
-    proxy_order.initialise_order!
-    record_subscription_issue(proxy_order.subscription) if proxy_order.order.nil?
-  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?
-
-    changes = cap_quantity_and_store_changes(order)
-    return handle_empty_order(order, changes) if order.line_items.where('quantity > 0').empty?
-
-    move_to_completion(order)
-    send_placement_email(order, changes)
-  rescue StandardError => e
-    record_and_log_error(:processing, order, e.message)
-    Bugsnag.notify(e, order: order)
+    place_order.call
   end
 
   def cap_quantity_and_store_changes(order)
@@ -77,16 +55,6 @@ class SubscriptionPlacementJob < ActiveJob::Base
     changes
   end
 
-  def handle_empty_order(order, changes)
-    order.reload.all_adjustments.destroy_all
-    order.update_order!
-    send_empty_email(order, changes)
-  end
-
-  def move_to_completion(order)
-    OrderWorkflow.new(order).complete!
-  end
-
   def unavailable_stock_lines_for(order)
     order.line_items.where('variant_id NOT IN (?)', available_variants_for(order).select(&:id))
   end
@@ -94,15 +62,4 @@ class SubscriptionPlacementJob < ActiveJob::Base
   def available_variants_for(order)
     OrderCycleDistributedVariants.new(order.order_cycle, order.distributor).available_variants
   end
-
-  def send_placement_email(order, changes)
-    record_issue(:changes, order) if changes.present?
-    record_success(order) if changes.blank?
-    SubscriptionMailer.placement_email(order, changes).deliver_now
-  end
-
-  def send_empty_email(order, changes)
-    record_issue(:empty, order)
-    SubscriptionMailer.empty_email(order, changes).deliver_now
-  end
 end
diff --git a/app/services/place_order.rb b/app/services/place_order.rb
new file mode 100644
index 0000000000..521defdcaa
--- /dev/null
+++ b/app/services/place_order.rb
@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+class PlaceOrder
+  def initialize(proxy_order, summarizer, logger, changes_loader)
+    @proxy_order = proxy_order
+    @subscription = proxy_order.subscription
+    @summarizer = summarizer
+    @logger = logger
+    @changes_loader = changes_loader
+  end
+
+  def call
+    return unless initialise_order
+
+    mark_as_processed
+
+    summarizer.record_order(order)
+    return summarizer.record_issue(:complete, order) if order.completed?
+
+    load_changes
+    return handle_empty_order if empty_order?
+
+    move_to_completion
+    send_placement_email
+  rescue StandardError => e
+    summarizer.record_and_log_error(:processing, e.message)
+    Bugsnag.notify(e, order: order)
+  end
+
+  private
+
+  attr_reader :proxy_order, :subscription, :order, :summarizer, :logger, :changes_loader, :changes
+
+  def initialise_order
+    logger.info("Placing Order for Proxy Order #{proxy_order.id}")
+
+    @order = proxy_order.initialise_order!
+
+    if order.nil?
+      summarizer.record_subscription_issue(subscription)
+      return false
+    end
+    true
+  rescue StandardError => e
+    Bugsnag.notify(e, subscription: subscription, proxy_order: proxy_order)
+    false
+  end
+
+  def mark_as_processed
+    proxy_order.update_column(:placed_at, Time.now)
+  end
+
+  def load_changes
+    @changes = changes_loader.call
+  end
+
+  def empty_order?
+    order.line_items.where('quantity > 0').empty?
+  end
+
+  def handle_empty_order
+    order.reload.all_adjustments.destroy_all
+    order.update_order!
+    send_empty_email
+  end
+
+  def send_empty_email
+    summarizer.record_issue(:empty, order)
+    SubscriptionMailer.empty_email(order, changes).deliver_now
+  end
+
+  def move_to_completion
+    OrderWorkflow.new(order).complete!
+  end
+
+  def send_placement_email
+    summarizer.record_issue(:changes, order) if changes.present?
+    summarizer.record_success(order) if changes.blank?
+
+    SubscriptionMailer.placement_email(order, changes).deliver_now
+  end
+end
diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb
index 06b092a400..408b6f2245 100644
--- a/spec/jobs/subscription_placement_job_spec.rb
+++ b/spec/jobs/subscription_placement_job_spec.rb
@@ -51,21 +51,25 @@ describe SubscriptionPlacementJob do
   describe "performing the job" do
     context "when unplaced proxy_orders exist" do
       let!(:subscription) { create(:subscription, with_items: true) }
-      let!(:proxy_order) { create(:proxy_order, subscription: subscription) }
+      let!(:proxy_order) { create(:proxy_order, subscription: subscription, order: build(:order)) }
 
       before do
         allow(job).to receive(:proxy_orders) { ProxyOrder.where(id: proxy_order.id) }
-        allow(job).to receive(:place_order)
-      end
-
-      it "marks placeable proxy_orders as processed by setting placed_at" do
-        expect{ job.perform }.to change{ proxy_order.reload.placed_at }
-        expect(proxy_order.placed_at).to be_within(5.seconds).of Time.zone.now
       end
 
       it "processes placeable proxy_orders" do
+        summarizer = instance_double(OrderManagement::Subscriptions::Summarizer)
+        service = PlaceOrder.new(
+          proxy_order,
+          summarizer,
+          JobLogger.logger,
+          lambda { job.send(:cap_quantity_and_store_changes, order) }
+        )
+
+        allow(PlaceOrder).to receive(:new) { service }
+        allow(service).to receive(:call)
         job.perform
-        expect(job).to have_received(:place_order).with(proxy_order.reload.order)
+        expect(service).to have_received(:call)
       end
     end
   end
@@ -125,18 +129,6 @@ describe SubscriptionPlacementJob do
           expect(changes[line_item2.id]).to be 3
           expect(changes[line_item3.id]).to be 3
         end
-
-        context "and the order has been placed" do
-          before do
-            allow(order).to receive(:ensure_available_shipping_rates) { true }
-            allow(order).to receive(:process_each_payment) { true }
-            job.send(:place_order, order.reload)
-          end
-
-          it "removes the unavailable items from the shipment" do
-            expect(order.shipment.manifest.size).to eq 1
-          end
-        end
       end
     end
   end
@@ -157,17 +149,25 @@ describe SubscriptionPlacementJob do
 
     before do
       expect_any_instance_of(Spree::Payment).to_not receive(:process!)
-      allow(job).to receive(:send_placement_email)
-      allow(job).to receive(:send_empty_email)
+      allow_any_instance_of(PlaceOrder).to receive(:send_placement_email)
+      allow_any_instance_of(PlaceOrder).to receive(:send_empty_email)
     end
 
     context "when the order is already complete" do
       before { break unless order.next! while !order.completed? }
 
       it "records an issue and ignores it" do
+        summarizer = instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true)
+        service = PlaceOrder.new(
+          proxy_order,
+          summarizer,
+          JobLogger.logger,
+          lambda { job.send(:cap_quantity_and_store_changes, order) }
+        )
+
         ActionMailer::Base.deliveries.clear
-        expect(job).to receive(:record_issue).with(:complete, order).once
-        expect{ job.send(:place_order, order) }.to_not change{ order.reload.state }
+        expect(summarizer).to receive(:record_issue).with(:complete, order).once
+        expect{ service.call }.to_not change{ order.reload.state }
         expect(order.payments.first.state).to eq "checkout"
         expect(ActionMailer::Base.deliveries.count).to be 0
       end
@@ -180,9 +180,10 @@ describe SubscriptionPlacementJob do
         end
 
         it "uses the same shipping method after advancing the order" do
-          job.send(:place_order, order)
-          expect(order.state).to eq "complete"
+          allow(proxy_order).to receive(:order) { order }
+          job.send(:place_order_for, proxy_order)
           order.reload
+          expect(order.state).to eq "complete"
           expect(order.shipping_method).to eq(shipping_method)
         end
       end
@@ -193,40 +194,65 @@ describe SubscriptionPlacementJob do
         end
 
         it "does not place the order, clears all adjustments, and sends an empty_order email" do
-          expect{ job.send(:place_order, order) }.to_not change{
-                                                           order.reload.completed_at
-                                                         }.from(nil)
+          summarizer = instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true, record_issue: true)
+          service = PlaceOrder.new(
+            proxy_order,
+            summarizer,
+            JobLogger.logger,
+            lambda { job.send(:cap_quantity_and_store_changes, order) }
+          )
+
+          allow(service).to receive(:send_placement_email)
+          allow(service).to receive(:send_empty_email)
+
+          expect{ service.call }.to_not change{ order.reload.completed_at }.from(nil)
           expect(order.all_adjustments).to be_empty
           expect(order.total).to eq 0
           expect(order.adjustment_total).to eq 0
-          expect(job).to_not have_received(:send_placement_email)
-          expect(job).to have_received(:send_empty_email)
+          expect(service).to_not have_received(:send_placement_email)
+          expect(service).to have_received(:send_empty_email)
         end
       end
 
       context "when at least one stock item is available after capping stock" do
+        let(:summarizer) do
+          instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true, record_success: true)
+        end
+        let(:service) do
+          PlaceOrder.new(
+            proxy_order,
+            summarizer,
+            JobLogger.logger,
+            lambda { job.send(:cap_quantity_and_store_changes, order) }
+          )
+        end
+
+        before do
+          allow(service).to receive(:send_placement_email)
+        end
+
         it "processes the order to completion, but does not process the payment" do
           # If this spec starts complaining about no shipping methods being available
           # on CI, there is probably another spec resetting the currency though Rails.cache.clear
-          expect{ job.send(:place_order, order) }.to change{ order.reload.completed_at }.from(nil)
+          expect{ service.call }.to change{ order.reload.completed_at }.from(nil)
           expect(order.completed_at).to be_within(5.seconds).of Time.zone.now
           expect(order.payments.first.state).to eq "checkout"
         end
 
         it "does not enqueue confirmation emails" do
-          expect{ job.send(:place_order, order) }
+          expect{ service.call }
             .to_not have_enqueued_mail(Spree::OrderMailer, :confirm_email_for_customer)
 
-          expect(job).to have_received(:send_placement_email).with(order, anything).once
+          expect(service).to have_received(:send_placement_email).once
         end
 
         context "when progression of the order fails" do
-          before { allow(order).to receive(:next) { false } }
+          before { allow(service).to receive(:move_to_completion).and_raise(StandardError) }
 
           it "records an error and does not attempt to send an email" do
-            expect(job).to_not receive(:send_placement_email)
-            expect(job).to receive(:record_and_log_error).once
-            job.send(:place_order, order)
+            expect(service).to_not receive(:send_placement_email)
+            expect(summarizer).to receive(:record_and_log_error).once
+            service.call
           end
         end
       end
@@ -238,58 +264,10 @@ describe SubscriptionPlacementJob do
       end
 
       it "records an error " do
-        expect(job).to receive(:record_subscription_issue)
+        expect_any_instance_of(OrderManagement::Subscriptions::Summarizer).to receive(:record_subscription_issue)
         expect(job).to_not receive(:place_order)
         job.send(:place_order_for, proxy_order)
       end
     end
   end
-
-  describe "#send_placement_email" do
-    let!(:order) { double(:order) }
-    let(:mail_mock) { double(:mailer_mock, deliver_now: true) }
-
-    before do
-      allow(SubscriptionMailer).to receive(:placement_email) { mail_mock }
-    end
-
-    context "when changes are present" do
-      let(:changes) { double(:changes) }
-
-      it "logs an issue and sends the email" do
-        expect(job).to receive(:record_issue).with(:changes, order).once
-        job.send(:send_placement_email, order, changes)
-        expect(SubscriptionMailer).to have_received(:placement_email).with(order, changes)
-        expect(mail_mock).to have_received(:deliver_now)
-      end
-    end
-
-    context "when no changes are present" do
-      let(:changes) { {} }
-
-      it "logs a success and sends the email" do
-        expect(job).to receive(:record_success).with(order).once
-        job.send(:send_placement_email, order, changes)
-        expect(SubscriptionMailer).to have_received(:placement_email)
-        expect(mail_mock).to have_received(:deliver_now)
-      end
-    end
-  end
-
-  describe "#send_empty_email" do
-    let!(:order) { double(:order) }
-    let(:changes) { double(:changes) }
-    let(:mail_mock) { double(:mailer_mock, deliver_now: true) }
-
-    before do
-      allow(SubscriptionMailer).to receive(:empty_email) { mail_mock }
-    end
-
-    it "logs an issue and sends the email" do
-      expect(job).to receive(:record_issue).with(:empty, order).once
-      job.send(:send_empty_email, order, changes)
-      expect(SubscriptionMailer).to have_received(:empty_email).with(order, changes)
-      expect(mail_mock).to have_received(:deliver_now)
-    end
-  end
 end
diff --git a/spec/services/place_order_spec.rb b/spec/services/place_order_spec.rb
new file mode 100644
index 0000000000..191e5313e0
--- /dev/null
+++ b/spec/services/place_order_spec.rb
@@ -0,0 +1,132 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PlaceOrder do
+  subject { described_class.new(proxy_order, summarizer, logger, changes) }
+
+  let(:changes) { {} }
+  let(:proxy_order) { create(:proxy_order, order: order) }
+  let(:order) { build(:order) }
+  let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer) }
+  let(:logger) { instance_double(JobLogger.logger.class, info: true) }
+
+  let(:mail_mock) { double(:mailer_mock, deliver_now: true) }
+
+  describe "#call" do
+    let!(:subscription) { create(:subscription, with_items: true) }
+    let!(:proxy_order) { create(:proxy_order, subscription: subscription, order: order) }
+
+    let(:changes) { lambda { {} } }
+    let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true, record_issue: true) }
+
+    before do
+      allow(SubscriptionMailer).to receive(:empty_email) { mail_mock }
+      subject.initialise_order
+    end
+
+    it "marks placeable proxy_orders as processed by setting placed_at" do
+      expect{ subject.call(order, subject) }.to change{ proxy_order.reload.placed_at }
+      expect(proxy_order.placed_at).to be_within(5.seconds).of Time.zone.now
+    end
+  end
+
+  describe "#send_placement_email" do
+    let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true) }
+
+    before do
+      allow(SubscriptionMailer).to receive(:placement_email) { mail_mock }
+    end
+
+    before do
+      order.line_items << build(:line_item)
+
+      order_workflow = instance_double(OrderWorkflow, complete!: true)
+      allow(OrderWorkflow).to receive(:new).with(order).and_return(order_workflow)
+    end
+
+    context "when no changes are present" do
+      let(:changes) { lambda { {} } }
+
+      it "logs a success and sends the email" do
+        expect(summarizer).to receive(:record_success).with(order).once
+
+        subject.call
+
+        expect(SubscriptionMailer).to have_received(:placement_email)
+        expect(mail_mock).to have_received(:deliver_now)
+      end
+    end
+
+    context "when changes are present" do
+      let(:changeset) { double(:changes) }
+      let(:changes) { lambda { changeset } }
+
+      it "logs an issue and sends the email" do
+        expect(summarizer).to receive(:record_issue).with(:changes, order).once
+
+        subject.call
+
+        expect(SubscriptionMailer).to have_received(:placement_email).with(order, changeset)
+        expect(mail_mock).to have_received(:deliver_now)
+      end
+    end
+  end
+
+  describe "#send_empty_email" do
+    let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true) }
+
+    let(:changeset) { double(:changes) }
+    let(:changes) { lambda { changeset } }
+
+    before do
+      allow(SubscriptionMailer).to receive(:empty_email) { mail_mock }
+    end
+
+    it "logs an issue and sends the email" do
+      expect(summarizer).to receive(:record_issue).with(:empty, order).once
+
+      subject.call
+
+      expect(SubscriptionMailer).to have_received(:empty_email).with(order, changeset)
+      expect(mail_mock).to have_received(:deliver_now)
+    end
+  end
+
+  describe "#move_to_completion" do
+    let(:order_cycle) { create(:simple_order_cycle) }
+    let(:shop) { order_cycle.coordinator }
+    let(:order) { create(:order, order_cycle: order_cycle, distributor: shop) }
+    let(:ex) { create(:exchange, order_cycle: order_cycle, sender: shop, receiver: shop, incoming: false) }
+    let(:variant1) { create(:variant, on_hand: 5) }
+    let(:variant2) { create(:variant, on_hand: 5) }
+    let(:variant3) { create(:variant, on_hand: 5) }
+    let!(:line_item1) { create(:line_item, order: order, variant: variant1, quantity: 3) }
+    let!(:line_item2) { create(:line_item, order: order, variant: variant2, quantity: 3) }
+    let!(:line_item3) { create(:line_item, order: order, variant: variant3, quantity: 3) }
+
+    context "and some items are not available from the order cycle" do
+      before { [variant2, variant3].each { |v| ex.variants << v } }
+
+      context "and insufficient stock exists to fulfil the order for some items" do
+        before do
+          variant1.update_attribute(:on_hand, 5)
+          variant2.update_attribute(:on_hand, 2)
+          variant3.update_attribute(:on_hand, 0)
+        end
+
+        context "and the order has been placed" do
+          before do
+            allow(order).to receive(:ensure_available_shipping_rates) { true }
+            allow(order).to receive(:process_each_payment) { true }
+          end
+
+          it "removes the unavailable items from the shipment" do
+            subject.move_to_completion
+            expect(order.reload.shipment.manifest.size).to eq 1
+          end
+        end
+      end
+    end
+  end
+end