Merge pull request #3734 from luisramos0/2-0-validate-distribution

Removing a variant from OC will prevent user with that variant in the cart from checking out

.rubocop_manual_todo.yml

@@ -154,7 +154,7 @@ Metrics/LineLength:
   - lib/open_food_network/available_payment_method_filter.rb
   - lib/open_food_network/bulk_coop_report.rb
   - lib/open_food_network/customers_report.rb
-  - lib/open_food_network/distribution_change_validator.rb
+  - app/services/order_cycle_distributed_variants.rb
   - lib/open_food_network/enterprise_fee_applicator.rb
   - lib/open_food_network/enterprise_fee_calculator.rb
   - lib/open_food_network/enterprise_issue_validator.rb
@@ -290,7 +290,7 @@ Metrics/LineLength:
   - spec/lib/open_food_network/bulk_coop_report_spec.rb
   - spec/lib/open_food_network/cached_products_renderer_spec.rb
   - spec/lib/open_food_network/customers_report_spec.rb
-  - spec/lib/open_food_network/distribution_change_validator_spec.rb
+  - spec/services/order_cycle_distributed_variants.rb
   - spec/lib/open_food_network/enterprise_fee_applicator_spec.rb
   - spec/lib/open_food_network/enterprise_fee_calculator_spec.rb
   - spec/lib/open_food_network/enterprise_injection_data_spec.rb

.rubocop_todo.yml

@@ -311,7 +311,7 @@ Layout/EmptyLinesAroundClassBody:
     - 'app/serializers/api/currency_config_serializer.rb'
     - 'app/serializers/api/product_serializer.rb'
     - 'lib/discourse/single_sign_on.rb'
-    - 'lib/open_food_network/distribution_change_validator.rb'
+    - 'app/services/order_cycle_distributed_variants.rb'
     - 'lib/open_food_network/lettuce_share_report.rb'
     - 'lib/open_food_network/order_and_distributor_report.rb'
     - 'lib/open_food_network/rack_request_blocker.rb'
@@ -1901,7 +1901,7 @@ Style/MethodDefParentheses:
   Exclude:
     - 'app/helpers/enterprises_helper.rb'
     - 'app/models/spree/product_decorator.rb'
-    - 'lib/open_food_network/distribution_change_validator.rb'
+    - 'app/services/order_cycle_distributed_variants.rb'
     - 'lib/open_food_network/feature_toggle.rb'
     - 'lib/open_food_network/group_buy_report.rb'
     - 'spec/support/request/authentication_workflow.rb'

app/controllers/checkout_controller.rb

@@ -169,7 +169,7 @@ class CheckoutController < Spree::CheckoutController
   def load_order
     @order = current_order
     redirect_to main_app.shop_path and return unless @order and @order.checkout_allowed?
-    raise_insufficient_quantity and return if @order.insufficient_stock_lines.present?
+    redirect_to_cart_path and return unless valid_order_line_items?
     redirect_to main_app.shop_path and return if @order.completed?
     before_address
     setup_for_current_state
@@ -184,8 +184,11 @@ class CheckoutController < Spree::CheckoutController
     @order.ship_address = finder.ship_address
   end
 
-  # Overriding Spree's methods
-  def raise_insufficient_quantity
+  def valid_order_line_items?
+    @order.insufficient_stock_lines.empty? && OrderCycleDistributedVariants.new(@order.order_cycle, @order.distributor).distributes_order_variants?(@order)
+  end
+
+  def redirect_to_cart_path
     respond_to do |format|
       format.html do
         redirect_to cart_path

app/controllers/spree/orders_controller_decorator.rb

@@ -21,14 +21,15 @@ Spree::OrdersController.class_eval do
   def edit
     @order = current_order(true)
     @insufficient_stock_lines = @order.insufficient_stock_lines
+    @unavailable_order_variants = OrderCycleDistributedVariants.new(current_order_cycle, current_distributor).unavailable_order_variants(@order)
 
     if @order.line_items.empty?
       redirect_to main_app.shop_path
     else
       associate_user
 
-      if @order.insufficient_stock_lines.present?
-        flash[:error] = t("spree.inventory_error_flash_for_insufficient_quantity")
+      if @order.insufficient_stock_lines.present? || @unavailable_order_variants.present?
+        flash[:error] = t("spree.orders.error_flash_for_unavailable_items")
       end
     end
   end

app/jobs/subscription_placement_job.rb

@@ -67,7 +67,7 @@ class SubscriptionPlacementJob
   end
 
   def available_variants_for(order)
