Merge pull request #3230 from luisramos0/stock_levels

3222 Improve stock levels calculation code on cart populate and add specs to test handling of variant overrides
This commit is contained in:
Pau Pérez Fabregat
2019-01-16 12:54:25 +01:00
committed by GitHub
7 changed files with 194 additions and 141 deletions

View File

@@ -23,9 +23,7 @@ Layout/AlignArray:
- 'lib/open_food_network/order_and_distributor_report.rb'
- 'lib/open_food_network/orders_and_fulfillments_report.rb'
- 'lib/open_food_network/packing_report.rb'
- 'spec/controllers/cart_controller_spec.rb'
- 'spec/lib/open_food_network/order_grouper_spec.rb'
- 'spec/services/cart_service_spec.rb'
# Offense count: 122
# Cop supports --auto-correct.
@@ -246,7 +244,6 @@ Layout/EmptyLines:
- 'spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb'
- 'spec/serializers/admin/for_order_cycle/supplied_product_serializer_spec.rb'
- 'spec/serializers/credit_card_serializer_spec.rb'
- 'spec/services/cart_service_spec.rb'
- 'spec/support/delayed_job_helper.rb'
- 'spec/support/matchers/table_matchers.rb'
@@ -646,7 +643,6 @@ Layout/SpaceAroundOperators:
- 'lib/open_food_network/xero_invoices_report.rb'
- 'lib/spree/product_filters.rb'
- 'spec/controllers/admin/enterprises_controller_spec.rb'
- 'spec/controllers/cart_controller_spec.rb'
- 'spec/features/admin/bulk_order_management_spec.rb'
- 'spec/features/admin/bulk_product_update_spec.rb'
- 'spec/features/consumer/shopping/checkout_spec.rb'
@@ -811,7 +807,6 @@ Layout/SpaceInsideHashLiteralBraces:
- 'spec/controllers/admin/order_cycles_controller_spec.rb'
- 'spec/controllers/admin/subscriptions_controller_spec.rb'
- 'spec/controllers/api/statuses_controller_spec.rb'
- 'spec/controllers/cart_controller_spec.rb'
- 'spec/controllers/checkout_controller_spec.rb'
- 'spec/controllers/enterprises_controller_spec.rb'
- 'spec/controllers/shop_controller_spec.rb'
@@ -862,7 +857,6 @@ Layout/SpaceInsideHashLiteralBraces:
- 'spec/requests/checkout/failed_checkout_spec.rb'
- 'spec/requests/checkout/stripe_connect_spec.rb'
- 'spec/serializers/enterprise_serializer_spec.rb'
- 'spec/services/cart_service_spec.rb'
- 'spec/services/order_syncer_spec.rb'
- 'spec/services/subscription_form_spec.rb'
- 'spec/spec_helper.rb'
@@ -900,14 +894,6 @@ Layout/Tab:
- 'spec/lib/spree/product_filters_spec.rb'
- 'spec/models/spree/line_item_spec.rb'
# Offense count: 1
# Cop supports --auto-correct.
# Configuration parameters: EnforcedStyle.
# SupportedStyles: final_newline, final_blank_line
Layout/TrailingBlankLines:
Exclude:
- 'spec/controllers/cart_controller_spec.rb'
# Offense count: 57
# Cop supports --auto-correct.
# Configuration parameters: AllowInHeredoc.
@@ -1390,7 +1376,6 @@ Rails/HttpStatus:
- 'app/controllers/api/enterprise_fees_controller.rb'
- 'app/controllers/api/enterprises_controller.rb'
- 'app/controllers/application_controller.rb'
- 'app/controllers/cart_controller.rb'
- 'app/controllers/checkout_controller.rb'
- 'app/controllers/enterprises_controller.rb'
- 'app/controllers/line_items_controller.rb'
@@ -1564,7 +1549,6 @@ Style/BracesAroundHashParameters:
- 'spec/controllers/admin/manager_invitations_controller_spec.rb'
- 'spec/controllers/admin/order_cycles_controller_spec.rb'
- 'spec/controllers/api/order_cycles_controller_spec.rb'
- 'spec/controllers/cart_controller_spec.rb'
- 'spec/controllers/enterprises_controller_spec.rb'
- 'spec/controllers/line_items_controller_spec.rb'
- 'spec/controllers/spree/admin/adjustments_controller_spec.rb'
@@ -1597,7 +1581,6 @@ Style/BracesAroundHashParameters:
- 'spec/models/spree/product_spec.rb'
- 'spec/models/spree/taxon_spec.rb'
- 'spec/serializers/admin/customer_serializer_spec.rb'
- 'spec/services/cart_service_spec.rb'
- 'spec/spec_helper.rb'
- 'spec/support/cancan_helper.rb'
- 'spec/support/request/authentication_workflow.rb'
@@ -1919,7 +1902,6 @@ Style/HashSyntax:
- 'spec/controllers/admin/stripe_connect_settings_controller_spec.rb'
- 'spec/controllers/api/order_cycles_controller_spec.rb'
- 'spec/controllers/base_controller_spec.rb'
- 'spec/controllers/cart_controller_spec.rb'
- 'spec/controllers/spree/admin/payment_methods_controller_spec.rb'
- 'spec/controllers/spree/admin/payments_controller_spec.rb'
- 'spec/controllers/spree/api/products_controller_spec.rb'
@@ -2007,7 +1989,6 @@ Style/MethodCallWithoutArgsParentheses:
Exclude:
- 'app/controllers/spree/admin/payment_methods_controller_decorator.rb'
- 'app/views/json/_groups.rabl'
- 'spec/controllers/cart_controller_spec.rb'
- 'spec/features/consumer/registration_spec.rb'
- 'spec/models/spree/payment_method_spec.rb'
- 'spec/support/request/ui_component_helper.rb'

