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 all commits
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
24 changes: 13 additions & 11 deletions app/assets/javascripts/darkswarm/services/cart.js.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,20 @@ Darkswarm.factory 'Cart', (CurrentOrder, Variants, $timeout, $http, $modal, $roo
scope = $rootScope.$new(true)
scope.variants = []

# TODO: These changes to quantity/max_quantity trigger another cart update, which
# is unnecessary.

# TODO: These changes to quantity/max_quantity trigger another cart update, which is unnecessary.
for li in @line_items when li.quantity > 0
if stockLevels[li.variant.id]?
li.variant.count_on_hand = stockLevels[li.variant.id].on_hand
if li.quantity > li.variant.count_on_hand
li.quantity = li.variant.count_on_hand
scope.variants.push li.variant
if li.variant.count_on_hand == 0 && li.max_quantity > li.variant.count_on_hand
li.max_quantity = li.variant.count_on_hand
scope.variants.push(li.variant) unless li.variant in scope.variants
continue unless stockLevels[li.variant.id]?

li.variant.on_hand = stockLevels[li.variant.id].on_hand
li.variant.on_demand = stockLevels[li.variant.id].on_demand
continue if li.variant.on_demand

if li.quantity > li.variant.on_hand
li.quantity = li.variant.on_hand
scope.variants.push li.variant
if li.variant.on_hand == 0 && li.max_quantity > li.variant.on_hand
li.max_quantity = li.variant.on_hand
scope.variants.push(li.variant) unless li.variant in scope.variants
Copy link
Contributor

@sauloperez sauloperez Feb 19, 2019

Choose a reason for hiding this comment

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

can we add some blank lines in between? It's feel like a wall 😱

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 have added some line breaks... I dont want to change this logic but it sure is calling for some improvement...

Copy link
Contributor

@sauloperez sauloperez Feb 22, 2019

Choose a reason for hiding this comment

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

not changing logic but a bit of spacing would ease the reading...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive added some spaces, I am not sure if you are ok with it now or if you are asking for more spaces? :-D


if scope.variants.length > 0
$modal.open(templateUrl: "out_of_stock.html", scope: scope, windowClass: 'out-of-stock-modal')
Expand Down
6 changes: 3 additions & 3 deletions app/assets/javascripts/templates/out_of_stock.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

%p{'ng-repeat' => "v in variants"}
%em {{ v.name_to_display }} - {{ v.unit_to_display }}
%span{'ng-if' => "v.count_on_hand == 0"}
%span{'ng-if' => "v.on_hand == 0"}
{{ 'js.out_of_stock.now_out_of_stock' | t }}
%span{'ng-if' => "v.count_on_hand > 0"}
{{ 'js.out_of_stock.only_n_remainging' | t:{ num: v.count_on_hand } }}
%span{'ng-if' => "v.on_hand > 0"}
{{ 'js.out_of_stock.only_n_remainging' | t:{ num: v.on_hand } }}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
placeholder: "0",
"ofn-disable-scroll" => true,
"ng-model" => "variant.line_item.quantity",
"ofn-on-hand" => "{{variant.on_demand && 9999 || variant.count_on_hand }}",
"ng-disabled" => "!variant.on_demand && variant.count_on_hand == 0",
"ofn-on-hand" => "{{variant.on_demand && 9999 || variant.on_hand }}",
"ng-disabled" => "!variant.on_demand && variant.on_hand == 0",
name: "variants[{{::variant.id}}]", id: "variants_{{::variant.id}}"}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"ng-model" => "variant.line_item.quantity",
placeholder: "{{::'shop_variant_quantity_min' | t}}",
"ofn-disable-scroll" => true,
"ofn-on-hand" => "{{variant.on_demand && 9999 || variant.count_on_hand }}",
"ofn-on-hand" => "{{variant.on_demand && 9999 || variant.on_hand }}",
name: "variants[{{::variant.id}}]", id: "variants_{{::variant.id}}"}
%span.bulk-input
%input.bulk.second{type: :number,
Expand Down
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
2 changes: 1 addition & 1 deletion app/models/product_import/entry_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ def assign_defaults(object, entry)
end
when 'overwrite_empty'
if object.public_send(attribute).blank? ||
((attribute == 'on_hand' || attribute == 'count_on_hand') &&
((attribute == 'on_hand') &&
entry.on_hand_nil)

object.assign_attributes(attribute => setting['value'])
Expand Down
1 change: 0 additions & 1 deletion app/models/product_import/entry_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,6 @@ def check_on_hand_nil(entry, object)
return if entry.on_hand.present?

object.on_hand = 0 if object.respond_to?(:on_hand)
object.count_on_hand = 0 if object.respond_to?(:count_on_hand)
entry.on_hand_nil = true
end

Expand Down
12 changes: 6 additions & 6 deletions app/models/product_import/products_reset_strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def reset(enterprise_ids)

return 0 if enterprise_ids.blank?

reset_variants_count_on_hand(enterprise_variants_relation)
reset_variants_on_hand(enterprise_variants_relation)
end

private
Expand All @@ -29,17 +29,17 @@ def enterprise_variants_relation
relation.where('spree_variants.id NOT IN (?)', excluded_items_ids)
end

def reset_variants_count_on_hand(variants)
def reset_variants_on_hand(variants)
updated_records_count = 0
variants.each do |variant|
updated_records_count += 1 if reset_variant_count_on_hand(variant)
updated_records_count += 1 if reset_variant_on_hand(variant)
end
updated_records_count
end

def reset_variant_count_on_hand(variant)
variant.count_on_hand = 0
variant.count_on_hand.zero?
def reset_variant_on_hand(variant)
variant.on_hand = 0
variant.on_hand.zero?
end
end
end
2 changes: 1 addition & 1 deletion app/models/product_import/spreadsheet_entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class SpreadsheetEntry
:distributor_id, :name, :display_name, :sku, :unit_value,
:unit_description, :variant_unit, :variant_unit_scale,
:variant_unit_name, :display_as, :category, :primary_taxon_id,
:price, :on_hand, :count_on_hand, :on_demand,
:price, :on_hand, :on_demand,
:tax_category_id, :shipping_category_id, :description,
:import_date, :enterprise, :enterprise_id

Expand Down
1 change: 1 addition & 0 deletions app/models/spree/line_item_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@

def cap_quantity_at_stock!
scoper.scope(variant)
return if variant.on_demand
update_attributes!(quantity: variant.on_hand) if quantity > variant.on_hand
end

Expand Down
3 changes: 2 additions & 1 deletion app/serializers/api/admin/product_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ def thumb_url
end

def on_hand
object.on_hand.nil? ? 0 : object.on_hand.to_f.finite? ? object.on_hand : I18n.t(:on_demand)
return 0 if object.on_hand.nil?
object.on_hand
end

def price
Expand Down
3 changes: 2 additions & 1 deletion app/serializers/api/admin/variant_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ class Api::Admin::VariantSerializer < ActiveModel::Serializer
has_many :variant_overrides

def on_hand
object.on_hand.nil? ? 0 : ( object.on_hand.to_f.finite? ? object.on_hand : I18n.t(:on_demand) )
return 0 if object.on_hand.nil?
object.on_hand
end

def price
Expand Down
2 changes: 1 addition & 1 deletion app/serializers/api/variant_serializer.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class Api::VariantSerializer < ActiveModel::Serializer
attributes :id, :is_master, :count_on_hand, :name_to_display, :unit_to_display, :unit_value
attributes :id, :is_master, :on_hand, :name_to_display, :unit_to_display, :unit_value
attributes :options_text, :on_demand, :price, :fees, :price_with_fees, :product_name
attributes :tag_list

Expand Down
2 changes: 1 addition & 1 deletion app/services/order_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def build_line_items
attrs[:line_items].each do |li|
next unless variant = Spree::Variant.find_by_id(li[:variant_id])
scoper.scope(variant)
li[:quantity] = stock_limited_quantity(variant.count_on_hand, li[:quantity])
li[:quantity] = stock_limited_quantity(variant.on_hand, li[:quantity])
li[:price] = variant.price
build_item_from(li)
end
Expand Down
13 changes: 4 additions & 9 deletions app/services/variants_stock_levels.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ def call(order, requested_variant_ids)
order_variant_ids = variant_stock_levels.keys
missing_variant_ids = requested_variant_ids - order_variant_ids
missing_variant_ids.each do |variant_id|
variant_on_hand = Spree::Variant.find(variant_id).on_hand
variant_stock_levels[variant_id] = { quantity: 0, max_quantity: 0, on_hand: variant_on_hand }
variant = Spree::Variant.find(variant_id)
variant_stock_levels[variant_id] = { quantity: 0, max_quantity: 0, on_hand: variant.on_hand, on_demand: variant.on_demand }
end

# The code above is most probably dead code, this bugsnag notification will confirm it
Expand All @@ -37,14 +37,9 @@ def variant_stock_levels(line_items)
[line_item.variant.id,
{ quantity: line_item.quantity,
max_quantity: line_item.max_quantity,
on_hand: wrap_json_infinity(line_item.variant.on_hand) }]
on_hand: line_item.variant.on_hand,
on_demand: line_item.variant.on_demand }]
end
]
end