-    DistributionChangeValidator.new(order).variants_available_for_distribution(order.distributor, order.order_cycle)
+    OrderCycleDistributedVariants.new(order.order_cycle, order.distributor).available_variants
   end
 
   def send_placement_email(order, changes)

app/models/spree/order_decorator.rb

@@ -1,5 +1,4 @@
 require 'open_food_network/enterprise_fee_calculator'
-require 'open_food_network/distribution_change_validator'
 require 'open_food_network/feature_toggle'
 require 'open_food_network/tag_rule_applicator'
 require 'concerns/order_shipment'
@@ -81,7 +80,7 @@ Spree::Order.class_eval do
   # -- Methods
   def products_available_from_new_distribution
     # Check that the line_items in the current order are available from a newly selected distribution
-    errors.add(:base, I18n.t(:spree_order_availability_error)) unless DistributionChangeValidator.new(self).can_change_to_distribution?(distributor, order_cycle)
+    errors.add(:base, I18n.t(:spree_order_availability_error)) unless OrderCycleDistributedVariants.new(order_cycle, distributor).distributes_order_variants?(self)
   end
 
   def using_guest_checkout?

app/services/cart_service.rb

@@ -137,7 +137,7 @@ class CartService
   end
 
   def check_variant_available_under_distribution(variant)
-    return true if DistributionChangeValidator.new(@order).variants_available_for_distribution(@distributor, @order_cycle).include? variant
+    return true if OrderCycleDistributedVariants.new(@order_cycle, @distributor).available_variants.include? variant
 
     errors.add(:base, I18n.t(:spree_order_populator_availability_error))
     false

app/services/order_cycle_distributed_variants.rb

@@ -0,0 +1,19 @@
+class OrderCycleDistributedVariants
+  def initialize(order_cycle, distributor)
+    @order_cycle = order_cycle
+    @distributor = distributor    
+  end
+
+  def distributes_order_variants?(order)
+    unavailable_order_variants(order).empty?
+  end
+
+  def unavailable_order_variants(order)
+    order.line_item_variants - available_variants
+  end
+
+  def available_variants
+    return [] unless @order_cycle
+    @order_cycle.variants_distributed_by(@distributor)
+  end
+end

app/views/spree/orders/_line_item.html.haml

@@ -14,6 +14,11 @@
         = variant.in_stock? ? t(".insufficient_stock", :on_hand => variant.on_hand) : t(".out_of_stock")
         %br/
 
+    - if @unavailable_order_variants.andand.include? line_item.variant
+      %span.out-of-stock
+        = t(".unavailable_item")
+        %br/
+
   %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"}

config/locales/en.yml

@@ -3065,6 +3065,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using
       format: ! '%Y-%m-%d'
       js_format: 'yy-mm-dd'
     orders:
+      error_flash_for_unavailable_items: "An item in your cart has become unavailable."
       edit:
         login_to_view_order: "Please log in to view your order."
       bought:
@@ -3072,6 +3073,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using
       line_item:
         insufficient_stock: "Insufficient stock available, only %{on_hand} remaining"
         out_of_stock: "Out of Stock"
+        unavailable_item: "Currently unavailable"
     shipment_states:
       backorder: backorder
       partial: partial

knapsack_rspec_report.json

@@ -164,7 +164,7 @@
   "spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb": 0.7021048069000244,
   "spec/features/admin/tax_settings_spec.rb": 0.4577906131744385,
   "spec/models/spree/product_property_spec.rb": 0.4479696750640869,
-  "spec/lib/open_food_network/distribution_change_validator_spec.rb": 0.2241048812866211,
+  "spec/services/order_cycle_distributed_variants_spec.rb": 0.2241048812866211,
   "spec/models/spree/option_value_spec.rb": 0.5916051864624023,
   "spec/controllers/spree/api/line_items_controller_spec.rb": 0.5350861549377441,
   "spec/helpers/order_cycles_helper_spec.rb": 0.40488767623901367,

lib/open_food_network/distribution_change_validator.rb

@@ -1,15 +0,0 @@
-class DistributionChangeValidator
-
-  def initialize order
-    @order = order
-  end
-
-  def can_change_to_distribution?(distributor, order_cycle)
-    (@order.line_item_variants - variants_available_for_distribution(distributor, order_cycle)).empty?
-  end
-
-  def variants_available_for_distribution(distributor, order_cycle)
-    return [] unless order_cycle
-    order_cycle.variants_distributed_by(distributor)
-  end
-end

spec/controllers/checkout_controller_spec.rb

@@ -36,22 +36,39 @@ describe CheckoutController, type: :controller do
     flash[:info].should == "The hub you have selected is temporarily closed for orders. Please try again later."
   end
 
