Merge pull request #5804 from luisramos0/oc_inv_bug_313

Apply PR #5775 to branch v3.1.2 to make v3.1.3 patch

.rubocop_todo.yml

@@ -363,6 +363,13 @@ Rails/UniqueValidationWithoutIndex:
     - 'app/models/customer.rb'
     - 'app/models/exchange.rb'
 
+# Offense count: 2
+# Configuration parameters: Include.
+# Include: app/models/**/*.rb
+Rails/UniqueValidationWithoutIndex:
+  Exclude:
+    - 'app/models/spree/stock_item.rb'
+
 # Offense count: 1
 # Configuration parameters: Environments.
 # Environments: development, test, production

app/assets/stylesheets/darkswarm/home_tagline.scss

@@ -17,10 +17,12 @@
     position: fixed;
     left: 0;
     right: 0;
-    bottom: 0;
+    top: 0;
     z-index: -1;
     width: 100%;
     height: 100%;
+    // Use vh units for new browsers - fixed issue 1253
+    height: 100vh;
   }
 
   h1 {

app/models/spree/product_decorator.rb

@@ -63,8 +63,7 @@ Spree::Product.class_eval do
   scope :visible_for, lambda { |enterprise|
     joins('LEFT OUTER JOIN spree_variants AS o_spree_variants ON (o_spree_variants.product_id = spree_products.id)').
       joins('LEFT OUTER JOIN inventory_items AS o_inventory_items ON (o_spree_variants.id = o_inventory_items.variant_id)').
-      where('o_inventory_items.enterprise_id = (?) AND visible = (?)', enterprise, true).
-      select('DISTINCT spree_products.*')
+      where('o_inventory_items.enterprise_id = (?) AND visible = (?)', enterprise, true)
   }
 
   # -- Scopes

app/models/spree/stock_item.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+module Spree
+  class StockItem < ActiveRecord::Base
+    acts_as_paranoid
+
+    belongs_to :stock_location, class_name: 'Spree::StockLocation'
+    belongs_to :variant, class_name: 'Spree::Variant'
+    has_many :stock_movements, dependent: :destroy
+
+    validates :stock_location, :variant, presence: true
+    validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }
+    validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? }
+
+    delegate :weight, to: :variant
+    delegate :name, to: :variant, prefix: true
+
+    def backordered_inventory_units
+      Spree::InventoryUnit.backordered_for_stock_item(self)
+    end
+
+    def adjust_count_on_hand(value)
+      with_lock do
+        self.count_on_hand = count_on_hand + value
+        process_backorders if in_stock?
+
+        save!
+      end
+    end
+
+    def in_stock?
+      count_on_hand.positive?
+    end
+
+    # Tells whether it's available to be included in a shipment
+    def available?
+      in_stock? || backorderable?
+    end
+
+    def variant
+      Spree::Variant.unscoped { super }
+    end
+
+    private
+
+    def count_on_hand=(value)
+      self[:count_on_hand] = value
+    end
+
+    def process_backorders
+      backordered_inventory_units.each do |unit|
+        break unless in_stock?
+
+        unit.fill_backorder
+      end
+    end
+  end
+end

app/serializers/api/admin/for_order_cycle/enterprise_serializer.rb

@@ -30,9 +30,11 @@ class Api::Admin::ForOrderCycle::EnterpriseSerializer < ActiveModel::Serializer
   def products_scope
     products_relation = object.supplied_products
     if order_cycle.prefers_product_selection_from_coordinator_inventory_only?
-      products_relation = products_relation.visible_for(order_cycle.coordinator)
+      products_relation = products_relation.
+        visible_for(order_cycle.coordinator).
+        select('DISTINCT spree_products.*')
     end
-    products_relation.order(:name)
+    products_relation
   end
 
   def products

app/services/permitted_attributes/order_cycle.rb