View File

@@ -20,28 +20,16 @@ class CartController < BaseController
variant_ids = variant_ids_in(cart_service.variants_h)
render json: { error: false, stock_levels: stock_levels(current_order, variant_ids) },
status: 200
render json: { error: false,
stock_levels: VariantsStockLevels.new.call(current_order, variant_ids) },
status: :ok
else
render json: { error: true }, status: 412
render json: { error: true }, status: :precondition_failed
end
end
populate_variant_attributes
end
# Report the stock levels in the order for all variant ids requested
def stock_levels(order, variant_ids)
stock_levels = li_stock_levels(order)
li_variant_ids = stock_levels.keys
(variant_ids - li_variant_ids).each do |variant_id|
stock_levels[variant_id] = { quantity: 0, max_quantity: 0, on_hand: Spree::Variant.find(variant_id).on_hand }
end
stock_levels
end
def variant_ids_in(variants_h)
variants_h.map { |v| v[:variant_id].to_i }
end
@@ -79,21 +67,4 @@ class CartController < BaseController
max_quantity: max_quantity)
end
end
def li_stock_levels(order)
Hash[
order.line_items.map do |li|
[li.variant.id,
{ quantity: li.quantity,
max_quantity: li.max_quantity,
on_hand: wrap_json_infinity(li.variant.on_hand) }]
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 ? 2147483647 : number
end
end

View File

@@ -80,11 +80,19 @@ class CartService
end
def read_products_hash(data)
# This is most probably dead code, this bugsnag notification will confirm it
notify_bugsnag(data) if data[:products].present?
(data[:products] || []).map do |_product_id, variant_id|
{ variant_id: variant_id, quantity: data[:quantity] }
end
end
def notify_bugsnag(data)
Bugsnag.notify(RuntimeError.new("CartService.populate called with products hash"),
data: data.as_json)
end
def read_variants_hash(data)
(data[:variants] || []).map do |variant_id, quantity|
if quantity.is_a?(Hash)

View File

@@ -0,0 +1,50 @@
# Report the stock levels of:
# - all variants in the order
# - all requested variant ids
class VariantsStockLevels
def call(order, requested_variant_ids)
variant_stock_levels = variant_stock_levels(order.line_items)
# Variants are not scoped here and so the stock levels reported are incorrect
# See cart_controller_spec for more details and #3222
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 }
end
# The code above is most probably dead code, this bugsnag notification will confirm it
notify_bugsnag(order, requested_variant_ids, order_variant_ids) if missing_variant_ids.present?
variant_stock_levels
end
private
def notify_bugsnag(order, requested_variant_ids, order_variant_ids)
error_msg = "VariantsStockLevels.call with variants in the request that are not in the order"
Bugsnag.notify(RuntimeError.new(error_msg),
requested_variant_ids: requested_variant_ids.as_json,
order_variant_ids: order_variant_ids.as_json,
order: order.as_json,
line_items: order.line_items.as_json)
end
def variant_stock_levels(line_items)
Hash[
line_items.map do |line_item|
[line_item.variant.id,
{ quantity: line_item.quantity,
max_quantity: line_item.max_quantity,
on_hand: wrap_json_infinity(line_item.variant.on_hand) }]
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

View File

@@ -3,13 +3,34 @@ require 'spec_helper'
describe CartController, type: :controller do
let(:order) { create(:order) }
describe "returning stock levels in JSON on success" do
let(:product) { create(:simple_product) }
describe "basic behaviour" do
let(:cart_service) { double }
it "returns stock levels as JSON" do
before do
allow(CartService).to receive(:new).and_return(cart_service)
end
it "returns HTTP success when successful" do
allow(cart_service).to receive(:populate) { true }
allow(cart_service).to receive(:variants_h) { {} }
xhr :post, :populate, use_route: :spree, format: :json
expect(response.status).to eq(200)
end
it "returns failure when unsuccessful" do
allow(cart_service).to receive(:populate).and_return false
xhr :post, :populate, use_route: :spree, format: :json
expect(response.status).to eq(412)
end
it "tells cart_service to overwrite" do
expect(cart_service).to receive(:populate).with({}, true)
xhr :post, :populate, use_route: :spree, format: :json
end
it "returns stock levels as JSON on success" do
allow(controller).to receive(:variant_ids_in) { [123] }
allow(controller).to receive(:stock_levels) { 'my_stock_levels' }
allow(CartService).to receive(:new).and_return(cart_service = double())
allow_any_instance_of(VariantsStockLevels).to receive(:call).and_return("my_stock_levels")
allow(cart_service).to receive(:populate) { true }
allow(cart_service).to receive(:variants_h) { {} }
@@ -19,90 +40,67 @@ describe CartController, type: :controller do
expect(data['stock_levels']).to eq('my_stock_levels')
end
describe "generating stock levels" do
let!(:order) { create(:order) }
let!(:li) { create(:line_item, order: order, variant: v, quantity: 2, max_quantity: 3) }
let!(:v) { create(:variant, count_on_hand: 4) }
let!(:v2) { create(:variant, count_on_hand: 2) }
before do
order.reload
allow(controller).to receive(:current_order) { order }
end
it "returns a hash with variant id, quantity, max_quantity and stock on hand" do
expect(controller.stock_levels(order, [v.id])).to eq(
{v.id => {quantity: 2, max_quantity: 3, on_hand: 4}}
)
end
it "includes all line items, even when the variant_id is not specified" do
expect(controller.stock_levels(order, [])).to eq(
{v.id => {quantity: 2, max_quantity: 3, on_hand: 4}}
)
end
it "includes an empty quantity entry for variants that aren't in the order" do
expect(controller.stock_levels(order, [v.id, v2.id])).to eq(
{v.id => {quantity: 2, max_quantity: 3, on_hand: 4},
v2.id => {quantity: 0, max_quantity: 0, on_hand: 2}}
)
end
describe "encoding Infinity" do
let!(:v) { create(:variant, on_demand: true, count_on_hand: 0) }
it "encodes Infinity as a large, finite integer" do
expect(controller.stock_levels(order, [v.id])).to eq(
{v.id => {quantity: 2, max_quantity: 3, on_hand: 2147483647}}
)
end
end
end
it "extracts variant ids from the cart service" do
variants_h = [{:variant_id=>"900", :quantity=>2, :max_quantity=>nil},
{:variant_id=>"940", :quantity=>3, :max_quantity=>3}]
variants_h = [{ variant_id: "900", quantity: 2, max_quantity: nil },
{ variant_id: "940", quantity: 3, max_quantity: 3 }]
expect(controller.variant_ids_in(variants_h)).to eq([900, 940])
end
end
context "handling variant overrides correctly" do
let(:product) { create(:simple_product, supplier: producer) }
let(:producer) { create(:supplier_enterprise) }
let!(:variant_in_the_order) { create(:variant, count_on_hand: 4) }
let!(:variant_not_in_the_order) { create(:variant, count_on_hand: 2) }
let(:hub) { create(:distributor_enterprise, with_payment_and_shipping: true) }
let!(:variant_override_in_the_order) { create(:variant_override, hub: hub, variant: variant_in_the_order, price: 55.55, count_on_hand: 20, default_stock: nil, resettable: false) }
let!(:variant_override_not_in_the_order) { create(:variant_override, hub: hub, variant: variant_not_in_the_order, count_on_hand: 7, default_stock: nil, resettable: false) }
let(:order_cycle) { create(:simple_order_cycle, suppliers: [producer], coordinator: hub, distributors: [hub]) }
let!(:order) { subject.current_order(true) }
let!(:line_item) { create(:line_item, order: order, variant: variant_in_the_order, quantity: 2, max_quantity: 3) }
before do
order_cycle.exchanges.outgoing.first.variants = [variant_in_the_order, variant_not_in_the_order]
order.order_cycle = order_cycle
order.distributor = hub
order.save
end
it "returns the variant override stock levels of the variant in the order" do
spree_post :populate, variants: { variant_in_the_order.id => 1 }
data = JSON.parse(response.body)
expect(data['stock_levels'][variant_in_the_order.id.to_s]["on_hand"]).to eq 20
end
it "returns the variant override stock levels of the variant requested but not in the order" do
# This test passes because the variant requested gets added to the order
# If the variant was not added to the order, VariantsStockLevels alternative calculation would fail
# See #3222 for more details
# This indicates that the VariantsStockLevels alternative calculation is never reached
spree_post :populate, variants: { variant_not_in_the_order.id => 1 }
data = JSON.parse(response.body)
expect(data['stock_levels'][variant_not_in_the_order.id.to_s]["on_hand"]).to eq 7
end
end
context "adding a group buy product to the cart" do
it "sets a variant attribute for the max quantity" do
distributor_product = create(:distributor_enterprise)
p = create(:product, :distributors => [distributor_product], :group_buy => true)
p = create(:product, distributors: [distributor_product], group_buy: true)
order = subject.current_order(true)
allow(order).to receive(:distributor) { distributor_product }
expect(order).to receive(:set_variant_attributes).with(p.master, {'max_quantity' => '3'})
expect(order).to receive(:set_variant_attributes).with(p.master, max_quantity: '3')
allow(controller).to receive(:current_order).and_return(order)
expect do
spree_post :populate, variants: { p.master.id => 1 }, variant_attributes: { p.master.id => {max_quantity: 3 } }
spree_post :populate, variants: { p.master.id => 1 }, variant_attributes: { p.master.id => { max_quantity: 3 } }
end.to change(Spree::LineItem, :count).by(1)
end
it "returns HTTP success when successful" do
allow(CartService).to receive(:new).and_return(cart_service = double())
allow(cart_service).to receive(:populate) { true }
allow(cart_service).to receive(:variants_h) { {} }
xhr :post, :populate, use_route: :spree, format: :json
expect(response.status).to eq(200)
end
it "returns failure when unsuccessful" do
allow(CartService).to receive(:new).and_return(cart_service = double())
allow(cart_service).to receive(:populate).and_return false
xhr :post, :populate, use_route: :spree, format: :json
expect(response.status).to eq(412)
end
it "tells cart_service to overwrite" do
allow(CartService).to receive(:new).and_return(cart_service = double())
expect(cart_service).to receive(:populate).with({}, true)
xhr :post, :populate, use_route: :spree, format: :json
end
end
end

View File

@@ -21,7 +21,7 @@ describe CartService do
describe "populate" do
it "adds a variant" do
cart_service.populate({variants: {v.id.to_s => {quantity: '1', max_quantity: '2'}}}, true)
cart_service.populate({ variants: { v.id.to_s => { quantity: '1', max_quantity: '2' } } }, true)
li = order.find_line_item_by_variant(v)
expect(li).to be
expect(li.quantity).to eq(1)
@@ -32,7 +32,7 @@ describe CartService do
it "updates a variant's quantity, max quantity and final_weight_volume" do
order.add_variant v, 1, 2
cart_service.populate({variants: {v.id.to_s => {quantity: '2', max_quantity: '3'}}}, true)
cart_service.populate({ variants: { v.id.to_s => { quantity: '2', max_quantity: '3' } } }, true)
li = order.find_line_item_by_variant(v)
expect(li).to be
expect(li.quantity).to eq(2)
@@ -43,7 +43,7 @@ describe CartService do
it "removes a variant" do
order.add_variant v, 1, 2
cart_service.populate({variants: {}}, true)
cart_service.populate({ variants: {} }, true)
order.line_items(:reload)
li = order.find_line_item_by_variant(v)
expect(li).not_to be
@@ -73,7 +73,7 @@ describe CartService do
it "attempts cart add with max_quantity" do
allow(cart_service).to receive(:distribution_can_supply_products_in_cart).and_return true
params = {variants: {"1" => {quantity: 1, max_quantity: 2}}}
params = { variants: { "1" => { quantity: 1, max_quantity: 2 } } }
allow(order).to receive(:with_lock).and_yield
allow(cart_service).to receive(:varies_from_cart) { true }
allow(cart_service).to receive(:variants_removed) { [] }
@@ -87,45 +87,45 @@ describe CartService do
it "returns true when item is not in cart and a quantity is specified" do
expect(cart_service).to receive(:line_item_for_variant_id).with(variant.id).and_return(nil)
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '2'})).to be true
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '2')).to be true
end
it "returns true when item is not in cart and a max_quantity is specified" do
expect(cart_service).to receive(:line_item_for_variant_id).with(variant.id).and_return(nil)
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '0', max_quantity: '2'})).to be true
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '0', max_quantity: '2')).to be true
end
it "returns false when item is not in cart and no quantity or max_quantity are specified" do
expect(cart_service).to receive(:line_item_for_variant_id).with(variant.id).and_return(nil)
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '0'})).to be false
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '0')).to be false
end
it "returns true when quantity varies" do
li = double(:line_item, quantity: 1, max_quantity: nil)
allow(cart_service).to receive(:line_item_for_variant_id) { li }
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '2'})).to be true
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '2')).to be true
end
it "returns true when max_quantity varies" do
li = double(:line_item, quantity: 1, max_quantity: nil)
allow(cart_service).to receive(:line_item_for_variant_id) { li }
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '1', max_quantity: '3'})).to be true
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '1', max_quantity: '3')).to be true
end
it "returns false when max_quantity varies only in nil vs 0" do
li = double(:line_item, quantity: 1, max_quantity: nil)
allow(cart_service).to receive(:line_item_for_variant_id) { li }
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '1'})).to be false
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '1')).to be false
end
it "returns false when both are specified and neither varies" do
li = double(:line_item, quantity: 1, max_quantity: 2)
allow(cart_service).to receive(:line_item_for_variant_id) { li }
expect(cart_service.send(:varies_from_cart, {variant_id: variant.id, quantity: '1', max_quantity: '2'})).to be false
expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '1', max_quantity: '2')).to be false
end
end
@@ -137,12 +137,12 @@ describe CartService do
it "returns nothing when all items in the cart are provided" do
allow(cart_service).to receive(:variant_ids_in_cart) { [123] }
expect(cart_service.send(:variants_removed, [{variant_id: '123'}])).to eq([])
expect(cart_service.send(:variants_removed, [{ variant_id: '123' }])).to eq([])
end
it "returns nothing when items are added to cart" do
allow(cart_service).to receive(:variant_ids_in_cart) { [123] }
expect(cart_service.send(:variants_removed, [{variant_id: '123'}, {variant_id: '456'}])).to eq([])
expect(cart_service.send(:variants_removed, [{ variant_id: '123' }, { variant_id: '456' }])).to eq([])
end
it "does not return duplicates" do
@@ -288,7 +288,6 @@ describe CartService do
end
end
describe "support" do
describe "checking if order cycle is required for a variant" do
it "requires an order cycle when the product has no product distributions" do
@@ -307,8 +306,7 @@ describe CartService do
it "provides the distributor and order cycle for the order" do
expect(order).to receive(:distributor).and_return(distributor)
expect(order).to receive(:order_cycle).and_return(order_cycle)
expect(cart_service.send(:distributor_and_order_cycle)).to eq([distributor,
order_cycle])
expect(cart_service.send(:distributor_and_order_cycle)).to eq([distributor, order_cycle])
end
end
end