-  it "redirects to the cart when some items are out of stock" do
-    controller.stub(:current_distributor).and_return(distributor)
-    controller.stub(:current_order_cycle).and_return(order_cycle)
-    controller.stub(:current_order).and_return(order)
-    order.stub_chain(:insufficient_stock_lines, :present?).and_return true
-    get :edit
-    response.should redirect_to spree.cart_path
-  end
+  describe "redirection to the cart" do
+    let(:order_cycle_distributed_variants) { double(:order_cycle_distributed_variants) }
 
-  it "renders when both distributor and order cycle is selected" do
-    controller.stub(:current_distributor).and_return(distributor)
-    controller.stub(:current_order_cycle).and_return(order_cycle)
-    controller.stub(:current_order).and_return(order)
-    order.stub_chain(:insufficient_stock_lines, :present?).and_return false
-    get :edit
-    response.should be_success
+    before do
+      allow(controller).to receive(:current_order).and_return(order)
+      allow(order).to receive(:distributor).and_return(distributor)
+      order.order_cycle = order_cycle
+
+      allow(OrderCycleDistributedVariants).to receive(:new).with(order_cycle, distributor).and_return(order_cycle_distributed_variants)      
+    end
+
+    it "redirects when some items are out of stock" do
+      allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return false
+
+      get :edit
+      expect(response).to redirect_to spree.cart_path
+    end
+
+    it "redirects when some items are not available" do
+      allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true
+      expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(false)
+
+      get :edit
+      expect(response).to redirect_to spree.cart_path
+    end
+
+    it "does not redirect when items are available and in stock" do
+      allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true
+      expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(true)
+
+      get :edit
+      expect(response).to be_success
+    end
   end
 
   describe "building the order" do

spec/controllers/spree/orders_controller_spec.rb

@@ -89,6 +89,9 @@ describe Spree::OrdersController, type: :controller do
       allow(controller).to receive(:current_order).and_return order
       allow(order).to receive_message_chain(:line_items, :empty?).and_return true
       allow(order).to receive(:insufficient_stock_lines).and_return []
+      allow(order).to receive(:line_item_variants).and_return []
+      allow(order_cycle).to receive(:variants_distributed_by).and_return []
+
       session[:access_token] = order.token
       spree_get :edit
       expect(response).to redirect_to shop_path
@@ -161,6 +164,18 @@ describe Spree::OrdersController, type: :controller do
           expect(flash[:error]).to eq("An item in your cart has become unavailable.")
         end
       end
+
+      describe "when an item is unavailable" do
+        before do
+          order.order_cycle = create(:simple_order_cycle, distributors: [d], variants: [])
+        end
+
+        it "displays a flash message when we view the cart" do
+          spree_get :edit
+          expect(response.status).to eq 200
+          expect(flash[:error]).to eq("An item in your cart has become unavailable.")
+        end
+      end
     end
   end
 

spec/lib/open_food_network/distribution_change_validator_spec.rb

@@ -1,42 +0,0 @@
-require 'open_food_network/distribution_change_validator'
-
-describe DistributionChangeValidator do
-  let(:order) { double(:order) }
-  let(:subject) { DistributionChangeValidator.new(order) }
-  let(:product) { double(:product) }
-
-  describe "checking if an order can change to a specified new distribution" do
-    let(:distributor) { double(:distributor) }
-    let(:order_cycle) { double(:order_cycle) }
-
-    it "returns false when a variant is not available for the specified distribution" do
-      order.should_receive(:line_item_variants) { [1] }
-      subject.should_receive(:variants_available_for_distribution).
-        with(distributor, order_cycle) { [] }
-      expect(subject.can_change_to_distribution?(distributor, order_cycle)).to be false
-    end
-
-    it "returns true when all variants are available for the specified distribution" do
-      order.should_receive(:line_item_variants) { [1] }
-      subject.should_receive(:variants_available_for_distribution).
-        with(distributor, order_cycle) { [1] }
-      expect(subject.can_change_to_distribution?(distributor, order_cycle)).to be true
-    end
-  end
-
-  describe "finding variants that are available through a particular order cycle" do
-    it "finds variants distributed by order cycle" do
-      variant = double(:variant)
-      distributor = double(:distributor)
-      order_cycle = double(:order_cycle)
-
-      order_cycle.should_receive(:variants_distributed_by).with(distributor) { [variant] }
-
-      expect(subject.variants_available_for_distribution(distributor, order_cycle)).to eq [variant]
-    end
-
-    it "returns an empty array when order cycle is nil" do
-      expect(subject.variants_available_for_distribution(nil, nil)).to eq []
-    end
-  end
-end

