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

3222 Improve stock levels calculation code on cart populate and add specs to test handling of variant overrides #3230

Merged
merged 5 commits into from Jan 16, 2019
Copy path 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'
@@ -20,28 +20,16 @@ def populate

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 @@ def populate_variant_attributes_from_product(order)
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
Copy path View file
@@ -80,11 +80,19 @@ def read_variants(data)
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)
@@ -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
@@ -3,13 +3,34 @@
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 @@
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

Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.