Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autoremove/update item from the cart if stock changed during checkout #13121

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
9 changes: 2 additions & 7 deletions app/controllers/concerns/order_stock_check.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ def valid_order_line_items?
end

def handle_insufficient_stock
return if sufficient_stock?
stock_service = Orders::CheckStockService.new(@order)
return if stock_service.sufficient_stock?

flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity)
redirect_to main_app.cart_path
Expand All @@ -33,10 +34,4 @@ def check_order_cycle_expiry
format.html { redirect_to main_app.shop_path, status: :see_other }
end
end

private

def sufficient_stock?
@sufficient_stock ||= @order.insufficient_stock_lines.blank?
end
end
15 changes: 15 additions & 0 deletions app/services/orders/check_stock_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

module Orders
class CheckStockService
attr_reader :order

def initialize(order: )
@order = order
end

def sufficient_stock?
@sufficient_stock ||= order.insufficient_stock_lines.blank?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of caching doesn't work with boolean values. If the result is false then it gets re-computed on every call. I think that you need to check for nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it was just copy and paste and broken before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we're checking if defined?, the assignment should always apply. So the || is redundant.

Suggested change
@sufficient_stock ||= order.insufficient_stock_lines.blank?
@sufficient_stock = order.insufficient_stock_lines.blank?

Would be nice to see a spec to prove it the fix :P

end
end
end
24 changes: 24 additions & 0 deletions spec/services/orders/check_stock_service_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

require 'spec_helper'

RSpec.describe Orders::CheckStockService do
subject { described_class.new(order:) }

let(:order) { create(:order_with_line_items) }

describe "#sufficient_stock?" do
it "returns true if enough stock" do
expect(subject.sufficient_stock?).to be(true)
end

context "when one or more item are out of stock" do
it "returns false" do
variant = order.line_items.first.variant
variant.update!(on_demand: false, on_hand: 0)

expect(subject.sufficient_stock?).to be(false)
end
end
end
end