From f61cdb6608d66ca0a516bd206bc5b80deb12e866 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 2 Jun 2021 17:42:59 +0200 Subject: [PATCH] Extract CapQuantityAndStoreChanges service This is a responsiblity of its own that makes the job's tests really complex. --- app/jobs/subscription_placement_job.rb | 29 +------ .../cap_quantity_and_store_changes.rb | 42 ++++++++++ app/services/place_order.rb | 8 +- spec/jobs/subscription_placement_job_spec.rb | 75 +++-------------- .../cap_quantity_and_store_changes_spec.rb | 81 +++++++++++++++++++ spec/services/place_order_spec.rb | 48 ++--------- 6 files changed, 143 insertions(+), 140 deletions(-) create mode 100644 app/services/cap_quantity_and_store_changes.rb create mode 100644 spec/services/cap_quantity_and_store_changes_spec.rb diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index 674b6ae2f4..729fac24b2 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -30,36 +30,9 @@ class SubscriptionPlacementJob < ActiveJob::Base proxy_order, summarizer, JobLogger.logger, - lambda { cap_quantity_and_store_changes(proxy_order.order) } + CapQuantityAndStoreChanges.new(proxy_order.order) ) place_order.call end - - def cap_quantity_and_store_changes(order) - changes = {} - order.insufficient_stock_lines.each do |line_item| - changes[line_item.id] = line_item.quantity - line_item.cap_quantity_at_stock! - end - unavailable_stock_lines_for(order).each do |line_item| - changes[line_item.id] = changes[line_item.id] || line_item.quantity - line_item.update(quantity: 0) - - Spree::OrderInventory.new(order).verify(line_item, order.shipment) - end - if changes.present? - order.line_items.reload - order.update_order_fees! - end - changes - end - - def unavailable_stock_lines_for(order) - order.line_items.where('variant_id NOT IN (?)', available_variants_for(order).select(&:id)) - end - - def available_variants_for(order) - OrderCycleDistributedVariants.new(order.order_cycle, order.distributor).available_variants - end end diff --git a/app/services/cap_quantity_and_store_changes.rb b/app/services/cap_quantity_and_store_changes.rb new file mode 100644 index 0000000000..b3a5634277 --- /dev/null +++ b/app/services/cap_quantity_and_store_changes.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +class CapQuantityAndStoreChanges + def initialize(order) + @order = order + end + + def call + changes = {} + + order.insufficient_stock_lines.each do |line_item| + changes[line_item.id] = line_item.quantity + line_item.cap_quantity_at_stock! + end + + unavailable_stock_lines_for.each do |line_item| + changes[line_item.id] = changes[line_item.id] || line_item.quantity + line_item.update(quantity: 0) + + Spree::OrderInventory.new(order).verify(line_item, order.shipment) + end + + if changes.present? + order.line_items.reload + order.update_order_fees! + end + + changes + end + + private + + attr_reader :order + + def unavailable_stock_lines_for + order.line_items.where('variant_id NOT IN (?)', available_variants_for.select(&:id)) + end + + def available_variants_for + OrderCycleDistributedVariants.new(order.order_cycle, order.distributor).available_variants + end +end diff --git a/app/services/place_order.rb b/app/services/place_order.rb index d08aef2868..535a9028dc 100644 --- a/app/services/place_order.rb +++ b/app/services/place_order.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true class PlaceOrder - def initialize(proxy_order, summarizer, logger, changes_loader) + def initialize(proxy_order, summarizer, logger, stock_changes_loader) @proxy_order = proxy_order @subscription = proxy_order.subscription @summarizer = summarizer @logger = logger - @changes_loader = changes_loader + @stock_changes_loader = stock_changes_loader end def call @@ -29,7 +29,7 @@ class PlaceOrder private - attr_reader :proxy_order, :subscription, :order, :summarizer, :logger, :changes_loader, :changes + attr_reader :proxy_order, :subscription, :order, :summarizer, :logger, :stock_changes_loader, :changes def initialise_order logger.info("Placing Order for Proxy Order #{proxy_order.id}") @@ -51,7 +51,7 @@ class PlaceOrder end def load_changes - @changes = changes_loader.call + @changes = stock_changes_loader.call end def empty_order? diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb index 408b6f2245..d27ecf5e1f 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -63,76 +63,19 @@ describe SubscriptionPlacementJob do proxy_order, summarizer, JobLogger.logger, - lambda { job.send(:cap_quantity_and_store_changes, order) } + CapQuantityAndStoreChanges.new(proxy_order.order) ) allow(PlaceOrder).to receive(:new) { service } allow(service).to receive(:call) + job.perform + expect(service).to have_received(:call) end end end - describe "checking that line items are available to purchase" 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 "when all items are available from the order cycle" do - before { [variant1, 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 - - it "caps quantity at the stock level for stock-limited items, and reports the change" do - changes = job.send(:cap_quantity_and_store_changes, order.reload) - expect(line_item1.reload.quantity).to be 3 # not capped - expect(line_item2.reload.quantity).to be 2 # capped - expect(line_item3.reload.quantity).to be 0 # capped - expect(changes[line_item1.id]).to be nil - expect(changes[line_item2.id]).to be 3 - expect(changes[line_item3.id]).to be 3 - end - end - end - - 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 - - it "sets quantity to 0 for unavailable items, and reports the change" do - changes = job.send(:cap_quantity_and_store_changes, order.reload) - expect(line_item1.reload.quantity).to be 0 # unavailable - expect(line_item2.reload.quantity).to be 2 # capped - expect(line_item3.reload.quantity).to be 0 # capped - expect(changes[line_item1.id]).to be 3 - expect(changes[line_item2.id]).to be 3 - expect(changes[line_item3.id]).to be 3 - end - end - end - end - describe "processing a subscription order" do let!(:shipping_method_created_earlier) { create(:shipping_method, distributors: [shop]) } let!(:shipping_method) { create(:shipping_method, distributors: [shop]) } @@ -162,7 +105,7 @@ describe SubscriptionPlacementJob do proxy_order, summarizer, JobLogger.logger, - lambda { job.send(:cap_quantity_and_store_changes, order) } + CapQuantityAndStoreChanges.new(order) ) ActionMailer::Base.deliveries.clear @@ -189,8 +132,10 @@ describe SubscriptionPlacementJob do end context "when no stock items are available after capping stock" do + let(:store_changes) { CapQuantityAndStoreChanges.new(order) } + before do - allow(job).to receive(:unavailable_stock_lines_for) { order.line_items } + allow(store_changes).to receive(:unavailable_stock_lines_for) { order.line_items } end it "does not place the order, clears all adjustments, and sends an empty_order email" do @@ -199,13 +144,13 @@ describe SubscriptionPlacementJob do proxy_order, summarizer, JobLogger.logger, - lambda { job.send(:cap_quantity_and_store_changes, order) } + 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 { 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 @@ -223,7 +168,7 @@ describe SubscriptionPlacementJob do proxy_order, summarizer, JobLogger.logger, - lambda { job.send(:cap_quantity_and_store_changes, order) } + CapQuantityAndStoreChanges.new(order) ) end diff --git a/spec/services/cap_quantity_and_store_changes_spec.rb b/spec/services/cap_quantity_and_store_changes_spec.rb new file mode 100644 index 0000000000..2c55ff20a2 --- /dev/null +++ b/spec/services/cap_quantity_and_store_changes_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CapQuantityAndStoreChanges do + describe "checking that line items are available to purchase" 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 "when all items are available from the order cycle" do + before { [variant1, 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 + + it "caps quantity at the stock level for stock-limited items, and reports the change" do + changes = CapQuantityAndStoreChanges.new(order.reload).call + + expect(line_item1.reload.quantity).to be 3 # not capped + expect(line_item2.reload.quantity).to be 2 # capped + expect(line_item3.reload.quantity).to be 0 # capped + expect(changes[line_item1.id]).to be nil + expect(changes[line_item2.id]).to be 3 + expect(changes[line_item3.id]).to be 3 + end + end + end + + 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 + + it "sets quantity to 0 for unavailable items, and reports the change" do + changes = CapQuantityAndStoreChanges.new(order.reload).call + + expect(line_item1.reload.quantity).to be 0 # unavailable + expect(line_item2.reload.quantity).to be 2 # capped + expect(line_item3.reload.quantity).to be 0 # capped + expect(changes[line_item1.id]).to be 3 + 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 } + + order.create_proposed_shipments + end + + it "removes the unavailable items from the shipment" do + expect { + CapQuantityAndStoreChanges.new(order.reload).call + }.to change { + order.reload.shipment.manifest.size + }.from(2).to(1) + end + end + end + end + end +end diff --git a/spec/services/place_order_spec.rb b/spec/services/place_order_spec.rb index 3bc6c76746..ec1337137b 100644 --- a/spec/services/place_order_spec.rb +++ b/spec/services/place_order_spec.rb @@ -5,9 +5,8 @@ require 'spec_helper' describe PlaceOrder do include ActiveSupport::Testing::TimeHelpers - subject { described_class.new(proxy_order, summarizer, logger, changes) } + subject { described_class.new(proxy_order, summarizer, logger, stock_changes_loader) } - let(:changes) { {} } let(:proxy_order) { create(:proxy_order, order: order) } let(:order) { build(:order) } let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer) } @@ -19,7 +18,7 @@ describe PlaceOrder do let!(:subscription) { create(:subscription, with_items: true) } let!(:proxy_order) { create(:proxy_order, subscription: subscription, order: order) } - let(:changes) { lambda { {} } } + let(:stock_changes_loader) { lambda { {} } } let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true, record_issue: true) } before do @@ -49,7 +48,7 @@ describe PlaceOrder do end context "when no changes are present" do - let(:changes) { lambda { {} } } + let(:stock_changes_loader) { lambda { {} } } it "logs a success and sends the email" do expect(summarizer).to receive(:record_success).with(order).once @@ -63,7 +62,7 @@ describe PlaceOrder do context "when changes are present" do let(:changeset) { double(:changes) } - let(:changes) { lambda { changeset } } + let(:stock_changes_loader) { lambda { changeset } } it "logs an issue and sends the email" do expect(summarizer).to receive(:record_issue).with(:changes, order).once @@ -80,7 +79,7 @@ describe PlaceOrder do let(:summarizer) { instance_double(OrderManagement::Subscriptions::Summarizer, record_order: true) } let(:changeset) { double(:changes) } - let(:changes) { lambda { changeset } } + let(:stock_changes_loader) { lambda { changeset } } before do allow(SubscriptionMailer).to receive(:empty_email) { mail_mock } @@ -95,41 +94,4 @@ describe PlaceOrder do 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