From 04312b05c62702fa00889aaf47b46a13f1d9fe0a Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Mon, 7 Aug 2023 19:18:01 +0100 Subject: [PATCH 1/4] add orders without billing address to SearchOrders#fetch_orders' results --- app/services/search_orders.rb | 12 ++++++++- .../spree/admin/orders/_table_row.html.haml | 2 +- .../api/v0/orders_controller_spec.rb | 27 +++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb index 60cf824e63..ce41bb036e 100644 --- a/app/services/search_orders.rb +++ b/app/services/search_orders.rb @@ -19,7 +19,17 @@ class SearchOrders includes(:payments, :subscription, :shipments, :bill_address, :distributor, :order_cycle). ransack(params[:q]) - @search.result(distinct: true).joins(:bill_address) + @search = @search.result(distinct: true) + + if ['bill_address', + 'billing_address'].any?{ |param| + params.dig(:q, :s)&.starts_with?(param) + } + @search = @search.left_joins(:bill_address). + select('spree_addresses.*, spree_orders.*') + end + + @search end def search_query diff --git a/app/views/spree/admin/orders/_table_row.html.haml b/app/views/spree/admin/orders/_table_row.html.haml index 3f8feb8d42..52875e7008 100644 --- a/app/views/spree/admin/orders/_table_row.html.haml +++ b/app/views/spree/admin/orders/_table_row.html.haml @@ -33,7 +33,7 @@ %a{ href: "mailto:#{order.email}", target: "_blank" } = order.email %td - = order.bill_address.full_name_for_sorting + = order&.bill_address.full_name_for_sorting %td.align-center %span = order.display_total diff --git a/spec/controllers/api/v0/orders_controller_spec.rb b/spec/controllers/api/v0/orders_controller_spec.rb index 773a9170b8..3f2e710bc2 100644 --- a/spec/controllers/api/v0/orders_controller_spec.rb +++ b/spec/controllers/api/v0/orders_controller_spec.rb @@ -149,6 +149,33 @@ module Api .map{ |o| o[:id] }).to eq serialized_orders([order2, order3, order1, order4]) .map{ |o| o["id"] } end + + context "with an order without billing address" do + let!(:order7) { + create(:order_with_line_items, billing_address: nil, order_cycle: order_cycle, + distributor: distributor) + } + + it 'can show orders without bill address' do + get :index, params: { q: {} }, + as: :json + + expect(json_response['orders'] + .map{ |o| o[:id] }).to match_array serialized_orders([order2, order3, order1, order4, + order7]) + .map{ |o| + o["id"] + } + end + + it 'can sort orders by bill_address.lastname' do + get :index, params: { q: { s: 'bill_address_lastname ASC' } }, + as: :json + expect(json_response['orders'] + .map{ |o| o[:id] }).to eq serialized_orders([order2, order3, order1, order4, order7]) + .map{ |o| o["id"] } + end + end end context 'with pagination' do From ccafdc44942beadabc770c79bf4c3b264ab012f0 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 24 Aug 2023 10:12:13 +1000 Subject: [PATCH 2/4] Remove redundant code bill_address is already joined in this query. The class variable isn't needed outside this scope. Arguably I think the condition on the select isn't needed; it wouldn't hurt to always select spree_addresses. But I'll try to avoid changing too much.. --- app/services/search_orders.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb index ce41bb036e..e9430b85ad 100644 --- a/app/services/search_orders.rb +++ b/app/services/search_orders.rb @@ -15,21 +15,19 @@ class SearchOrders attr_reader :params, :current_user def fetch_orders - @search = search_query. + search = search_query. includes(:payments, :subscription, :shipments, :bill_address, :distributor, :order_cycle). - ransack(params[:q]) - - @search = @search.result(distinct: true) + ransack(params[:q]). + result(distinct: true) if ['bill_address', 'billing_address'].any?{ |param| params.dig(:q, :s)&.starts_with?(param) } - @search = @search.left_joins(:bill_address). - select('spree_addresses.*, spree_orders.*') + search = search.select('spree_addresses.*, spree_orders.*') end - @search + search end def search_query From d8da8089010524ea8b01e8cee57c78ad0c84c347 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Aug 2023 15:15:34 +1000 Subject: [PATCH 3/4] Resolve pending spec, reported issue is fixed --- app/views/spree/admin/orders/_table_row.html.haml | 2 +- spec/system/admin/orders_spec.rb | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/views/spree/admin/orders/_table_row.html.haml b/app/views/spree/admin/orders/_table_row.html.haml index 52875e7008..b25b7c2c1a 100644 --- a/app/views/spree/admin/orders/_table_row.html.haml +++ b/app/views/spree/admin/orders/_table_row.html.haml @@ -33,7 +33,7 @@ %a{ href: "mailto:#{order.email}", target: "_blank" } = order.email %td - = order&.bill_address.full_name_for_sorting + = order&.bill_address&.full_name_for_sorting %td.align-center %span = order.display_total diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 1ed41fb713..2d362a6e38 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -269,8 +269,6 @@ describe ' end it "displays non-empty cart orders" do - pending "issue #11120" - # empty cart order does not appear in the results expect(page).not_to have_content order_empty.number @@ -278,8 +276,6 @@ describe ' expect(page).to have_content order_not_empty.number # non-empty cart order, with no with bill- and ship-address appear in the results - - # pending issue #11120 expect(page).to have_content order_not_empty_no_address.number end end From 200729f1988c93ddf6293fc97dc37c5710b160cc Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Aug 2023 15:45:52 +1000 Subject: [PATCH 4/4] Show incomplete orders when sorting by name An inner join with the billing address was hiding some orders when sorting by billing address name. Telling Rails that those fields are referenced triggers an outer left join including orders without billing address. But when the Bulk Order Management page sorts by `bill_address_lastname` then Ransack does most of the magic, except that we need to override the select in combination with distinct results. --- app/models/spree/order.rb | 6 ++++-- app/services/search_orders.rb | 5 +---- spec/system/admin/orders_spec.rb | 7 +++++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 6fc2ec1ac0..ea823f06b5 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -149,11 +149,13 @@ module Spree } scope :sort_by_billing_address_name_asc, -> { - joins(:bill_address).order("spree_addresses.lastname ASC, spree_addresses.firstname ASC") + references(:bill_address) + .order("spree_addresses.lastname ASC, spree_addresses.firstname ASC") } scope :sort_by_billing_address_name_desc, -> { - joins(:bill_address).order("spree_addresses.lastname DESC, spree_addresses.firstname DESC") + references(:bill_address) + .order("spree_addresses.lastname DESC, spree_addresses.firstname DESC") } scope :with_line_items_variants_and_products_outer, lambda { diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb index e9430b85ad..e8992d1b90 100644 --- a/app/services/search_orders.rb +++ b/app/services/search_orders.rb @@ -20,10 +20,7 @@ class SearchOrders ransack(params[:q]). result(distinct: true) - if ['bill_address', - 'billing_address'].any?{ |param| - params.dig(:q, :s)&.starts_with?(param) - } + if params.dig(:q, :s)&.starts_with?("bill_address_") search = search.select('spree_addresses.*, spree_orders.*') end diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 2d362a6e38..d64b74eb9d 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -277,6 +277,13 @@ describe ' # non-empty cart order, with no with bill- and ship-address appear in the results expect(page).to have_content order_not_empty_no_address.number + + # And the same orders are displayed when sorting by name: + find("th a", text: "NAME").click + + expect(page).to have_no_content order_empty.number + expect(page).to have_content order_not_empty.number + expect(page).to have_content order_not_empty_no_address.number end end