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

[Spree Upgrade] Remove variant.count_on_hand (keep variant.on_hand only) #3513

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 9 additions & 40 deletions app/models/concerns/variant_stock.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,61 +20,30 @@ module VariantStock
after_update :save_stock
end

# Returns the number of items of the variant available in the stock. When
# allowing on demand, it returns infinite.
#
# Spree computes it as the sum of the count_on_hand of all its stock_items.
# Returns the number of items of the variant available.
# Spree computes total_on_hand as the sum of the count_on_hand of all its stock_items.
#
# @return [Float|Integer]
def on_hand
warn_deprecation(__method__, '#total_on_hand')

if on_demand
Float::INFINITY
Copy link
Member

Choose a reason for hiding this comment

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

This is a step backwards. We changed this because Spree introduced this logic of returning infinity. This change would make us less compatible with Spree again. I would prefer to adjust the cart logic or the serializer to deal with infinity. The 9999 value in the cart logic is just a workaround for not dealing with infinity properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a step forward Maikel. I will explain.
Returning Infinity is poor, it's magic, the client should get specific on_hand and on_demand values all the time and the client should decide what to do, no magic.

Also, one important detail for this logic is that in spree, and soon in ofn, on_demand means backorderable: that means that even if the item is backorderable (which will be the on_demand field), the on_hand value will still be used. Basically stock can go to negative.

So, this is a move in the direction of making on_demand equal to backorderable and also it removes magic.

did I convince you? :-D

Copy link
Contributor Author

@luisramos0 luisramos0 Feb 22, 2019

Choose a reason for hiding this comment

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

"The 9999 value in the cart logic is just a workaround"

I am not too worried with client side code here (although I have tested it and it works), we can improve it later.
Here we are changing OFN core behaviour, and I think that it is important to be very clear and simple, i.e., not mixing on_hand and on_demand in the core. Leaving that up to the client.

Copy link
Member

Choose a reason for hiding this comment

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

I really liked the infinity solution as it seemed mathematically correct and solving all the problems. But you convinced me with the different use of on_hand. There are two different uses that Spree started to separate:

  • How many can I order? Infinity is right here.
  • How many are in stock? That's a finite number of the stock item.

And on_hand sounds a bit more like the answer to the second question.

else
total_on_hand
end
end

# Returns the number of items available in the stock for this variant
#
# @return [Float|Integer]
def count_on_hand
warn_deprecation(__method__, '#total_on_hand')
total_on_hand
end

# Sets the stock level when `track_inventory_levels` config is
# set. It raises otherwise.
# Sets the stock level of the variant.
# This will only work if `track_inventory_levels` config is set
# and if there is a stock item for the variant.
#
# @raise [StandardError] when the track_inventory_levels config
# key is not set.
# @raise [StandardError] when the track_inventory_levels config key is not set
# and when the variant has no stock item
def on_hand=(new_level)
warn_deprecation(__method__, '#total_on_hand')

error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
raise error unless Spree::Config.track_inventory_levels

self.count_on_hand = new_level
end

# Sets the stock level. As opposed to #on_hand= it does not check
# `track_inventory_levels`'s value as it was previously an ActiveModel
# setter of the database column of the `spree_variants` table. That is why
# #on_hand= is more widely used in Spree's codebase using #count_on_hand=
# underneath.
#
# So, if #count_on_hand= is used, `track_inventory_levels` won't be taken
# into account thus dismissing instance's configuration.
#
# It does ensure there's a stock item for the variant however. See
# #raise_error_if_no_stock_item_available for details.
#
# @raise [StandardError] when the variant has no stock item yet
def count_on_hand=(new_level)
warn_deprecation(__method__, '#total_on_hand')

raise_error_if_no_stock_item_available

overwrite_stock_levels(new_level)
end

Expand Down Expand Up @@ -131,7 +100,7 @@ def can_supply?(quantity)
# Here we depend only on variant.total_on_hand and variant.on_demand.
# This way, variant_overrides only need to override variant.total_on_hand and variant.on_demand.
def fill_status(quantity)
if count_on_hand >= quantity
if on_hand >= quantity
on_hand = quantity
backordered = 0
else
Expand Down
54 changes: 16 additions & 38 deletions spec/models/concerns/variant_stock_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@
end

describe '#on_hand' do
context 'when the variant is ordered on demand' do
context 'when the variant is on demand' do
before do
variant.stock_items.first.update_attribute(
:backorderable, true
)
end

it 'returns infinite' do
expect(variant.on_hand).to eq(Float::INFINITY)
it 'returns the total items in stock anyway' do
expect(variant.on_hand).to eq(variant.stock_items.sum(&:count_on_hand))
end
end

Expand All @@ -44,28 +44,27 @@
end
end

describe '#count_on_hand' do
before { allow(variant).to receive(:total_on_hand) }

it 'delegates to #total_on_hand' do
variant.count_on_hand
expect(variant).to have_received(:total_on_hand)
end
end

describe '#on_hand=' do
context 'when track_inventory_levels is set' do
before do
allow(variant).to receive(:count_on_hand=)
allow(Spree::Config)
.to receive(:track_inventory_levels) { true }
end

it 'delegates to #count_on_hand=' do
it 'sets the new level as the stock item\'s count_on_hand' do
variant.on_hand = 3
expect(variant)
.to have_received(:count_on_hand=).with(3)
unique_stock_item = variant.stock_items.first
expect(unique_stock_item.count_on_hand).to eq(3)
end

context 'when the variant has no stock item' do
let(:variant) { build(:variant) }

it 'raises' do
expect { variant.on_hand = 3 }
.to raise_error(StandardError)
end
end
end

context 'when track_inventory_levels is not set' do
Expand All @@ -81,27 +80,6 @@
end
end

describe '#count_on_hand=' do
context 'when the variant has a stock item' do
let(:variant) { create(:variant) }

it 'sets the new level as the stock item\'s count_on_hand' do
variant.count_on_hand = 3
unique_stock_item = variant.stock_items.first
expect(unique_stock_item.count_on_hand).to eq(3)
end
end

context 'when the variant has no stock item' do
let(:variant) { build(:variant) }

it 'raises' do
expect { variant.count_on_hand = 3 }
.to raise_error(StandardError)
end
end
end

describe '#on_demand' do
context 'when the variant has a stock item' do
let(:variant) { create(:variant) }
Expand Down Expand Up @@ -187,7 +165,7 @@
end

context 'when variant out of stock' do
before { variant.count_on_hand = 0 }
before { variant.on_hand = 0 }

it "returns true for zero" do
expect(variant.can_supply?(0)).to eq(true)
Expand Down