From 1f3560e96472d52e43110bf668600fb43be02c3f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 23 Jun 2021 12:42:04 +0200 Subject: [PATCH] Pass in the order when it's already initialized At `CapQuantity`'s instantiation time the proxy's order is not yet initialized, and so `CapQuantity` was checking against nil all the time. This went unnoticed because the job's specs were not integration-level tests and were stubbing way too many things. --- app/jobs/subscription_placement_job.rb | 2 +- app/services/cap_quantity.rb | 7 ++- app/services/place_proxy_order.rb | 8 ++- spec/jobs/subscription_placement_job_spec.rb | 58 ++++++-------------- spec/services/cap_quantity_spec.rb | 6 +- spec/services/place_proxy_order_spec.rb | 49 +++++++++++++---- 6 files changed, 66 insertions(+), 64 deletions(-) diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index fe270c3a45..254cf3958b 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -26,6 +26,6 @@ class SubscriptionPlacementJob < ActiveJob::Base end def place_order_for(proxy_order) - PlaceProxyOrder.new(proxy_order, summarizer, JobLogger.logger, CapQuantity.new(proxy_order.order)).call + PlaceProxyOrder.new(proxy_order, summarizer, JobLogger.logger, CapQuantity.new).call end end diff --git a/app/services/cap_quantity.rb b/app/services/cap_quantity.rb index afab26e227..ca81bf098b 100644 --- a/app/services/cap_quantity.rb +++ b/app/services/cap_quantity.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true class CapQuantity - def initialize(order) - @order = order + def initialize @changes = {} end - def call + def call(order) + @order = order + cap_insufficient_stock! verify_line_items diff --git a/app/services/place_proxy_order.rb b/app/services/place_proxy_order.rb index ffce95c12a..fa505c989b 100644 --- a/app/services/place_proxy_order.rb +++ b/app/services/place_proxy_order.rb @@ -7,6 +7,7 @@ class PlaceProxyOrder @summarizer = summarizer @logger = logger @stock_changes_loader = stock_changes_loader + @order = nil end def call @@ -29,12 +30,13 @@ class PlaceProxyOrder private - attr_reader :proxy_order, :subscription, :order, :summarizer, :logger, :stock_changes_loader, :changes + attr_reader :proxy_order, :subscription, :summarizer, :logger, :stock_changes_loader, :changes + attr_accessor :order def initialise_order logger.info("Placing Order for Proxy Order #{proxy_order.id}") - @order = proxy_order.initialise_order! + self.order = proxy_order.initialise_order! if order.nil? summarizer.record_subscription_issue(subscription) @@ -51,7 +53,7 @@ class PlaceProxyOrder end def load_changes - @changes = stock_changes_loader.call + @changes = stock_changes_loader.call(order) end def empty_order? diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb index d2e65f4b1d..e44caf472b 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe SubscriptionPlacementJob do let(:job) { SubscriptionPlacementJob.new } + let(:summarizer) { OrderManagement::Subscriptions::Summarizer.new } describe "finding proxy_orders that are ready to be placed" do let(:shop) { create(:distributor_enterprise) } @@ -59,13 +60,7 @@ describe SubscriptionPlacementJob do end it "processes placeable proxy_orders" do - summarizer = instance_double(OrderManagement::Subscriptions::Summarizer) - service = PlaceProxyOrder.new( - proxy_order, - summarizer, - JobLogger.logger, - CapQuantity.new(proxy_order.order) - ) + service = PlaceProxyOrder.new(proxy_order, summarizer, JobLogger.logger, CapQuantity.new) allow(PlaceProxyOrder).to receive(:new) { service } allow(service).to receive(:call) @@ -110,44 +105,35 @@ describe SubscriptionPlacementJob do context "when the order is not already complete" do context "when no stock items are available after capping stock" do - let(:store_changes) { CapQuantity.new(order) } + let(:service) do + PlaceProxyOrder.new(proxy_order, summarizer, JobLogger.logger, store_updater) + end + let(:store_updater) { CapQuantity.new } before do - allow(store_changes).to receive(:unavailable_stock_lines_for) { order.line_items } + fake_relation = instance_double(ActiveRecord::Relation, select: -123) + allow(store_updater).to receive(:available_variants_for).and_return(fake_relation) end it "does not place the order, clears all adjustments, and sends an empty_order email" do - summarizer = instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true, record_issue: true) - service = PlaceProxyOrder.new( - proxy_order, - summarizer, - JobLogger.logger, - store_changes - ) - 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 + service.call + + expect(proxy_order.order.reload.completed_at).to be_nil + expect(proxy_order.order.all_adjustments).to be_empty + expect(proxy_order.order.total).to eq 0 + expect(proxy_order.order.adjustment_total).to eq 0 + 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 - PlaceProxyOrder.new( - proxy_order, - summarizer, - JobLogger.logger, - CapQuantity.new(order) - ) + PlaceProxyOrder.new(proxy_order, summarizer, JobLogger.logger, CapQuantity.new) end before do @@ -180,17 +166,5 @@ describe SubscriptionPlacementJob do end end end - - context "when the proxy order fails to generate an order" do - before do - allow(proxy_order).to receive(:order) { nil } - end - - it "records an error " do - 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 end diff --git a/spec/services/cap_quantity_spec.rb b/spec/services/cap_quantity_spec.rb index 8d1c36d5ef..5ddcd9f7b4 100644 --- a/spec/services/cap_quantity_spec.rb +++ b/spec/services/cap_quantity_spec.rb @@ -35,7 +35,7 @@ describe CapQuantity do end it "caps quantity at the stock level for stock-limited items, and reports the change" do - changes = CapQuantity.new(order).call + changes = CapQuantity.new.call(order) expect(line_item1.reload.quantity).to be 3 # not capped expect(line_item2.reload.quantity).to be 2 # capped @@ -58,7 +58,7 @@ describe CapQuantity do end it "sets quantity to 0 for unavailable items, and reports the change" do - changes = CapQuantity.new(order).call + changes = CapQuantity.new.call(order) expect(line_item1.reload.quantity).to be 0 # unavailable expect(line_item2.reload.quantity).to be 2 # capped @@ -77,7 +77,7 @@ describe CapQuantity do end it "removes the unavailable items from the shipment" do - expect { CapQuantity.new(order).call } + expect { CapQuantity.new.call(order) } .to change { order.reload.shipment.manifest.size }.from(2).to(1) end end diff --git a/spec/services/place_proxy_order_spec.rb b/spec/services/place_proxy_order_spec.rb index 0e3d2b9495..0e1ea14033 100644 --- a/spec/services/place_proxy_order_spec.rb +++ b/spec/services/place_proxy_order_spec.rb @@ -9,7 +9,7 @@ describe PlaceProxyOrder do let(:proxy_order) { create(:proxy_order, order: order) } let(:order) { build(:order, distributor: build(:enterprise)) } - let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer) } + let(:summarizer) { OrderManagement::Subscriptions::Summarizer.new } let(:logger) { instance_double(JobLogger.logger.class, info: true) } let(:mail_mock) { double(:mailer_mock, deliver_now: true) } @@ -18,10 +18,11 @@ describe PlaceProxyOrder do let!(:subscription) { create(:subscription, with_items: true) } let!(:proxy_order) { create(:proxy_order, subscription: subscription, order: order) } - let(:stock_changes_loader) { lambda { {} } } - let(:summarizer) { OrderManagement::Subscriptions::Summarizer.new } + let(:changes) { {} } + let(:stock_changes_loader) { instance_double(CapQuantity) } before do + allow(stock_changes_loader).to receive(:call).and_return(changes) allow(SubscriptionMailer).to receive(:empty_email) { mail_mock } end @@ -85,6 +86,22 @@ describe PlaceProxyOrder do end end end + + context "when the proxy order fails to generate an order" do + before do + allow(proxy_order).to receive(:initialise_order!) { nil } + end + + it "records an error" do + expect(summarizer).to receive(:record_subscription_issue) + subject.call + end + + it 'does not process the proxy order' do + subject.call + expect(proxy_order.reload.placed_at).to be_nil + end + end end describe "#send_placement_email" do @@ -94,9 +111,7 @@ describe PlaceProxyOrder do 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) @@ -104,7 +119,12 @@ describe PlaceProxyOrder do end context "when no changes are present" do - let(:stock_changes_loader) { lambda { {} } } + let(:changes) { {} } + let(:stock_changes_loader) { instance_double(CapQuantity) } + + before do + allow(stock_changes_loader).to receive(:call).with(order).and_return(changes) + end it "logs a success and sends the email" do expect(summarizer).to receive(:record_success).with(order).once @@ -117,15 +137,19 @@ describe PlaceProxyOrder do end context "when changes are present" do - let(:changeset) { double(:changes) } - let(:stock_changes_loader) { lambda { changeset } } + let(:changes) { double(:changes) } + let(:stock_changes_loader) { instance_double(CapQuantity) } + + before do + allow(stock_changes_loader).to receive(:call).with(order).and_return(changes) + end 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(SubscriptionMailer).to have_received(:placement_email).with(order, changes) expect(mail_mock).to have_received(:deliver_now) end end @@ -136,10 +160,11 @@ describe PlaceProxyOrder do instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true) } - let(:changeset) { double(:changes) } - let(:stock_changes_loader) { lambda { changeset } } + let(:changes) { double(:changes) } + let(:stock_changes_loader) { instance_double(CapQuantity) } before do + allow(stock_changes_loader).to receive(:call).with(order).and_return(changes) allow(SubscriptionMailer).to receive(:empty_email) { mail_mock } end @@ -148,7 +173,7 @@ describe PlaceProxyOrder do subject.call - expect(SubscriptionMailer).to have_received(:empty_email).with(order, changeset) + expect(SubscriptionMailer).to have_received(:empty_email).with(order, changes) expect(mail_mock).to have_received(:deliver_now) end end