Merge pull request #3626 from softr8/admin/improve-stock-items-manage…

…ment

Improving stock items management

backend/app/controllers/spree/admin/stock_items_controller.rb

@@ -40,24 +40,33 @@ def load_product
       end
 
       def load_stock_management_data
-        @stock_locations = Spree::StockLocation.accessible_by(current_ability)
-        @stock_item_stock_locations = params[:stock_location_id].present? ? @stock_locations.where(id: params[:stock_location_id]) : @stock_locations
+        @stock_locations = Spree::StockLocation.accessible_by(current_ability, :read)
+        @stock_item_stock_locations = Spree::DeprecatedInstanceVariableProxy.new(
+          view_context,
+          :@stock_locations,
+          :stock_item_stock_locations,
+          Spree::Deprecation,
+          "Please, do not use @stock_item_stock_locations anymore in the views, use @stock_locations",
+        )
         @variant_display_attributes = self.class.variant_display_attributes
-        @variants = Spree::Config.variant_search_class.new(params[:variant_search_term], scope: variant_scope).results
-        @variants = @variants.includes(:images, stock_items: :stock_location, product: :variant_images)
-        @variants = @variants.includes(option_values: :option_type)
-        @variants = @variants.order(id: :desc).page(params[:page]).per(params[:per_page] || Spree::Config[:orders_per_page])
+        @variants = Spree::Config.variant_search_class.new(params[:variant_search_term], scope: variant_scope).results.
+            order(id: :desc).page(params[:page]).per(params[:per_page] || Spree::Config[:orders_per_page])
       end
 
       def variant_scope
-        scope = Spree::Variant.accessible_by(current_ability)
-        if @product
-          scope = scope.where(
-            product: @product,
-            is_master: !@product.has_variants?
+        scope = Spree::Variant
+          .accessible_by(current_ability)
+          .distinct.order(:sku)
+          .includes(
+            :images,
+            stock_items: :stock_location,
+            product: :variant_images,
+            option_values: :option_type
           )
-        end
-        scope = scope.order(:sku)
+
+        scope = scope.where(product: @product, is_master: !@product.has_variants?) if @product
+        scope = scope.by_stock_location(params[:stock_location_id]) if params[:stock_location_id].present?
+
         scope
       end
 

backend/app/views/spree/admin/shared/_variant_search.html.erb

@@ -4,7 +4,14 @@
       <div class="field-block col-3">
         <div class="field">
           <%= label_tag nil, Spree::StockLocation.model_name.human %>
-          <%= select_tag :stock_location_id, options_from_collection_for_select(stock_locations, :id, :name, params[:stock_location_id]), { include_blank: t('spree.all'), class: 'custom-select fullwidth', "data-placeholder" => t('spree.select_a_stock_location') } %>
+          <%= select_tag(
+            :stock_location_id,
+            options_from_collection_for_select(stock_locations, :id, :name, params[:stock_location_id]),
+            include_blank: t('spree.all'),
+            class: 'select2 fullwidth',
+            "data-placeholder" => t('spree.select_a_stock_location'),
+            multiple: true,
+          ) %>
         </div>
       </div>
       <div class="<%= if content_for?(:sidebar) then 'col-6' else 'col-9' end %>">

backend/app/views/spree/admin/stock_items/_stock_management.html.erb

@@ -1,6 +1,6 @@
 <% admin_layout "full-width" %>
 
-<%= paginate @variants, theme: "solidus_admin" %>
+<%= paginate variants, theme: "solidus_admin" %>
 
 <table class="index stock-table" id="listing_product_stock">
   <colgroup>
@@ -71,18 +71,30 @@
               <col style="width: 15%">
             </colgroup>
             <% variant.stock_items.each do |item| %>
-              <% if @stock_item_stock_locations.include?(item.stock_location) %>
-                <tr class="js-edit-stock-item stock-item-edit-row" data-variant-id="<%= variant.id %>" data-stock-item="<%= item.to_json %>" data-stock-location-name="<%= item.stock_location.name %>" data-track-inventory="<%= variant.should_track_inventory? %>" data-can-edit="<%= can?(:edit, Spree::StockItem) %>" data-variant-sku="<%= variant.sku %>">
-                  <%# This is rendered in JS %>
-                </tr>
-              <% end %>
+              <tr
+                class="js-edit-stock-item stock-item-edit-row"
+                data-variant-id="<%= variant.id %>"
+                data-stock-item="<%= item.to_json %>"
+                data-stock-location-name="<%= item.stock_location.name %>"
+                data-track-inventory="<%= variant.should_track_inventory? %>"
+                data-can-edit="<%= can?(:admin, Spree::StockItem) %>"
+                data-variant-sku="<%= variant.sku %>"
+              >
+                <%# This is rendered in JS %>
+              </tr>
             <% end %>
-            <% locations_without_items = @stock_item_stock_locations - variant.stock_items.flat_map(&:stock_location) %>
+            <% locations_without_items = stock_locations - variant.stock_items.flat_map(&:stock_location) %>
             <% if locations_without_items.any? && can?(:create, Spree::StockItem) %>
               <tr class="js-add-stock-item stock-item-edit-row" data-variant-id="<%= variant.id %>">
                 <form>
                   <td class='location-name-cell'>
-                    <%= select_tag :stock_location_id, options_from_collection_for_select(locations_without_items, :id, :name), class: 'custom-select', prompt: t('spree.add_to_stock_location'), id: "variant-stock-location-#{variant.id}" %>
+                    <%= select_tag(
+                      :stock_location_id,
+                      options_from_collection_for_select(locations_without_items, :id, :name),
+                      class: 'custom-select',
+                      prompt: t('spree.add_to_stock_location'),
+                      id: "variant-stock-location-#{variant.id}",
+                    ) %>
                   </td>
                   <td class="align-center">
                     <%= check_box_tag :backorderable, 'backorderable', false, id: "variant-backorderable-#{variant.id}" %>
@@ -104,4 +116,4 @@
   <% end %>
 </table>
 
-<%= paginate @variants, theme: "solidus_admin" %>
+<%= paginate variants, theme: "solidus_admin" %>

backend/app/views/spree/admin/stock_items/index.html.erb

@@ -19,7 +19,10 @@
 <% end %>
 
 <% if @variants.any? %>
-  <%= render partial: 'stock_management', locals: { variants: @variants } %>
+  <%= render partial: 'stock_management', locals: {
+    variants: @variants,
+    stock_locations: @stock_locations,
+  } %>
 <% else %>
   <div class="fullwidth no-objects-found">
     <%= t('spree.no_variants_found_try_again') %>

backend/spec/controllers/spree/admin/stock_items_controller_spec.rb

@@ -71,6 +71,20 @@ module Admin
             )
           end
         end