# Rails to_json encodes Float::INFINITY as Infinity, which is not valid JSON
# Return it as a large integer (max 32 bit signed int)
def wrap_json_infinity(number)
number == Float::INFINITY ? 2_147_483_647 : number
end
end
2 changes: 1 addition & 1 deletion app/views/shop/products/_form.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
%div.pad-top{ "infinite-scroll" => "incrementLimit()", "infinite-scroll-distance" => "1", "infinite-scroll-disabled" => 'filteredProducts.length <= limit' }
%product.animate-repeat{"ng-controller" => "ProductNodeCtrl", "ng-repeat" => "product in visibleProducts track by product.id", "id" => "product-{{ product.id }}"}
= render "shop/products/summary"
%shop-variant{variant: 'variant', "ng-repeat" => "variant in product.variants | orderBy: ['name_to_display','unit_value'] track by variant.id", "id" => "variant-{{ variant.id }}", "ng-class" => "{'out-of-stock': !variant.on_demand && variant.count_on_hand == 0}"}
%shop-variant{variant: 'variant', "ng-repeat" => "variant in product.variants | orderBy: ['name_to_display','unit_value'] track by variant.id", "id" => "variant-{{ variant.id }}", "ng-class" => "{'out-of-stock': !variant.on_demand && variant.on_hand == 0}"}

