From 9c280ee61215ff65e95d64dd28b2b1082a299ca7 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Dec 2021 21:26:02 +0100 Subject: [PATCH 1/5] add new line --- app/views/spree/admin/shared/_sortable_header.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/spree/admin/shared/_sortable_header.html.haml b/app/views/spree/admin/shared/_sortable_header.html.haml index 3aa8908576..2804ad20e4 100644 --- a/app/views/spree/admin/shared/_sortable_header.html.haml +++ b/app/views/spree/admin/shared/_sortable_header.html.haml @@ -1,4 +1,4 @@ %a{'ng-click' => "sortOptions.toggle('#{column_name}')"} = t(".#{column_name.to_s}") %span{'ng-show' => "sorting == '#{column_name} asc'"}= "▲".html_safe - %span{'ng-show' => "sorting == '#{column_name} desc'"}= "▼".html_safe \ No newline at end of file + %span{'ng-show' => "sorting == '#{column_name} desc'"}= "▼".html_safe From 7a48ffdd3849121f19daabf0541364154df39efb Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Dec 2021 21:27:42 +0100 Subject: [PATCH 2/5] Add order.full_name as new column + add the linked sortable header via `bill_address_lastname` --- app/views/spree/admin/orders/index.html.haml | 4 ++-- config/locales/en.yml | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/views/spree/admin/orders/index.html.haml b/app/views/spree/admin/orders/index.html.haml index d06562fd2a..0d889cce2c 100644 --- a/app/views/spree/admin/orders/index.html.haml +++ b/app/views/spree/admin/orders/index.html.haml @@ -41,7 +41,7 @@ %span{'ng-show' => "sorting == 'completed_at asc'"}= "▲".html_safe %span{'ng-show' => "sorting == 'completed_at desc' || sorting === undefined"}= "▼".html_safe - - ['number', 'state', 'payment_state', 'shipment_state', 'email', 'total'].each do |column_name| + - ['number', 'state', 'payment_state', 'shipment_state', 'email', 'bill_address_lastname', 'total'].each do |column_name| %th = render partial: 'spree/admin/shared/sortable_header', locals: {column_name: column_name} @@ -76,7 +76,7 @@ %td %a{ ng: { href: "mailto:{{order.email}}" } } {{order.email}} - %br + %td {{order.full_name}} %td.align-center %span{'ng-bind-html' => 'order.display_total'} diff --git a/config/locales/en.yml b/config/locales/en.yml index eec506ff28..919b99a127 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3881,6 +3881,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using shipment_state: "Shipment State" email: "Email" total: "Total" + bill_address_lastname: "Name" general_settings: edit: legal_settings: "Legal Settings" From 45beefc5336fc9895c467dec7a8bfd1bae0cfdec Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 15 Dec 2021 20:29:12 +0000 Subject: [PATCH 3/5] Keep the order of passed argument Do not retrieve the orders list sorted by completed_at but by the order_ids array itself Add BulkInvoiceJob spec and cover sorting behaviour --- app/jobs/bulk_invoice_job.rb | 7 +++--- spec/jobs/bulk_invoice_job_spec.rb | 21 +++++++++++++++++ spec/services/bulk_invoice_service_spec.rb | 26 ---------------------- 3 files changed, 25 insertions(+), 29 deletions(-) create mode 100644 spec/jobs/bulk_invoice_job_spec.rb diff --git a/app/jobs/bulk_invoice_job.rb b/app/jobs/bulk_invoice_job.rb index 7335c552d6..3a1969c49a 100644 --- a/app/jobs/bulk_invoice_job.rb +++ b/app/jobs/bulk_invoice_job.rb @@ -4,7 +4,7 @@ class BulkInvoiceJob < ActiveJob::Base def perform(order_ids, filepath) pdf = CombinePDF.new - orders_from(order_ids).each do |order| + sorted_orders(order_ids).each do |order| invoice = renderer.render_to_string(order) pdf << CombinePDF.parse(invoice) @@ -15,8 +15,9 @@ class BulkInvoiceJob < ActiveJob::Base private - def orders_from(order_ids) - Spree::Order.where(id: order_ids).order("completed_at DESC") + # Ensures the records are returned in the same order the ids were originally given in + def sorted_orders(order_ids) + Spree::Order.where(id: order_ids).to_a.sort_by{ |order| order_ids.index(order.id) } end def renderer diff --git a/spec/jobs/bulk_invoice_job_spec.rb b/spec/jobs/bulk_invoice_job_spec.rb new file mode 100644 index 0000000000..d783e88321 --- /dev/null +++ b/spec/jobs/bulk_invoice_job_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe BulkInvoiceJob do + let(:order1) { build(:order, id: 1) } + let(:order2) { build(:order, id: 2) } + let(:order3) { build(:order, id: 3) } + + let(:order_ids) { [3, 1, 2] } + + subject { BulkInvoiceJob.new(order_ids, "/tmp/file/path") } + + describe "#sorted_orders" do + it "returns results in their original order" do + expect(Spree::Order).to receive(:where).and_return([order1, order2, order3]) + + expect(subject.__send__(:sorted_orders, order_ids)).to eq [order3, order1, order2] + end + end +end diff --git a/spec/services/bulk_invoice_service_spec.rb b/spec/services/bulk_invoice_service_spec.rb index 62aaa977cb..c2142d15b6 100644 --- a/spec/services/bulk_invoice_service_spec.rb +++ b/spec/services/bulk_invoice_service_spec.rb @@ -53,30 +53,4 @@ describe BulkInvoiceService do expect(filepath).to eq 'tmp/invoices/1234567.pdf' end end - - describe "#orders_from" do - let(:renderer) { InvoiceRenderer.new } - - before do - allow(InvoiceRenderer).to receive(:new).and_return(renderer) - end - - it "orders with completed desc" do - order_old = create(:order_with_distributor, :with_line_item, :completed, - completed_at: 2.minutes.ago) - order_oldest = create(:order_with_distributor, :with_line_item, :completed, - completed_at: 4.minutes.ago) - order_older = create(:order_with_distributor, :with_line_item, :completed, - completed_at: 3.minutes.ago) - - expect(renderer).to receive(:render_to_string).with(order_old).ordered.and_return("") - expect(renderer).to receive(:render_to_string).with(order_older).ordered.and_return("") - expect(renderer).to receive(:render_to_string).with(order_oldest).ordered.and_return("") - - order_ids = [order_oldest, order_old, order_older].map(&:id) - perform_enqueued_jobs do - service.start_pdf_job(order_ids) - end - end - end end From 35e20fc179da821ca2a65f19b30ff90b79e90621 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 15 Dec 2021 20:03:33 +0000 Subject: [PATCH 4/5] Can sort orders via bill_address.lastname We only need to check the orders sort Fix ordering in api/v0/orders_controller_spec --- app/services/search_orders.rb | 2 +- .../controllers/api/v0/orders_controller_spec.rb | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb index 6ff7afcf3d..6b08d78cfb 100644 --- a/app/services/search_orders.rb +++ b/app/services/search_orders.rb @@ -19,7 +19,7 @@ class SearchOrders includes(:payments, :subscription, :shipments, :bill_address, :distributor, :order_cycle). ransack(params[:q]) - @search.result(distinct: true) + @search.result(distinct: true).joins(:bill_address) end def search_query diff --git a/spec/controllers/api/v0/orders_controller_spec.rb b/spec/controllers/api/v0/orders_controller_spec.rb index 42d75da5ab..cc72207f64 100644 --- a/spec/controllers/api/v0/orders_controller_spec.rb +++ b/spec/controllers/api/v0/orders_controller_spec.rb @@ -21,18 +21,19 @@ module Api let!(:order_cycle2) { create(:simple_order_cycle, coordinator: coordinator2) } let!(:order1) do create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, - distributor: distributor, billing_address: create(:address), total: 5.0) + distributor: distributor, billing_address: create(:address, lastname: "c"), total: 5.0) end let!(:order2) do create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, - distributor: distributor2, billing_address: create(:address), total: 10.0) + distributor: distributor2, billing_address: create(:address, lastname: "a"), total: 10.0) end let!(:order3) do create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, - distributor: distributor, billing_address: create(:address), total: 1.0 ) + distributor: distributor, billing_address: create(:address, lastname: "b"), total: 1.0 ) end let!(:order4) do - create(:completed_order_with_fees, order_cycle: order_cycle2, distributor: distributor2, total: 15.0) + create(:completed_order_with_fees, order_cycle: order_cycle2, distributor: distributor2, + billing_address: create(:address, lastname: "d"), total: 15.0) end let!(:order5) { create(:order, state: 'cart', completed_at: nil) } let!(:line_item1) do @@ -134,6 +135,13 @@ module Api expect(json_response['orders']).to eq serialized_orders([order4, order2, order1, order3]) end + + it 'can sort orders by bill_address.lastname' do + get :index, params: { q: { completed_at_not_null: true, s: 'bill_address_lastname ASC' } }, + as: :json + + expect(json_response['orders'].map{ |o| o[:id] }).to eq serialized_orders([order2, order3, order1, order4]).map{ |o| o["id"] } + end end context 'with pagination' do From 2a0b7df7f42c15687102a57f79788613ea877c7f Mon Sep 17 00:00:00 2001 From: jibees Date: Wed, 22 Dec 2021 10:03:34 +0100 Subject: [PATCH 5/5] clearer intention - it uses simple index_by and map which is easier to understand than each_with_index.to_h - it avoids sorting and therefore should be the most efficient solution. Co-authored-by: Maikel --- app/jobs/bulk_invoice_job.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/jobs/bulk_invoice_job.rb b/app/jobs/bulk_invoice_job.rb index 3a1969c49a..29e68c1088 100644 --- a/app/jobs/bulk_invoice_job.rb +++ b/app/jobs/bulk_invoice_job.rb @@ -17,7 +17,8 @@ class BulkInvoiceJob < ActiveJob::Base # Ensures the records are returned in the same order the ids were originally given in def sorted_orders(order_ids) - Spree::Order.where(id: order_ids).to_a.sort_by{ |order| order_ids.index(order.id) } + orders_by_id = Spree::Order.where(id: order_ids).to_a.index_by(&:id) + order_ids.map { |id| orders_by_id[id] } end def renderer