diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 7b05201c47..6d1ae388cb 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/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 diff --git a/app/models/concerns/product_sort_by_stocks.rb b/app/models/concerns/product_sort_by_stocks.rb new file mode 100644 index 0000000000..1fce5fda3f --- /dev/null +++ b/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 diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index a0b9da8dfe..5b2c4d2838 100755 --- a/app/models/spree/product.rb +++ b/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 diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 15d7646c6c..003194a3fb 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/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') diff --git a/config/locales/en.yml b/config/locales/en.yml index 1dcd04cb82..6460bab0d5 100644 --- a/config/locales/en.yml +++ b/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" diff --git a/spec/models/spree/product_sort_by_stocks_spec.rb b/spec/models/spree/product_sort_by_stocks_spec.rb new file mode 100644 index 0000000000..573fea404e --- /dev/null +++ b/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 diff --git a/spec/system/admin/products_v3/index_spec.rb b/spec/system/admin/products_v3/index_spec.rb index 83e72a885e..130831ca66 100644 --- a/spec/system/admin/products_v3/index_spec.rb +++ b/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