Merge pull request #13533 from chahmedejaz/task/13435-sort-products-by-on-hand-amount

Sort product list by 'on hand' amount

app/controllers/admin/products_v3_controller.rb

@@ -123,6 +123,13 @@ module Admin
       @page = params[:page].presence || 1
       @per_page = params[:per_page].presence || 15
       @q = params.permit(q: {})[:q] || { s: 'name asc' }
+
+      # Transform on_hand sorting to include backorderable_priority (on-demand) for proper ordering
+      if @q[:s] == 'on_hand asc'
+        @q[:s] = ['backorderable_priority asc', @q[:s]]
+      elsif @q[:s] == 'on_hand desc'
+        @q[:s] = ['backorderable_priority desc', @q[:s]]
+      end
     end
 
     def producers
@@ -155,8 +162,27 @@ module Admin
       product_query = OpenFoodNetwork::Permissions.new(spree_current_user)
         .editable_products.merge(product_scope_with_includes).ransack(ransack_query).result
 
-      @pagy, @products = pagy(product_query.order(:name), limit: @per_page, page: @page,
-                                                          size: [1, 2, 2, 1])
+      # Postgres requires ORDER BY expressions to appear in the SELECT list when using DISTINCT.
+      # When the current ransack sort uses the computed stock columns, include them in the select
+      # so the generated COUNT/DISTINCT query is valid.
+      sort_columns = Array(@q && @q[:s]).flatten
+      if sort_columns.any? { |s|
+           s.to_s.include?('on_hand') || s.to_s.include?('backorderable_priority')
+         }
+
+        product_query = product_query.select(
+          Arel.sql('spree_products.*'),
+          Spree::Product.backorderable_priority_sql,
+          Spree::Product.on_hand_sql
+        )
+      end
+
+      @pagy, @products = pagy(
+        product_query.order(:name),
+        limit: @per_page,
+        page: @page,
+        size: [1, 2, 2, 1]
+      )
     end
 
     def product_scope

app/models/concerns/product_sort_by_stocks.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+module ProductSortByStocks
+  extend ActiveSupport::Concern
+
+  included do
+    @on_hand_sql = Arel.sql("(
+      SELECT COALESCE(SUM(si.count_on_hand), 0)
+      FROM spree_variants v
+      JOIN spree_stock_items si ON si.variant_id = v.id
+      WHERE v.product_id = spree_products.id
+      GROUP BY v.product_id
+    )")
+
+    @backorderable_priority_sql = Arel.sql("(
+      SELECT BOOL_OR(si.backorderable)
+      FROM spree_variants v
+      JOIN spree_stock_items si ON si.variant_id = v.id
+      WHERE v.product_id = spree_products.id
+      GROUP BY v.product_id
+    )")
+
+    class << self
+      attr_reader :on_hand_sql, :backorderable_priority_sql
+    end
+
+    ransacker :on_hand do
+      @on_hand_sql
+    end
+
+    ransacker :backorderable_priority do
+      @backorderable_priority_sql
+    end
+  end
+end

app/models/spree/product.rb

@@ -19,6 +19,7 @@ require 'open_food_network/property_merge'
 module Spree
   class Product < ApplicationRecord
     include LogDestroyPerformer
+    include ProductSortByStocks
 
     self.belongs_to_required_by_default = false
     # These columns have been moved to variant. Currently this is only for documentation purposes,
@@ -30,7 +31,7 @@ module Spree
 
     acts_as_paranoid
 
-    searchable_attributes :meta_keywords, :sku
+    searchable_attributes :meta_keywords, :sku, :on_hand, :backorderable_priority
     searchable_associations :properties, :variants
     searchable_scopes :active, :with_properties
 

app/views/admin/products_v3/_table.html.haml

@@ -59,7 +59,8 @@
         %th.align-left.col-unit_scale.with-input= t('admin.products_page.columns.unit_scale')
         %th.align-left.col-unit.with-input= t('admin.products_page.columns.unit')
         %th.align-left.col-price.with-input= t('admin.products_page.columns.price')
-        %th.align-left.col-on_hand.with-input= t('admin.products_page.columns.on_hand')
+        = render partial: 'spree/admin/shared/stimulus_sortable_header',
+            locals: { column: :on_hand, sorted: params.dig(:q, :s), default: 'name asc' }
         %th.align-left.col-producer= t('admin.products_page.columns.producer')
         %th.align-left.col-category= t('admin.products_page.columns.category')
         %th.align-left.col-tax_category= t('admin.products_page.columns.tax_category')

config/locales/en.yml

@@ -4749,6 +4749,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using
           terms_of_service: "Terms of Service"
         sortable_header:
           name: "Name"
+          on_hand: "On Hand"
           number: "Number"
           completed_at: "Completed At"
           state: "State"