+
+        context "with specific stock location" do
+          let(:stock_location) { variant_1.stock_locations.first }
+
+          before do
+            variant_2.stock_items.delete_all
+          end
+
+          it "filters variants by stock locations" do
+            get :index, params: { stock_location_id: stock_location.id }
+            expect(assigns(:variants)).to include variant_1
+            expect(assigns(:variants)).not_to include variant_2
+          end
+        end
       end
     end
   end

backend/spec/features/admin/products/stock_management_spec.rb

@@ -73,17 +73,6 @@
       end
     end
 
-    context "when the admin user can't edit stock items" do
-      custom_authorization! do |_user|
-        cannot :edit, Spree::StockItem
-      end
-
-      it "doesn't allow editing", js: true do
-        click_link "Product Stock"
-        expect(first("input[type='number']")).to be_disabled
-      end
-    end
-
     def adjust_count_on_hand(variant_id, count_on_hand)
       within("tr#spree_variant_#{variant_id}") do
         find(:css, "input[type='number']").set(count_on_hand)

backend/spec/features/admin/stock_items_spec.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Stock Items Management', js: true do
+  stub_authorization!
+
+  let(:admin_user)   { create(:admin_user) }
+  let!(:variant_1) { create(:variant) }
+  let!(:variant_2) { create(:variant) }
+  let!(:stock_location) { create(:stock_location_without_variant_propagation) }
+
+  scenario 'User can add a new stock locations to any variant' do
+    visit spree.admin_stock_items_path
+    within('.js-add-stock-item', match: :first) do
+      find('[name="stock_location_id"]').select(stock_location.name)
+      fill_in('count_on_hand', with: 10)
+      click_on('Create')
+    end
+    expect(page).to have_content("Created successfully")
+  end
+end

core/app/models/spree/variant/scopes.rb

@@ -10,6 +10,10 @@ def self.prepended(base)
             order(Arel.sql("COALESCE((SELECT COUNT(*) FROM  #{Spree::LineItem.quoted_table_name} GROUP BY #{Spree::LineItem.quoted_table_name}.variant_id HAVING #{Spree::LineItem.quoted_table_name}.variant_id = #{Spree::Variant.quoted_table_name}.id), 0) DESC"))
           }
 
