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 2 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
10 changes: 5 additions & 5 deletions app/assets/javascripts/darkswarm/services/cart.js.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ Darkswarm.factory 'Cart', (CurrentOrder, Variants, $timeout, $http, $modal, $roo

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
li.variant.on_hand = stockLevels[li.variant.id].on_hand
if li.quantity > li.variant.on_hand
li.quantity = li.variant.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
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

if scope.variants.length > 0
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 @@ -214,7 +214,7 @@ def assign_defaults(object, entry)
object.assign_attributes(attribute => setting['value'])
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
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
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
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 @@ -602,7 +602,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
8 changes: 4 additions & 4 deletions spec/features/consumer/shopping/variant_overrides_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
expect do
expect do
complete_checkout
end.to change { product1_variant3.reload.count_on_hand }.by(0)
end.to change { product1_variant3.reload.on_hand }.by(0)
end.to change { product1_variant3_override.reload.count_on_hand }.by(-2)
end

Expand All @@ -134,7 +134,7 @@
expect do
expect do
complete_checkout
end.to change { product3_variant2.reload.count_on_hand }.by(0)
end.to change { product3_variant2.reload.on_hand }.by(0)
end.to change { product3_variant2_override.reload.count_on_hand }.by(-2)
end

Expand All @@ -143,12 +143,12 @@
click_checkout
expect do
complete_checkout
end.to change { product1_variant1.reload.count_on_hand }.by(-2)
end.to change { product1_variant1.reload.on_hand }.by(-2)
product1_variant1_override.reload.count_on_hand.should be_nil
end

it "does not show out of stock flags on order confirmation page" do
product1_variant3.count_on_hand = 0
product1_variant3.on_hand = 0
fill_in "variants[#{product1_variant3.id}]", with: "2"
click_checkout

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe "VariantOverridesCtrl", ->
beforeEach ->
# Ideally, count_on_hand is blank when the variant is on demand. However, this rule is not
# enforced.
variant = {id: 2, on_demand: true, count_on_hand: 20, on_hand: "On demand"}
variant = {id: 2, on_demand: true, on_hand: 20, on_hand: "On demand"}

it "clears count_on_hand when variant override uses producer stock settings", ->
scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1}
Expand Down Expand Up @@ -162,7 +162,7 @@ describe "VariantOverridesCtrl", ->
beforeEach ->
# Ideally, count_on_hand is blank when the variant is on demand. However, this rule is not
# enforced.
variant = {id: 2, on_demand: true, count_on_hand: 20, on_hand: t("on_demand")}
variant = {id: 2, on_demand: true, on_hand: 20, on_hand: t("on_demand")}

it "is 'On demand' when variant override uses producer stock settings", ->
scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1}
Expand All @@ -183,7 +183,7 @@ describe "VariantOverridesCtrl", ->
variant = null

beforeEach ->
variant = {id: 2, on_demand: false, count_on_hand: 20, on_hand: 20}
variant = {id: 2, on_demand: false, on_hand: 20, on_hand: 20}

it "is variant count on hand when variant override uses producer stock settings", ->
scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1}
Expand Down
2 changes: 1 addition & 1 deletion spec/javascripts/unit/bulk_product_update_spec.js.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ describe "filtering products for submission to database", ->
shipping_category_id: null
created_at: null
updated_at: null
count_on_hand: 0
on_hand: 0
producer_id: 5

group_buy: null
Expand Down
6 changes: 3 additions & 3 deletions spec/javascripts/unit/darkswarm/services/cart_spec.js.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,11 @@ describe 'Cart service', ->
expect(li.max_quantity).toEqual 0

it "resets the count on hand available", ->
li = {variant: {id: 1, count_on_hand: 10}, quantity: 5}
li = {variant: {id: 1, on_hand: 10}, quantity: 5}
Cart.line_items = [li]
stockLevels = {1: {quantity: 0, max_quantity: 0, on_hand: 0}}
Cart.compareAndNotifyStockLevels stockLevels
expect(li.variant.count_on_hand).toEqual 0
expect(li.variant.on_hand).toEqual 0

describe "when the quantity available is less than that requested", ->
it "reduces the quantity in the cart", ->
Expand All @@ -172,7 +172,7 @@ describe 'Cart service', ->
Cart.line_items = [li]
stockLevels = {1: {quantity: 5, on_hand: 6}}
Cart.compareAndNotifyStockLevels stockLevels
expect(li.variant.count_on_hand).toEqual 6
expect(li.variant.on_hand).toEqual 6

describe "when the client-side quantity has been increased during the request", ->
it "does not reset the quantity", ->
Expand Down
Loading