spec/requests/checkout/failed_checkout_spec.rb

@@ -23,6 +23,10 @@ describe "checking out an order that initially fails", type: :request do
   end
 
   before do
+    order_cycle_distributed_variants = double(:order_cycle_distributed_variants)
+    allow(OrderCycleDistributedVariants).to receive(:new).and_return(order_cycle_distributed_variants)
+    allow(order_cycle_distributed_variants).to receive(:distributes_order_variants?).and_return(true)
+
     order.reload.update_totals
     set_order order
   end

spec/requests/checkout/stripe_connect_spec.rb

@@ -27,6 +27,10 @@ describe "checking out an order with a Stripe Connect payment method", type: :re
   end
 
   before do
+    order_cycle_distributed_variants = double(:order_cycle_distributed_variants)
+    allow(OrderCycleDistributedVariants).to receive(:new).and_return(order_cycle_distributed_variants)
+    allow(order_cycle_distributed_variants).to receive(:distributes_order_variants?).and_return(true)
+
     allow(Stripe).to receive(:api_key) { "sk_test_12345" }
     order.update_attributes(distributor_id: enterprise.id, order_cycle_id: order_cycle.id)
     order.reload.update_totals

spec/services/cart_service_spec.rb

@@ -227,21 +227,23 @@ describe CartService do
     describe "checking variant is available under the distributor" do
       let(:product) { double(:product) }
       let(:variant) { double(:variant, product: product) }
+      let(:order_cycle_distributed_variants) { double(:order_cycle_distributed_variants) }
 
-      it "delegates to DistributionChangeValidator, returning true when available" do
-        dcv = double(:dcv)
-        expect(dcv).to receive(:variants_available_for_distribution).with(123, 234).and_return([variant])
-        expect(DistributionChangeValidator).to receive(:new).with(order).and_return(dcv)
+      before do
+        expect(OrderCycleDistributedVariants).to receive(:new).with(234, 123).and_return(order_cycle_distributed_variants)
         cart_service.instance_eval { @distributor = 123; @order_cycle = 234 }
+      end
+
+      it "delegates to OrderCycleDistributedVariants, returning true when available" do
+        expect(order_cycle_distributed_variants).to receive(:available_variants).and_return([variant])
+
         expect(cart_service.send(:check_variant_available_under_distribution, variant)).to be true
         expect(cart_service.errors).to be_empty
       end
 
-      it "delegates to DistributionChangeValidator, returning false and erroring otherwise" do
-        dcv = double(:dcv)
-        expect(dcv).to receive(:variants_available_for_distribution).with(123, 234).and_return([])
-        expect(DistributionChangeValidator).to receive(:new).with(order).and_return(dcv)
-        cart_service.instance_eval { @distributor = 123; @order_cycle = 234 }
+      it "delegates to OrderCycleDistributedVariants, returning false and erroring otherwise" do
+        expect(order_cycle_distributed_variants).to receive(:available_variants).and_return([])        
+
         expect(cart_service.send(:check_variant_available_under_distribution, variant)).to be false
         expect(cart_service.errors.to_a).to eq(["That product is not available from the chosen distributor or order cycle."])
       end

spec/services/order_cycle_distributed_variants_spec.rb

@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe OrderCycleDistributedVariants do
+  let(:order) { double(:order) }
+  let(:distributor) { double(:distributor) }
+  let(:order_cycle) { double(:order_cycle) }  
+  let(:subject) { OrderCycleDistributedVariants.new(order_cycle, distributor) }
+  let(:product) { double(:product) }
+
+  describe "checking if an order can change to a specified new distribution" do
+    it "returns false when a variant is not available for the specified distribution" do
+      allow(order).to receive(:line_item_variants).and_return([1])
+      allow(subject).to receive(:available_variants).and_return([])
+      expect(subject.distributes_order_variants?(order)).to be false
+    end
+
+    it "returns true when all variants are available for the specified distribution" do
+      allow(order).to receive(:line_item_variants).and_return([1])
+      allow(subject).to receive(:available_variants).and_return([1])
+      expect(subject.distributes_order_variants?(order)).to be true
+    end
+  end
+
+  describe "finding variants that are available through a particular order cycle" do
+    it "finds variants distributed by order cycle" do
+      variant = double(:variant)
+      allow(order_cycle).to receive(:variants_distributed_by).with(distributor).and_return([variant])
+
+      expect(subject.available_variants).to eq [variant]
+    end
+
+    it "returns an empty array when order cycle is nil" do
+      subject = OrderCycleDistributedVariants.new(nil, nil)
+      expect(subject.available_variants).to eq []
+    end
+  end
+end