View File

@@ -0,0 +1,47 @@
require 'spec_helper'
describe VariantsStockLevels do
let(:order) { create(:order) }
let!(:line_item) do
create(:line_item, order: order, variant: variant_in_the_order, quantity: 2, max_quantity: 3)
end
let!(:variant_in_the_order) { create(:variant, count_on_hand: 4) }
let!(:variant_not_in_the_order) { create(:variant, count_on_hand: 2) }
let(:variant_stock_levels) { VariantsStockLevels.new }
before do
order.reload
end
it "returns a hash with variant id, quantity, max_quantity and stock on hand" do
expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq(
variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 4 }
)
end
it "includes all line items, even when the variant_id is not specified" do
expect(variant_stock_levels.call(order, [])).to eq(
variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 4 }
)
end
it "includes an empty quantity entry for variants that aren't in the order" do
variant_ids = [variant_in_the_order.id, variant_not_in_the_order.id]
expect(variant_stock_levels.call(order, variant_ids)).to eq(
variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 4 },
variant_not_in_the_order.id => { quantity: 0, max_quantity: 0, on_hand: 2 }
)
end
describe "encoding Infinity" do
let!(:variant_in_the_order) { create(:variant, on_demand: true, count_on_hand: 0) }
it "encodes Infinity as a large, finite integer" do
expect(variant_stock_levels.call(order, [variant_in_the_order.id])).to eq(
variant_in_the_order.id => { quantity: 2, max_quantity: 3, on_hand: 2147483647 }
)
end
end
end