@@ -11,6 +11,7 @@ module PermittedAttributes
 
       @params.require(:order_cycle).permit(
         :name, :orders_open_at, :orders_close_at, :coordinator_id,
+        :preferred_product_selection_from_coordinator_inventory_only,
         incoming_exchanges: permitted_exchange_attributes,
         outgoing_exchanges: permitted_exchange_attributes,
         schedule_ids: [], coordinator_fee_ids: []

config/locales/en_GB.yml

@@ -182,6 +182,7 @@ en_GB:
         explainer: Automatic processing of these orders failed for an unknown reason. This should not occur, please contact us if you are seeing this.
   home: "OFN"
   title: "Open Food Network"
+  welcome_to: "Welcome to"
   site_meta_description: "The Open Food Network software platform allows farmers to sell produce online, at a price that works for them. It has been built specifically for selling food so it can handle tricky measures or stock levels that only food has - a dozen eggs, a bunch of parsley, a whole chicken that varies in weight…"
   search_by_name: Search by name, town, county or postcode...
   producers_join: UK producers are now welcome to join Open Food Network UK.
@@ -1713,6 +1714,7 @@ en_GB:
   remember_me: Remember Me
   are_you_sure: "Are you sure?"
   orders_open: "Orders open"
+  closing: "Closing"
   going_back_to_home_page: "Taking you back to the home page"
   creating: Creating
   updating: Updating

db/migrate/20200512070717_add_lock_version_to_stock_items.rb

@@ -0,0 +1,5 @@
+class AddLockVersionToStockItems < ActiveRecord::Migration
+  def change
+    add_column :spree_stock_items, :lock_version, :integer, default: 0
+  end
+end

db/migrate/20200514174526_reset_negative_nonbackorderable_count_on_hand_in_stock_items.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class ResetNegativeNonbackorderableCountOnHandInStockItems < ActiveRecord::Migration
+  module Spree
+    class StockItem < ActiveRecord::Base
+      self.table_name = "spree_stock_items"
+    end
+  end
+
+  def up
+    Spree::StockItem.where(backorderable: false)
+      .where("count_on_hand < 0")
+      .update_all(count_on_hand: 0)
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/schema.rb

@@ -901,6 +901,7 @@ ActiveRecord::Schema.define(version: 20200702112157) do
     t.datetime "updated_at",                        null: false
     t.boolean  "backorderable",     default: false
     t.datetime "deleted_at"
+    t.integer  "lock_version",      default: 0
   end
 
   add_index "spree_stock_items", ["stock_location_id", "variant_id"], name: "stock_item_by_loc_and_var_id", using: :btree

spec/controllers/admin/order_cycles_controller_spec.rb

@@ -178,10 +178,22 @@ module Admin
 
           it "returns an error message" do
             spree_put :update, params
+
             json_response = JSON.parse(response.body)
             expect(json_response['errors']).to be
           end
         end
+
+        it "can update preference product_selection_from_coordinator_inventory_only" do
+          expect(OrderCycleForm).to receive(:new).
+            with(order_cycle,
+                 { "preferred_product_selection_from_coordinator_inventory_only" => true },
+                 anything) { form_mock }
+          allow(form_mock).to receive(:save) { true }
+
+          spree_put :update, params.
+            merge(order_cycle: { preferred_product_selection_from_coordinator_inventory_only: true })
+        end
       end
     end
 

spec/controllers/checkout_controller_concurrency_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 # This is the first example of testing concurrency in the Open Food Network.
@@ -15,6 +17,20 @@ describe CheckoutController, concurrency: true, type: :controller do
   let(:payment_method) { create(:payment_method, distributors: [distributor]) }
   let(:breakpoint) { Mutex.new }
 
+  let(:address_params) { address.attributes.except("id") }
+  let(:order_params) {
+    {
+      "payments_attributes" => [
+        {
+          "payment_method_id" => payment_method.id,
+          "amount" => order.total
+        }
+      ],
+      "bill_address_attributes" => address_params,
+      "ship_address_attributes" => address_params,
+    }
+  }
+
   before do
     # Create a valid order ready for checkout:
     create(:shipping_method, distributors: [distributor])
@@ -26,7 +42,9 @@ describe CheckoutController, concurrency: true, type: :controller do
     allow(controller).to receive(:spree_current_user).and_return(order.user)
     allow(controller).to receive(:current_distributor).and_return(order.distributor)
     allow(controller).to receive(:current_order_cycle).and_return(order.order_cycle)
+  end
 
+  it "handles two concurrent orders successfully" do
     # New threads start running straight away. The breakpoint is after loading
     # the order and before advancing the order's state and making payments.
     breakpoint.lock
@@ -36,21 +54,6 @@ describe CheckoutController, concurrency: true, type: :controller do
       # I did not find out how to call the original code otherwise.
       ActiveSupport::Notifications.instrument("spree.checkout.update")
     end
-  end
-
-  it "waits for concurrent checkouts" do
-    # Basic data the user submits during checkout:
-    address_params = address.attributes.except("id")
-    order_params = {
-      "payments_attributes" => [
-        {
-          "payment_method_id" => payment_method.id,
-          "amount" => order.total
-        }
-      ],
-      "bill_address_attributes" => address_params,
-      "ship_address_attributes" => address_params,
-    }
 
     # Starting two checkout threads. The controller code will determine if
     # these two threads are synchronised correctly or run into a race condition.

spec/models/spree/line_item_spec.rb

@@ -97,7 +97,7 @@ module Spree
       end
 
       it "caps at zero when stock is negative" do
-        v.update! on_hand: -2
+        v.__send__(:stock_item).update_column(:count_on_hand, -2)
         li.cap_quantity_at_stock!
         expect(li.reload.quantity).to eq 0
       end
@@ -123,7 +123,7 @@ module Spree
           before { vo.update(count_on_hand: -3) }
 
           it "caps at zero" do
-            v.update(on_hand: -2)
+            v.__send__(:stock_item).update_column(:count_on_hand, -2)
             li.cap_quantity_at_stock!
             expect(li.reload.quantity).to eq 0
           end

spec/models/spree/stock_item_spec.rb

@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Spree::StockItem do
+  let(:stock_location) { create(:stock_location_with_items) }
+
+  subject { stock_location.stock_items.order(:id).first }
+
+  describe "validation" do
+    let(:stock_item) { stock_location.stock_items.first }
+
+    it "requires count_on_hand to be positive if not backorderable" do
+      stock_item.backorderable = false
+
+      stock_item.__send__(:count_on_hand=, 1)
+      expect(stock_item.valid?).to eq(true)
+
+      stock_item.__send__(:count_on_hand=, 0)
+      expect(stock_item.valid?).to eq(true)
+
+      stock_item.__send__(:count_on_hand=, -1)
+      expect(stock_item.valid?).to eq(false)
+    end
+
+    it "allows count_on_hand to be negative if backorderable" do
+      stock_item.backorderable = true
+
+      stock_item.__send__(:count_on_hand=, 1)
+      expect(stock_item.valid?).to eq(true)
+
+      stock_item.__send__(:count_on_hand=, -1)
+      expect(stock_item.valid?).to eq(true)
+    end
+  end
+
+  it 'maintains the count on hand for a variant' do
+    expect(subject.count_on_hand).to eq 15
+  end
+
+  it "can return the stock item's variant's name" do
+    expect(subject.variant_name).to eq(subject.variant.name)
+  end
+
+  context "available to be included in shipment" do
+    context "has stock" do
+      it { expect(subject).to be_available }
+    end
+
+    context "backorderable" do
+      before { subject.backorderable = true }
+      it { expect(subject).to be_available }
+    end
+
+    context "no stock and not backorderable" do
+      before do
+        subject.backorderable = false
+        allow(subject).to receive_messages(count_on_hand: 0)
+      end
+
+      it { expect(subject).not_to be_available }
+    end
+  end
+
+  context "adjust count_on_hand" do
+    let!(:current_on_hand) { subject.count_on_hand }
+
+    it 'is updated pessimistically' do
+      copy = Spree::StockItem.find(subject.id)
+
+      subject.adjust_count_on_hand(5)
+      expect(subject.count_on_hand).to eq(current_on_hand + 5)
+
+      expect(copy.count_on_hand).to eq(current_on_hand)
+      copy.adjust_count_on_hand(5)
+      expect(copy.count_on_hand).to eq(current_on_hand + 10)
+    end
+
+    context "item out of stock (by two items)" do
+      let(:inventory_unit) { double('InventoryUnit') }
+      let(:inventory_unit_2) { double('InventoryUnit2') }
+
+      before do
+        allow(subject).to receive(:backorderable?).and_return(true)
+        subject.adjust_count_on_hand(- (current_on_hand + 2))
+      end
+
+      it "doesn't process backorders" do
+        expect(subject).not_to receive(:backordered_inventory_units)
+        subject.adjust_count_on_hand(1)
+      end
+
+      context "adds new items" do
+        before { allow(subject).to receive_messages(backordered_inventory_units: [inventory_unit, inventory_unit_2]) }
+
+        it "fills existing backorders" do
+          expect(inventory_unit).to receive(:fill_backorder)
+          expect(inventory_unit_2).to receive(:fill_backorder)
+
+          subject.adjust_count_on_hand(3)
+          expect(subject.count_on_hand).to eq(1)
+        end
+      end
+    end
+  end
+end

spec/services/exchange_products_renderer_spec.rb

@@ -7,8 +7,9 @@ describe ExchangeProductsRenderer do
 
   describe "#exchange_products" do
     describe "for an incoming exchange" do
+      let(:exchange) { order_cycle.exchanges.incoming.first }
+
       it "loads products" do
-        exchange = order_cycle.exchanges.incoming.first
         products = renderer.exchange_products(true, exchange.sender)
 
         expect(products.first.supplier.name).to eq exchange.variants.first.product.supplier.name
@@ -16,14 +17,34 @@ describe ExchangeProductsRenderer do
     end
 
     describe "for an outgoing exchange" do
+      let(:exchange) { order_cycle.exchanges.outgoing.first }
+
       it "loads products" do
-        exchange = order_cycle.exchanges.outgoing.first
         products = renderer.exchange_products(false, exchange.receiver)
 
         suppliers = [exchange.variants[0].product.supplier.name, exchange.variants[1].product.supplier.name]
         expect(suppliers).to include products.first.supplier.name
         expect(suppliers).to include products.second.supplier.name
       end
+
+      context "showing products from coordinator inventory only" do
+        before { order_cycle.update prefers_product_selection_from_coordinator_inventory_only: true }
+
+        it "loads no products if there are no products from the coordinator inventory" do
+          products = renderer.exchange_products(false, exchange.receiver)
+
+          expect(products).to be_empty
+        end
+
+        it "loads products from the coordinator inventory" do
+          # Add variant already in the exchange to the coordinator's inventory
+          exchange.variants.first.inventory_items = [create(:inventory_item, enterprise: order_cycle.coordinator)]
+
+          products = renderer.exchange_products(false, exchange.receiver)
+
+          expect(products).to eq [exchange.variants.first.product]
+        end
+      end
     end
   end