%product{"ng-show" => "Products.loading"}
.row.summary
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
%input{ 'ng-model' => 'variant.price', 'ofn-decimal' => :true, :name => 'variant_price', 'ofn-track-variant' => 'price', :type => 'text' }
%td{ 'ng-show' => 'columns.on_hand.visible' }
%input.field{ 'ng-model' => 'variant.on_hand', 'ng-change' => 'updateOnHand(product)', :name => 'variant_on_hand', 'ng-if' => '!variant.on_demand', 'ofn-track-variant' => 'on_hand', :type => 'number' }
%span{ 'ng-bind' => 'variant.on_hand', :name => 'variant_on_hand', 'ng-if' => 'variant.on_demand' }
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the ng-bind not needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want "on demand" to be bound to the on_hand value, ng-bind would do that. the binding is only when variant is not on_demand, the ng-ifs are controlling that.

%span{ :name => 'variant_on_hand', 'ng-if' => 'variant.on_demand' }
= t(:on_demand)
%td{ 'ng-show' => 'columns.on_demand.visible' }
%input.field{ 'ng-model' => 'variant.on_demand', :name => 'variant_on_demand', 'ofn-track-variant' => 'on_demand', :type => 'checkbox' }
%td{ 'ng-show' => 'columns.category.visible' }
Expand Down
6 changes: 3 additions & 3 deletions app/views/spree/admin/variants/_autocomplete.js.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
</ul>
<ul class='variant-data'>
<li class='variant-sku'><strong><%= t('admin.sku') %>:</strong> {{variant.sku}}</li>
<li class='variant-on_hand'><strong><%= t('on_hand') %>:</strong> {{variant.count_on_hand}}</li>
<li class='variant-on_hand'><strong><%= t('on_hand') %>:</strong> {{variant.on_hand}}</li>
</ul>

{{#if variant.option_values}}
Expand All @@ -41,7 +41,7 @@
</colgroup>
<thead>
<th><%= Spree.t(:location) %></th>
<th><%= Spree.t(:count_on_hand) %></th>
<th><%= Spree.t(:on_hand) %></th>
<th><%= Spree.t(:quantity) %></th>
<th class="actions"></th>
</thead>
Expand All @@ -53,7 +53,7 @@
<td>Spree.t(:on_demand)</td>
{{else}}
<td>
{{variant.count_on_hand}}
{{variant.on_hand}}
</td>
{{/if}}
<td>
Expand Down
2 changes: 1 addition & 1 deletion app/views/spree/admin/variants/search.rabl
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# overriding spree/core/app/views/spree/admin/variants/search.rabl
#
collection @variants
attributes :sku, :options_text, :in_stock?, :on_demand, :count_on_hand, :id, :cost_price
attributes :sku, :options_text, :in_stock?, :on_demand, :on_hand, :id, :cost_price

node(:name) do |v|
# TODO: when products must have a unit, full_name will always be present
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/cart_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@
let!(:line_item) { create(:line_item, order: order, variant: variant_in_the_order, quantity: 2, max_quantity: 3) }

before do
variant_in_the_order.count_on_hand = 4
variant_not_in_the_order.count_on_hand = 2
variant_in_the_order.on_hand = 4
variant_not_in_the_order.on_hand = 2
order_cycle.exchanges.outgoing.first.variants = [variant_in_the_order, variant_not_in_the_order]
order.order_cycle = order_cycle
order.distributor = hub
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/spree/admin/variants_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module Admin
it "applies variant overrides" do
spree_get :search, q: 'Prod', distributor_id: d.id.to_s
assigns(:variants).should == [v1]
assigns(:variants).first.count_on_hand.should == 44
assigns(:variants).first.on_hand.should == 44
end

it "filters by order cycle" do
Expand Down
2 changes: 1 addition & 1 deletion spec/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@

after(:create) do |variant, evaluator|
variant.on_demand = evaluator.on_demand
variant.count_on_hand = evaluator.on_hand
variant.on_hand = evaluator.on_hand
variant.save
end
end
Expand Down
6 changes: 3 additions & 3 deletions spec/features/admin/reports_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,11 @@
product2.set_property 'Organic', 'NASAA 12345'
product1.taxons = [taxon]
product2.taxons = [taxon]
variant1.count_on_hand = 10
variant1.on_hand = 10
variant1.update_column(:sku, "sku1")
variant2.count_on_hand = 20
variant2.on_hand = 20
variant2.update_column(:sku, "sku2")
variant3.count_on_hand = 9
variant3.on_hand = 9
variant3.update_column(:sku, "")
variant1.option_values = [create(:option_value, :presentation => "Test")]
variant2.option_values = [create(:option_value, :presentation => "Something")]
Expand Down
Loading