+          scope :by_stock_location, ->(stock_location_id) {
+            joins(:stock_locations).where(spree_stock_locations: { id: stock_location_id })
+          }
+
           class << self
             # Returns variants that match a given option value
             #

core/lib/spree/deprecation.rb

@@ -33,9 +33,9 @@ module Spree
   #
   # Default deprecator is <tt>Spree::Deprecation</tt>.
   class DeprecatedInstanceVariableProxy < ActiveSupport::Deprecation::DeprecationProxy
-    def initialize(instance, method, var = "@#{method}", deprecator = Spree::Deprecation, message = nil)
+    def initialize(instance, method_or_var, var = "@#{method}", deprecator = Spree::Deprecation, message = nil)
       @instance = instance
-      @method = method
+      @method_or_var = method_or_var
       @var = var
       @deprecator = deprecator
       @message = message
@@ -44,12 +44,14 @@ def initialize(instance, method, var = "@#{method}", deprecator = Spree::Depreca
     private
 
     def target
-      @instance.__send__(@method)
+      return @instance.instance_variable_get(@method_or_var) if @instance.instance_variable_defined?(@method_or_var)
+
+      @instance.__send__(@method_or_var)
     end
 
     def warn(callstack, called, args)
-      message = @message || "#{@var} is deprecated! Call #{@method}.#{called} instead of #{@var}.#{called}."
-      message = [message, "Args: #{args.inspect}"].join(" ")
+      message = @message || "#{@var} is deprecated! Call #{@method_or_var}.#{called} instead of #{@var}.#{called}."
+      message = [message, "Args: #{args.inspect}"].join(" ") unless args.empty?
 
       @deprecator.warn(message, callstack)
     end

core/spec/models/spree/variant/scopes_spec.rb

@@ -7,7 +7,7 @@
   let!(:variant_1) { create(:variant, product: product) }
   let!(:variant_2) { create(:variant, product: product) }
 
-  context ".with_prices" do
+  describe ".with_prices" do
     context "when searching for the default pricing options" do
       it "finds all variants" do
         expect(Spree::Variant.with_prices).to contain_exactly(product.master, variant_1, variant_2)
@@ -44,15 +44,29 @@
     end
   end
 
-  it ".descend_by_popularity" do
+  specify ".descend_by_popularity" do
     # Requires a product with at least two variants, where one has a higher number of
     # orders than the other
     Spree::LineItem.delete_all # FIXME leaky database - too many line_items
     create(:line_item, variant: variant_1)
     expect(Spree::Variant.descend_by_popularity.first).to eq(variant_1)
   end
 
-  context "finding by option values" do
+  describe ".by_stock_location" do
+    let!(:stock_location_1) { create(:stock_location) }
+    let!(:stock_location_2) { create(:stock_location) }
+
+    it "finds variants by stock location" do
+      variants = Spree::Variant.where(id: [variant_1.id, variant_2.id]) # exclude the master variant
+      variant_1.stock_items.where.not(stock_location_id: stock_location_1.id).delete_all
+      variant_2.stock_items.where.not(stock_location_id: stock_location_2.id).delete_all
+
+      expect(variants.by_stock_location(stock_location_1.id)).to contain_exactly(variant_1)
+      expect(variants.by_stock_location(stock_location_2.id)).to contain_exactly(variant_2)
+    end
+  end
+
+  describe ".has_option" do
     let!(:option_type) { create(:option_type, name: "bar") }
     let!(:option_value_1) do
       option_value = create(:option_value, name: "foo", option_type: option_type)
@@ -68,26 +82,30 @@
 
     let!(:product_variants) { product.variants_including_master }
 
-    it "by objects" do
+    it "finds by option value objects" do
       variants = product_variants.has_option(option_type, option_value_1)
+
       expect(variants).to include(variant_1)
       expect(variants).not_to include(variant_2)
     end
 
-    it "by names" do
+    it "finds by option value names" do
       variants = product_variants.has_option("bar", "foo")
+
       expect(variants).to include(variant_1)
       expect(variants).not_to include(variant_2)
     end
 
-    it "by ids" do
+    it "finds by option value ids" do
       variants = product_variants.has_option(option_type.id, option_value_1.id)
+
       expect(variants).to include(variant_1)
       expect(variants).not_to include(variant_2)
     end
 
-    it "by mixed conditions" do
+    it "finds by option value with mixed conditions" do
       variants = product_variants.has_option(option_type.id, "foo", option_value_2)
+
       expect(variants).to be_empty
     end
   end