spec/models/spree/product_sort_by_stocks_spec.rb

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'ProductSortByStocks' do
+  let(:product) { create(:product) }
+
+  describe 'class level SQL accessors' do
+    it 'exposes SQL Arel nodes for sorting' do
+      expect(Spree::Product.on_hand_sql).to be_a(Arel::Nodes::SqlLiteral)
+      expect(Spree::Product.backorderable_priority_sql).to be_a(Arel::Nodes::SqlLiteral)
+    end
+  end
+
+  describe 'on_hand ransacker behaviour' do
+    it 'can be sorted via ransack by on_hand' do
+      product1 = create(:product)
+      product2 = create(:product)
+
+      product1.variants.first.stock_items.update_all(count_on_hand: 2)
+      product2.variants.first.stock_items.update_all(count_on_hand: 7)
+
+      # ransack sort key: 'on_hand asc' should put product1 before product2
+      result = Spree::Product.ransack(s: 'on_hand asc').result.to_a
+
+      expect(result.index(product1)).to be < result.index(product2)
+    end
+  end
+
+  describe 'backorderable_priority ransacker behaviour' do
+    it 'can be sorted via ransack by backorderable_priority' do
+      product1 = create(:product)
+      product2 = create(:product)
+
+      [product1, product2].each { |p|
+        p.variants.each { |v|
+          v.stock_items.update_all(backorderable: false)
+        }
+      }
+      product2.variants.first.stock_items.update_all(backorderable: true)
+
+      result = Spree::Product.ransack(s: 'backorderable_priority desc').result.to_a
+
+      expect(result.index(product2)).to be < result.index(product1)
+    end
+  end
+
+  describe 'combined sorting' do
+    # shared products for combined sorting examples
+    let!(:low) { create(:product) }
+    let!(:mid) { create(:product) }
+    let!(:high) { create(:product) }
+
+    it 'supports combined sorting: backorderable_priority (on-demad) then on_hand (asc)' do
+      low.variants.first.stock_items.update_all(count_on_hand: 1)
+      mid.variants.first.stock_items.update_all(count_on_hand: 5)
+      high.variants.first.stock_items.update_all(count_on_hand: 10)
+
+      # Make 'mid' backorderable so it sorts before 'low' in backorderable_priority asc
+      mid.variants.first.stock_items.update_all(backorderable: true)
+
+      # Controller transforms 'on_hand asc' into ['backorderable_priority asc', 'on_hand asc']
+      result = Spree::Product.ransack(s: ['backorderable_priority asc',
+                                          'on_hand asc']).result.to_a
+
+      expect(result).to eq([low, high, mid])
+    end
+
+    it 'supports combined sorting: backorderable_priority (on-demand) then on_hand (desc)' do
+      low.variants.first.stock_items.update_all(count_on_hand: 2)
+      mid.variants.first.stock_items.update_all(count_on_hand: 6)
+      high.variants.first.stock_items.update_all(count_on_hand: 9)
+
+      # make 'mid' backorderable so with primary sort desc by backorderable_priority,
+      # so mid should appear before high and low
+      mid.variants.first.stock_items.update_all(backorderable: true)
+
+      # Controller transforms 'on_hand desc' into ['backorderable_priority desc', 'on_hand desc']
+      result = Spree::Product.ransack(s: ['backorderable_priority desc',
+                                          'on_hand desc']).result.to_a
+
+      expect(result).to eq([mid, high, low])
+    end
+  end
+end

spec/system/admin/products_v3/index_spec.rb

@@ -164,6 +164,41 @@ RSpec.describe 'As an enterprise user, I can manage my products' do
         end
       end
     end
+
+    context "when clicked on 'On Hand' column header" do
+      it 'sorts products with on-demand at the top in descending order (bottom in ascending),
+          then by total stock across all variants' do
+        # Setup products with different stock levels and backorderable (on-demand) settings
+        # product with 2 on_hand stock items
+        product_a.variants.first.stock_items.update_all(count_on_hand: 2)
+        # product with on-demand stock items
+        product_b.variants.first.stock_items.update_all(count_on_hand: 0, backorderable: true)
+
+        # product with multiple variants having different on_hand stock items
+        product_c = create(:simple_product, name: "Cherries")
+        product_c.variants.first.stock_items.update_all(count_on_hand: 1, backorderable: false)
+        create(:variant, product: product_c, on_hand: 3)
+
+        # product with multiple variants having one on-demand item
+        product_d = create(:simple_product, name: "Dates")
+        product_d.variants.first.stock_items.update_all(count_on_hand: 1, backorderable: false)
+        create(:variant, product: product_d, on_hand: 0, on_demand: true)
+
+        within products_table do
+          on_hand_header = page.find('th > a[data-column="on_hand"]')
+
+          # Sort in acscending order
+          on_hand_header.click
+          expect(page).to have_content("On Hand ▲") # this indicates the re-sorted
+          expect(all_input_values).to match /Apples.*Cherries.*Bananas.*Dates/
+
+          # Sort in descending order
+          on_hand_header.click
+          expect(page).to have_content("On Hand ▼") # this indicates the re-sorted
+          expect(all_input_values).to match /Dates.*Bananas.*Cherries.*Apples/
+        end
+      end
+    end
   end
 
   describe "pagination" do