From c0a095ffafc1948e2892ae2cdb1ac2cbd41ec7a0 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Tue, 11 Dec 2018 18:12:58 +0000 Subject: [PATCH 1/5] Improve variant_overrides_spec variable names --- .../shopping/variant_overrides_spec.rb | 94 +++++++++---------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/spec/features/consumer/shopping/variant_overrides_spec.rb b/spec/features/consumer/shopping/variant_overrides_spec.rb index ff101fd5a08..0e2404252a0 100644 --- a/spec/features/consumer/shopping/variant_overrides_spec.rb +++ b/spec/features/consumer/shopping/variant_overrides_spec.rb @@ -13,26 +13,26 @@ let(:outgoing_exchange) { oc.exchanges.outgoing.first } let(:sm) { hub.shipping_methods.first } let(:pm) { hub.payment_methods.first } - let(:p1) { create(:simple_product, supplier: producer) } - let(:p2) { create(:simple_product, supplier: producer) } - let(:p3) { create(:simple_product, supplier: producer, on_demand: true) } - let(:v1) { create(:variant, product: p1, price: 11.11, unit_value: 1) } - let(:v2) { create(:variant, product: p1, price: 22.22, unit_value: 2) } - let(:v3) { create(:variant, product: p2, price: 33.33, unit_value: 3) } - let(:v4) { create(:variant, product: p1, price: 44.44, unit_value: 4) } - let(:v5) { create(:variant, product: p3, price: 55.55, unit_value: 5, on_demand: true) } - let(:v6) { create(:variant, product: p3, price: 66.66, unit_value: 6, on_demand: true) } - let!(:vo1) { create(:variant_override, hub: hub, variant: v1, price: 55.55, count_on_hand: nil, default_stock: nil, resettable: false) } - let!(:vo2) { create(:variant_override, hub: hub, variant: v2, count_on_hand: 0, default_stock: nil, resettable: false) } - let!(:vo3) { create(:variant_override, hub: hub, variant: v3, count_on_hand: 0, default_stock: nil, resettable: false) } - let!(:vo4) { create(:variant_override, hub: hub, variant: v4, count_on_hand: 3, default_stock: nil, resettable: false) } - let!(:vo5) { create(:variant_override, hub: hub, variant: v5, count_on_hand: 0, default_stock: nil, resettable: false) } - let!(:vo6) { create(:variant_override, hub: hub, variant: v6, count_on_hand: 6, default_stock: nil, resettable: false) } - let(:ef) { create(:enterprise_fee, enterprise: hub, fee_type: 'packing', calculator: Calculator::FlatPercentPerItem.new(preferred_flat_percent: 10)) } + let(:product1) { create(:simple_product, supplier: producer) } + let(:product2) { create(:simple_product, supplier: producer) } + let(:product3) { create(:simple_product, supplier: producer, on_demand: true) } + let(:product1_variant1) { create(:variant, product: product1, price: 11.11, unit_value: 1) } + let(:product1_variant2) { create(:variant, product: product1, price: 22.22, unit_value: 2) } + let(:product2_variant1) { create(:variant, product: product2, price: 33.33, unit_value: 3) } + let(:product1_variant3) { create(:variant, product: product1, price: 44.44, unit_value: 4) } + let(:product3_variant1) { create(:variant, product: product3, price: 55.55, unit_value: 5, on_demand: true) } + let(:product3_variant2) { create(:variant, product: product3, price: 66.66, unit_value: 6, on_demand: true) } + let!(:product1_variant1_override) { create(:variant_override, hub: hub, variant: product1_variant1, price: 55.55, count_on_hand: nil, default_stock: nil, resettable: false) } + let!(:product1_variant2_override) { create(:variant_override, hub: hub, variant: product1_variant2, count_on_hand: 0, default_stock: nil, resettable: false) } + let!(:product2_variant1_override) { create(:variant_override, hub: hub, variant: product2_variant1, count_on_hand: 0, default_stock: nil, resettable: false) } + let!(:product1_variant3_override) { create(:variant_override, hub: hub, variant: product1_variant3, count_on_hand: 3, default_stock: nil, resettable: false) } + let!(:product3_variant1_override) { create(:variant_override, hub: hub, variant: product3_variant1, count_on_hand: 0, default_stock: nil, resettable: false) } + let!(:product3_variant2_override) { create(:variant_override, hub: hub, variant: product3_variant2, count_on_hand: 6, default_stock: nil, resettable: false) } + let(:enterprise_fee) { create(:enterprise_fee, enterprise: hub, fee_type: 'packing', calculator: Calculator::FlatPercentPerItem.new(preferred_flat_percent: 10)) } before do - outgoing_exchange.variants = [v1, v2, v3, v4, v5, v6] - outgoing_exchange.enterprise_fees << ef + outgoing_exchange.variants = [product1_variant1, product1_variant2, product2_variant1, product1_variant3, product3_variant1, product3_variant2] + outgoing_exchange.enterprise_fees << enterprise_fee sm.calculator.preferred_amount = 0 visit shops_path click_link hub.name @@ -40,24 +40,24 @@ describe "viewing products" do it "shows the overridden price" do - page.should_not have_price with_currency(12.22) # $11.11 + 10% fee - page.should have_price with_currency(61.11) + page.should_not have_price with_currency(12.22) # product1_variant1.price ($11.11) + 10% fee + page.should have_price with_currency(61.11) # product1_variant1_override.price ($55.55) + 10% fee end it "looks up stock from the override" do # Product should appear but one of the variants is out of stock - page.should_not have_content v2.options_text + page.should_not have_content product1_variant2.options_text # Entire product should not appear - no stock - page.should_not have_content p2.name - page.should_not have_content v3.options_text + page.should_not have_content product2.name + page.should_not have_content product2_variant1.options_text # On-demand product with VO of no stock should NOT appear - page.should_not have_content v5.options_text + page.should_not have_content product3_variant1.options_text end it "calculates fees correctly" do - page.find("#variant-#{v1.id} .graph-button").click + page.find("#variant-#{product1_variant1.id} .graph-button").click page.find(".price_breakdown a").click page.should have_selector 'li.cost div', text: with_currency(55.55) page.should have_selector 'li.packing-fee div', text: with_currency(5.56) @@ -65,38 +65,38 @@ end it "shows the correct prices when products are in the cart" do - fill_in "variants[#{v1.id}]", with: "2" + fill_in "variants[#{product1_variant1.id}]", with: "2" show_cart wait_until_enabled 'li.cart a.button' visit shop_path - page.should_not have_price with_currency(12.22) + page.should have_price with_currency(61.11) end # The two specs below reveal an unrelated issue with fee calculation. See: # https://github.com/openfoodfoundation/openfoodnetwork/issues/312 it "shows the overridden price with fees in the quick cart" do - fill_in "variants[#{v1.id}]", with: "2" + fill_in "variants[#{product1_variant1.id}]", with: "2" show_cart - page.should have_selector "#cart-variant-#{v1.id} .quantity", text: '2' - page.should have_selector "#cart-variant-#{v1.id} .price", text: with_currency(61.11) - page.should have_selector "#cart-variant-#{v1.id} .total-price", text: with_currency(122.22) + page.should have_selector "#cart-variant-#{product1_variant1.id} .quantity", text: '2' + page.should have_selector "#cart-variant-#{product1_variant1.id} .price", text: with_currency(61.11) + page.should have_selector "#cart-variant-#{product1_variant1.id} .total-price", text: with_currency(122.22) end it "shows the correct prices in the shopping cart" do - fill_in "variants[#{v1.id}]", with: "2" + fill_in "variants[#{product1_variant1.id}]", with: "2" add_to_cart - page.should have_selector "tr.line-item.variant-#{v1.id} .cart-item-price", text: with_currency(61.11) + page.should have_selector "tr.line-item.variant-#{product1_variant1.id} .cart-item-price", text: with_currency(61.11) page.should have_field "order[line_items_attributes][0][quantity]", with: '2' - page.should have_selector "tr.line-item.variant-#{v1.id} .cart-item-total", text: with_currency(122.22) + page.should have_selector "tr.line-item.variant-#{product1_variant1.id} .cart-item-total", text: with_currency(122.22) page.should have_selector "#edit-cart .item-total", text: with_currency(122.22) page.should have_selector "#edit-cart .grand-total", text: with_currency(122.22) end it "shows the correct prices in the checkout" do - fill_in "variants[#{v1.id}]", with: "2" + fill_in "variants[#{product1_variant1.id}]", with: "2" click_checkout page.should have_selector 'form.edit_order .cart-total', text: with_currency(122.22) @@ -108,7 +108,7 @@ describe "creating orders" do it "creates the order with the correct prices" do - fill_in "variants[#{v1.id}]", with: "2" + fill_in "variants[#{product1_variant1.id}]", with: "2" click_checkout complete_checkout @@ -119,39 +119,39 @@ end it "subtracts stock from the override" do - fill_in "variants[#{v4.id}]", with: "2" + fill_in "variants[#{product1_variant3.id}]", with: "2" click_checkout expect do expect do complete_checkout - end.to change { v4.reload.count_on_hand }.by(0) - end.to change { vo4.reload.count_on_hand }.by(-2) + end.to change { product1_variant3.reload.count_on_hand }.by(0) + end.to change { product1_variant3_override.reload.count_on_hand }.by(-2) end it "subtracts stock from stock-overridden on_demand variants" do - fill_in "variants[#{v6.id}]", with: "2" + fill_in "variants[#{product3_variant2.id}]", with: "2" click_checkout expect do expect do complete_checkout - end.to change { v6.reload.count_on_hand }.by(0) - end.to change { vo6.reload.count_on_hand }.by(-2) + end.to change { product3_variant2.reload.count_on_hand }.by(0) + end.to change { product3_variant2_override.reload.count_on_hand }.by(-2) end it "does not subtract stock from overrides that do not override count_on_hand" do - fill_in "variants[#{v1.id}]", with: "2" + fill_in "variants[#{product1_variant1.id}]", with: "2" click_checkout expect do complete_checkout - end.to change { v1.reload.count_on_hand }.by(-2) - vo1.reload.count_on_hand.should be_nil + end.to change { product1_variant1.reload.count_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 - v4.update_attribute :count_on_hand, 0 - fill_in "variants[#{v4.id}]", with: "2" + product1_variant3.update_attribute :count_on_hand, 0 + fill_in "variants[#{product1_variant3.id}]", with: "2" click_checkout complete_checkout From d64170505329c32a30e9bfbb11111fade80e42fe Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 20 Dec 2018 20:29:03 +0000 Subject: [PATCH 2/5] Add variant scoping to availability_validator_decorator by using line_item.scoper and moving Spree::Stock::Quantifier.can_supply? to VariantStock so that it becomes overridable --- app/models/concerns/variant_stock.rb | 11 +++++++++ app/models/spree/line_item_decorator.rb | 8 +++---- .../stock/availability_validator_decorator.rb | 24 ++++++++++++------- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index f5b34d3d109..8621d23a7d6 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -114,6 +114,17 @@ def on_demand=(new_value) end end + # Moving Spree::Stock::Quantifier.can_supply? to the variant enables us to override this behaviour for variant overrides + # We can have this responsibility here in the variant because there is only one stock item per variant + # + # 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 can_supply?(quantity) + return true unless Spree::Config[:track_inventory_levels] + + on_demand || total_on_hand >= quantity + end + # We can have this responsibility here in the variant because there is only one stock item per variant # # This enables us to override this behaviour for variant overrides diff --git a/app/models/spree/line_item_decorator.rb b/app/models/spree/line_item_decorator.rb index 58ca4c2b390..dae53204cef 100644 --- a/app/models/spree/line_item_decorator.rb +++ b/app/models/spree/line_item_decorator.rb @@ -125,6 +125,10 @@ def sufficient_stock? end end + def scoper + @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(order.distributor) + end + private def update_inventory_with_scoping @@ -133,10 +137,6 @@ def update_inventory_with_scoping end alias_method_chain :update_inventory, :scoping - def scoper - @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(order.distributor) - end - def calculate_final_weight_volume if final_weight_volume.present? && quantity_was > 0 self.final_weight_volume = final_weight_volume * quantity / quantity_was diff --git a/app/models/spree/stock/availability_validator_decorator.rb b/app/models/spree/stock/availability_validator_decorator.rb index 29f9d0413c1..f687d6cd120 100644 --- a/app/models/spree/stock/availability_validator_decorator.rb +++ b/app/models/spree/stock/availability_validator_decorator.rb @@ -24,18 +24,26 @@ def quantity_in_shipment(line_item) def line_item_shipment(line_item) return line_item.target_shipment if line_item.target_shipment - return line_item.order.shipments.first if line_item.order.present? && line_item.order.shipments.any? + return line_item.order.shipments.first if line_item.order.andand.shipments.any? end - # This is the spree v2.0.4 implementation of validate - # But using the calculated quantity instead of the line_item.quantity. + # Overrides Spree v2.0.4 validate method version to: + # - scope variants to hub and thus acivate variant overrides + # - use calculated quantity instead of the line_item.quantity + # - rely on Variant.can_supply? instead of Stock::Quantified.can_supply? + # so that it works correctly for variant overrides def validate_quantity(line_item, quantity) - quantifier = Spree::Stock::Quantifier.new(line_item.variant_id) - return if quantifier.can_supply? quantity + line_item.scoper.scope(line_item.variant) + add_out_of_stock_error(line_item) unless line_item.variant.can_supply? quantity + end + + def add_out_of_stock_error(line_item) variant = line_item.variant - display_name = %Q{#{variant.name}} - display_name += %Q{ (#{variant.options_text})} unless variant.options_text.blank? - line_item.errors[:quantity] << Spree.t(:out_of_stock, :scope => :order_populator, :item => display_name.inspect) + display_name = %{#{variant.name}} + display_name += %{(#{variant.options_text})} if variant.options_text.present? + line_item.errors[:quantity] << Spree.t(:out_of_stock, + scope: :order_populator, + item: display_name.inspect) end end From 1835d97013f83576d2c4a93b80bdb08f7015bf34 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 20 Dec 2018 20:31:10 +0000 Subject: [PATCH 3/5] Adapt line_item.sufficient_stock? to spree 2 --- app/models/spree/line_item_decorator.rb | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/app/models/spree/line_item_decorator.rb b/app/models/spree/line_item_decorator.rb index dae53204cef..7758ed1d310 100644 --- a/app/models/spree/line_item_decorator.rb +++ b/app/models/spree/line_item_decorator.rb @@ -111,18 +111,15 @@ def unit_value final_weight_volume / quantity end - # MONKEYPATCH of Spree method - # Enables scoping of variant to hub/shop, so we check stock against relevant overrides if they exist - # Also skips stock check if requested quantity is zero + # Overrides Spree version to: + # - skip stock check if skip_stock_check flag is active + # - skip stock check if requested quantity is zero or negative + # - scope variants to hub and thus acivate variant overrides def sufficient_stock? - return true if quantity == 0 # This line added - scoper.scope(variant) # This line added - return true if variant.on_demand - if new_record? || !order.completed? - variant.on_hand >= quantity - else - variant.on_hand >= (quantity - self.changed_attributes['quantity'].to_i) - end + return true if skip_stock_check + return true if quantity <= 0 + scoper.scope(variant) + variant.can_supply?(quantity) end def scoper From 17ced61b3d2757aeba72d1bfaf6fe5a07af1dc98 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 20 Dec 2018 20:33:02 +0000 Subject: [PATCH 4/5] =?UTF-8?q?Override=20Spree::Variant.in=5Fstock=3F=20i?= =?UTF-8?q?n=20VariantStock=20so=20that=20we=20don=E2=80=99t=20depend=20on?= =?UTF-8?q?=20Spree::Stock::Quantifier?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/concerns/variant_stock.rb | 5 +++++ lib/open_food_network/scope_variant_to_hub.rb | 8 -------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index 8621d23a7d6..df8f52f16e7 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -125,6 +125,11 @@ def can_supply?(quantity) on_demand || total_on_hand >= quantity end + # We override Spree::Variant.in_stock? to avoid depending on the non-overidable method Spree::Stock::Quantifier.can_supply? + def in_stock?(quantity = 1) + can_supply?(quantity) + end + # We can have this responsibility here in the variant because there is only one stock item per variant # # This enables us to override this behaviour for variant overrides diff --git a/lib/open_food_network/scope_variant_to_hub.rb b/lib/open_food_network/scope_variant_to_hub.rb index 8b361610680..de99d6f7f74 100644 --- a/lib/open_food_network/scope_variant_to_hub.rb +++ b/lib/open_food_network/scope_variant_to_hub.rb @@ -20,14 +20,6 @@ def price_in(currency) Spree::Price.new(amount: price, currency: currency) end - # Old Spree has the same logic as here and doesn't need this override. - # But we need this to use VariantOverrides with Spree 2.0. - def in_stock? - return true unless Spree::Config[:track_inventory_levels] - - on_demand || (count_on_hand > 0) - end - # Uses variant_override.count_on_hand instead of Stock::Quantifier.stock_items.count_on_hand def total_on_hand @variant_override.andand.count_on_hand || super From 6c70998b64b90a37906126aa56de04e02fed496f Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 20 Dec 2018 20:35:25 +0000 Subject: [PATCH 5/5] Move StockLocation.fill_status to VariantStock so we can easily override it --- app/models/concerns/variant_stock.rb | 17 +++++++++++++++++ app/models/spree/stock_location_decorator.rb | 4 ++++ 2 files changed, 21 insertions(+) diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index df8f52f16e7..0a7ba06d283 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -130,6 +130,23 @@ def in_stock?(quantity = 1) can_supply?(quantity) end + # Moving Spree::StockLocation.fill_status to the variant enables us to override this behaviour for variant overrides + # We can have this responsibility here in the variant because there is only one stock item per variant + # + # 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 + on_hand = quantity + backordered = 0 + else + on_hand = [0, total_on_hand].max + backordered = on_demand ? (quantity - on_hand) : 0 + end + + [on_hand, backordered] + end + # We can have this responsibility here in the variant because there is only one stock item per variant # # This enables us to override this behaviour for variant overrides diff --git a/app/models/spree/stock_location_decorator.rb b/app/models/spree/stock_location_decorator.rb index 335c5576745..87702616c75 100644 --- a/app/models/spree/stock_location_decorator.rb +++ b/app/models/spree/stock_location_decorator.rb @@ -2,4 +2,8 @@ def move(variant, quantity, originator = nil) variant.move(quantity, originator) end + + def fill_status(variant, quantity) + variant.fill_status(quantity) + end end