From eea227bc22d5392a36202909f5c8922da9e31015 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Jul 2024 13:10:20 +1000 Subject: [PATCH 1/5] Style order spec block a tiny bit --- spec/models/spree/order_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 50ef24dc72..eb736345ce 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -216,8 +216,9 @@ RSpec.describe Spree::Order do end end - context "#finalize!" do - let(:order) { Spree::Order.create } + describe "#finalize!" do + subject(:order) { Spree::Order.create } + it "should set completed_at" do expect(order).to receive(:touch).with(:completed_at) order.finalize! From 94d560d34197f2f0762bb446c4ad8f673c54ca0f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Jul 2024 13:19:39 +1000 Subject: [PATCH 2/5] Replace expecting method call with outcome This is more realistic and robust. Don't mock the class under test (even though `touch` is actually provided by Active Record). --- spec/models/spree/order_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index eb736345ce..1b12fcbc3d 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -220,8 +220,12 @@ RSpec.describe Spree::Order do subject(:order) { Spree::Order.create } it "should set completed_at" do - expect(order).to receive(:touch).with(:completed_at) - order.finalize! + expect { + order.finalize! + order.reload + }.to change { + order.completed_at + }.from(nil) end it "should sell inventory units" do From cb4e7d6fe3732540c880a522b510c53635bb0cac Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Jul 2024 13:29:58 +1000 Subject: [PATCH 3/5] Fix spec to assert updating shipments The spec was asserting on all shipments of the order but there were one. In consequence, the spec didn't assert anything. Now I set up a shipment that is asserted on. I'm stil not sure how useful this spec is though. --- spec/models/spree/order_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 1b12fcbc3d..7fa0f365c1 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -229,10 +229,12 @@ RSpec.describe Spree::Order do end it "should sell inventory units" do - order.shipments.each do |shipment| - expect(shipment).to receive(:update!) - expect(shipment).to receive(:finalize!) - end + shipment = Spree::Shipment.new + order.shipments = [shipment] + + expect(shipment).to receive(:update!).with(order) + expect(shipment).to receive(:finalize!) + order.finalize! end From 2e36c699f6b103fdba5029180bf4250acfe12073 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Jul 2024 15:40:06 +1000 Subject: [PATCH 4/5] Test resulting stock instead of method calls The next test case wasn't asserting anything as well. The referenced method `decrease_stock_for_variant` doesn't actually exist. --- spec/models/spree/order_spec.rb | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 7fa0f365c1..c37fc28033 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -228,21 +228,15 @@ RSpec.describe Spree::Order do }.from(nil) end - it "should sell inventory units" do - shipment = Spree::Shipment.new - order.shipments = [shipment] + it "updates shipments and decreases stock" do + order = create(:order_ready_for_confirmation) + shipment = order.shipments.first + shipment.update_columns(updated_at: 1.minute.ago) - expect(shipment).to receive(:update!).with(order) - expect(shipment).to receive(:finalize!) - - order.finalize! - end - - it "should decrease the stock for each variant in the shipment" do - order.shipments.each do |shipment| - expect(shipment.stock_location).to receive(:decrease_stock_for_variant) - end - order.finalize! + expect { + order.finalize! + }.to change { order.variants.first.on_hand }.by(-1) + .and change { shipment.updated_at } end it "should change the shipment state to ready if order is paid" do From a37b0eb698c3dd8fe7fd833fabe4ea49e17feaf8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Jul 2024 16:57:07 +1000 Subject: [PATCH 5/5] Replace mocking on tested order object It's more realistic this way. --- spec/models/spree/order_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index c37fc28033..74d384e2c8 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -240,12 +240,12 @@ RSpec.describe Spree::Order do end it "should change the shipment state to ready if order is paid" do - Spree::Shipment.create(order:) - order.shipments.reload + order = create(:order_ready_for_confirmation) - allow(order).to receive_messages(paid?: true, complete?: true) - order.finalize! + order.payments.first.capture! + order.next! # calls `finalize!` order.reload # reload so we're sure the changes are persisted + expect(order.shipment_state).to eq 'ready' end