From 7bd32d496781e1678250e286f6baee8be92714b6 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 6 Jun 2019 18:09:25 +0100 Subject: [PATCH 1/5] Fix usage of variant.on_hand in subscriptions order factory, we now take on_demand into account This fixes a problem introduced in https://github.com/openfoodfoundation/openfoodnetwork/commit/12eab1bfa9b8e6ece85c1e5dd6e3691beda1d687#diff-c3c4192f302cc77e9a8547012fe86ddb, since then variant.on_hand does not return infinity if variant is on_demand --- app/services/order_factory.rb | 8 ++++---- spec/services/order_factory_spec.rb | 14 ++++++++++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/services/order_factory.rb b/app/services/order_factory.rb index fc4bd5f424..19c579e15b 100644 --- a/app/services/order_factory.rb +++ b/app/services/order_factory.rb @@ -48,7 +48,7 @@ class OrderFactory 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.on_hand, li[:quantity]) + li[:quantity] = stock_limited_quantity(variant.on_demand, variant.on_hand, li[:quantity]) li[:price] = variant.price build_item_from(li) end @@ -81,9 +81,9 @@ class OrderFactory @order.payments.create(payment_method_id: attrs[:payment_method_id], amount: @order.reload.total) end - def stock_limited_quantity(stock, requested) - return requested if opts[:skip_stock_check] - [stock, requested].min + def stock_limited_quantity(variant_on_demand, variant_on_hand, requested) + return requested if opts[:skip_stock_check] || variant_on_demand + [variant_on_hand, requested].min end def scoper diff --git a/spec/services/order_factory_spec.rb b/spec/services/order_factory_spec.rb index 7c9c0bd7d5..0c250b786e 100644 --- a/spec/services/order_factory_spec.rb +++ b/spec/services/order_factory_spec.rb @@ -31,7 +31,7 @@ describe OrderFactory do attrs end - it "builds a new order based the provided attributes" do + it "builds a new order based on the provided attributes" do expect{ order }.to change{ Spree::Order.count }.by(1) expect(order).to be_a Spree::Order expect(order.line_items.count).to eq 2 @@ -78,11 +78,21 @@ describe OrderFactory do end context "when skip_stock_check is not requested" do - it "initialised the order but limits stock to the available amount" do + it "initialises the order but limits stock to the available amount" do expect{ order }.to change{ Spree::Order.count }.by(1) expect(order).to be_a Spree::Order expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 2 end + + context "when variant is on_demand" do + before { variant1.update_attribute(:on_demand, true) } + + it "initialises the order with the requested quantity regardless of stock" do + expect{ order }.to change{ Spree::Order.count }.by(1) + expect(order).to be_a Spree::Order + expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 5 + end + end end context "when skip_stock_check is requested" do From 567196fe0e11508dd096c58dcd26c5c15a6fcb81 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Jun 2019 12:06:05 +0100 Subject: [PATCH 2/5] Fix line item verification of stock on the browser side by adding logic to handle completed orders with some reserved stock The shopping/orders_spec is now validating this edge case by using all stock available in one of the line items --- .../darkswarm/directives/on_hand.js.coffee | 8 +++++--- app/views/spree/orders/_line_item.html.haml | 3 ++- spec/features/consumer/shopping/orders_spec.rb | 11 +++++++---- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee b/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee index 9a849029b4..0173196138 100644 --- a/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee +++ b/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee @@ -14,9 +14,11 @@ Darkswarm.directive "ofnOnHand", -> ngModel.$parsers.push (viewValue) -> on_hand = parseInt(attr.ofnOnHand) - if parseInt(viewValue) > on_hand - alert t("js.insufficient_stock", {on_hand: on_hand}) - viewValue = on_hand + finalized_quantity = parseInt(attr.finalizedquantity) + available_quantity = on_hand + finalized_quantity + if parseInt(viewValue) > available_quantity + alert t("js.insufficient_stock", {on_hand: available_quantity}) + viewValue = available_quantity ngModel.$setViewValue viewValue ngModel.$render() diff --git a/app/views/spree/orders/_line_item.html.haml b/app/views/spree/orders/_line_item.html.haml index 14ef285565..cfd4d4a500 100644 --- a/app/views/spree/orders/_line_item.html.haml +++ b/app/views/spree/orders/_line_item.html.haml @@ -22,7 +22,8 @@ %td.text-right.cart-item-price{"data-hook" => "cart_item_price"} = line_item.single_display_amount_with_adjustments.to_html %td.text-center.cart-item-quantity{"data-hook" => "cart_item_quantity"} - = item_form.number_field :quantity, :min => 0, "ofn-on-hand" => "#{variant.on_demand && 9999 || variant.on_hand}", "ng-model" => "line_item_#{line_item.id}", :class => "line_item_quantity", :size => 5 + - finalized_quantity = @order.completed? ? line_item.quantity : 0 + = item_form.number_field :quantity, :min => 0, "ofn-on-hand" => "#{variant.on_demand && 9999 || variant.on_hand}", "finalizedquantity" => finalized_quantity, "ng-model" => "line_item_#{line_item.id}", :class => "line_item_quantity", :size => 5 %td.cart-item-total.text-right{"data-hook" => "cart_item_total"} = line_item.display_amount_with_adjustments.to_html unless line_item.quantity.nil? diff --git a/spec/features/consumer/shopping/orders_spec.rb b/spec/features/consumer/shopping/orders_spec.rb index 50ad7d7563..b9caaa7e75 100644 --- a/spec/features/consumer/shopping/orders_spec.rb +++ b/spec/features/consumer/shopping/orders_spec.rb @@ -146,7 +146,10 @@ feature "Order Management", js: true do within "tr.variant-#{item1.variant.id}" do expect(page).to have_content item1.product.name expect(page).to have_field 'order_line_items_attributes_0_quantity' - fill_in 'order_line_items_attributes_0_quantity', with: 2 + # The original item quantity is 1, there are 4 more items available in stock + # By changing quantity to 5 we validate the case where the original stock in the order + # must be taken into account to fullfil the order (no insufficient stock error) + fill_in 'order_line_items_attributes_0_quantity', with: 5 end expect(page).to have_button I18n.t(:save_changes) @@ -158,15 +161,15 @@ feature "Order Management", js: true do click_button I18n.t(:save_changes) - expect(find(".order-total.grand-total")).to have_content "85.00" - expect(item1.reload.quantity).to eq 2 + expect(find(".order-total.grand-total")).to have_content "115.00" + expect(item1.reload.quantity).to eq 5 # Deleting an item within "tr.variant-#{item2.variant.id}" do click_link "delete_line_item_#{item2.id}" end - expect(find(".order-total.grand-total")).to have_content "75.00" + expect(find(".order-total.grand-total")).to have_content "105.00" expect(Spree::LineItem.find_by_id(item2.id)).to be nil # Cancelling the order From 05a15d94415082b3fb8bbdbcfe58dbaa4ddf9156 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 10 Jun 2019 20:39:29 +0100 Subject: [PATCH 3/5] Extract method to remove some copy pasted code in order_factory_spec --- spec/services/order_factory_spec.rb | 30 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/spec/services/order_factory_spec.rb b/spec/services/order_factory_spec.rb index 0c250b786e..c6ae2f0565 100644 --- a/spec/services/order_factory_spec.rb +++ b/spec/services/order_factory_spec.rb @@ -32,8 +32,7 @@ describe OrderFactory do end it "builds a new order based on the provided attributes" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.line_items.count).to eq 2 expect(order.customer).to eq customer expect(order.user).to eq user @@ -64,8 +63,7 @@ describe OrderFactory do before { customer.update_attribute(:user_id, nil) } it "initialises the order without a user_id" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.user).to be nil end end @@ -79,8 +77,7 @@ describe OrderFactory do context "when skip_stock_check is not requested" do it "initialises the order but limits stock to the available amount" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 2 end @@ -88,8 +85,7 @@ describe OrderFactory do before { variant1.update_attribute(:on_demand, true) } it "initialises the order with the requested quantity regardless of stock" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 5 end end @@ -99,8 +95,7 @@ describe OrderFactory do let(:opts) { { skip_stock_check: true } } it "initialises the order with the requested quantity regardless" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 5 end end @@ -112,8 +107,7 @@ describe OrderFactory do context "when skip_stock_check is not requested" do it "initialised the order but limits stock to the available amount" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 3 end end @@ -122,8 +116,7 @@ describe OrderFactory do let(:opts) { { skip_stock_check: true } } it "initialises the order with the requested quantity regardless" do - expect{ order }.to change{ Spree::Order.count }.by(1) - expect(order).to be_a Spree::Order + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 6 end end @@ -133,7 +126,7 @@ describe OrderFactory do describe "determining the price for line items" do context "when no override is present" do it "uses the price from the variant" do - expect{ order }.to change{ Spree::Order.count }.by(1) + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).price).to eq 5.0 expect(order.total).to eq 38.0 end @@ -143,11 +136,16 @@ describe OrderFactory do let!(:override) { create(:variant_override, hub_id: shop.id, variant_id: variant1.id, price: 3.0) } it "uses the price from the override" do - expect{ order }.to change{ Spree::Order.count }.by(1) + expect_new_order expect(order.line_items.find_by_variant_id(variant1.id).price).to eq 3.0 expect(order.total).to eq 34.0 end end end + + def expect_new_order + expect{ order }.to change{ Spree::Order.count }.by(1) + expect(order).to be_a Spree::Order + end end end From 2b6e6c62ddabb85ba4ab18d440af4dc6fd9c19b6 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 10 Jun 2019 20:44:35 +0100 Subject: [PATCH 4/5] Simplify order_factory_spec by extracting copy pasted code to method --- spec/services/order_factory_spec.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/spec/services/order_factory_spec.rb b/spec/services/order_factory_spec.rb index c6ae2f0565..85e27881a5 100644 --- a/spec/services/order_factory_spec.rb +++ b/spec/services/order_factory_spec.rb @@ -78,7 +78,7 @@ describe OrderFactory do context "when skip_stock_check is not requested" do it "initialises the order but limits stock to the available amount" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 2 + expect(variant1_line_item.quantity).to eq 2 end context "when variant is on_demand" do @@ -86,7 +86,7 @@ describe OrderFactory do it "initialises the order with the requested quantity regardless of stock" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 5 + expect(variant1_line_item.quantity).to eq 5 end end end @@ -96,7 +96,7 @@ describe OrderFactory do it "initialises the order with the requested quantity regardless" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 5 + expect(variant1_line_item.quantity).to eq 5 end end end @@ -108,7 +108,7 @@ describe OrderFactory do context "when skip_stock_check is not requested" do it "initialised the order but limits stock to the available amount" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 3 + expect(variant1_line_item.quantity).to eq 3 end end @@ -117,7 +117,7 @@ describe OrderFactory do it "initialises the order with the requested quantity regardless" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).quantity).to eq 6 + expect(variant1_line_item.quantity).to eq 6 end end end @@ -127,7 +127,7 @@ describe OrderFactory do context "when no override is present" do it "uses the price from the variant" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).price).to eq 5.0 + expect(variant1_line_item.price).to eq 5.0 expect(order.total).to eq 38.0 end end @@ -137,7 +137,7 @@ describe OrderFactory do it "uses the price from the override" do expect_new_order - expect(order.line_items.find_by_variant_id(variant1.id).price).to eq 3.0 + expect(variant1_line_item.price).to eq 3.0 expect(order.total).to eq 34.0 end end @@ -147,5 +147,9 @@ describe OrderFactory do expect{ order }.to change{ Spree::Order.count }.by(1) expect(order).to be_a Spree::Order end + + def variant1_line_item + order.line_items.find_by_variant_id(variant1.id) + end end end From 7e2bead54de64c42ce92cfcd2897ac20cfdbede6 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 10 Jun 2019 21:00:20 +0100 Subject: [PATCH 5/5] Make finalizedquantity optional in the ofn-on-hand directive and extract avaiable quantity to a separate method for clarity --- .../javascripts/darkswarm/directives/on_hand.js.coffee | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee b/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee index 0173196138..1086363676 100644 --- a/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee +++ b/app/assets/javascripts/darkswarm/directives/on_hand.js.coffee @@ -13,9 +13,7 @@ Darkswarm.directive "ofnOnHand", -> ngModel.$setDirty = setDirty ngModel.$parsers.push (viewValue) -> - on_hand = parseInt(attr.ofnOnHand) - finalized_quantity = parseInt(attr.finalizedquantity) - available_quantity = on_hand + finalized_quantity + available_quantity = scope.available_quantity() if parseInt(viewValue) > available_quantity alert t("js.insufficient_stock", {on_hand: available_quantity}) viewValue = available_quantity @@ -23,3 +21,8 @@ Darkswarm.directive "ofnOnHand", -> ngModel.$render() viewValue + + scope.available_quantity = -> + on_hand = parseInt(attr.ofnOnHand) + finalized_quantity = parseInt(attr.finalizedquantity) || 0 # finalizedquantity is optional + on_hand + finalized_quantity