From b2b988b0636eb5b7200ba4805bae48df1cb973e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Sun, 24 May 2020 14:41:40 +0200 Subject: [PATCH 1/5] Apply new order directly inside service --- app/services/bulk_invoice_service.rb | 7 +++++-- spec/services/bulk_invoice_service_spec.rb | 7 +++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/services/bulk_invoice_service.rb b/app/services/bulk_invoice_service.rb index 18921bd413..ab69df03ca 100644 --- a/app/services/bulk_invoice_service.rb +++ b/app/services/bulk_invoice_service.rb @@ -7,9 +7,8 @@ class BulkInvoiceService def start_pdf_job(order_ids) pdf = CombinePDF.new - orders = Spree::Order.where(id: order_ids) - orders.each do |order| + orders_from(order_ids).each do |order| invoice = renderer.render_to_string(order) pdf << CombinePDF.parse(invoice) @@ -29,6 +28,10 @@ class BulkInvoiceService private + def orders_from(order_ids) + Spree::Order.where(id: order_ids).order("completed_at DESC") + end + def new_invoice_id Time.zone.now.to_i.to_s end diff --git a/spec/services/bulk_invoice_service_spec.rb b/spec/services/bulk_invoice_service_spec.rb index 4056a1b88f..1874a0fcb4 100644 --- a/spec/services/bulk_invoice_service_spec.rb +++ b/spec/services/bulk_invoice_service_spec.rb @@ -47,4 +47,11 @@ describe BulkInvoiceService do expect(filepath).to eq 'tmp/invoices/1234567.pdf' end end + + describe "#orders_from" do + it "orders with completed desc" do + expect(service.send(:orders_from, [1, 2]).to_sql) + .to include('ORDER BY completed_at DESC') + end + end end From 6056c1699f58a0a3bb75e8e7accbfe3d75e8f65b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 9 Jun 2020 10:10:34 +0200 Subject: [PATCH 2/5] Use batches in order to fetch orders --- app/services/bulk_invoice_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/bulk_invoice_service.rb b/app/services/bulk_invoice_service.rb index ab69df03ca..b4c096b3c3 100644 --- a/app/services/bulk_invoice_service.rb +++ b/app/services/bulk_invoice_service.rb @@ -8,7 +8,7 @@ class BulkInvoiceService def start_pdf_job(order_ids) pdf = CombinePDF.new - orders_from(order_ids).each do |order| + orders_from(order_ids).find_each do |order| invoice = renderer.render_to_string(order) pdf << CombinePDF.parse(invoice) From 795106aaa28293048c9003dc93d88894d2abb1c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 9 Jun 2020 10:11:11 +0200 Subject: [PATCH 3/5] Use real orders inside spec to extend coverage --- spec/services/bulk_invoice_service_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/services/bulk_invoice_service_spec.rb b/spec/services/bulk_invoice_service_spec.rb index 1874a0fcb4..52fe085288 100644 --- a/spec/services/bulk_invoice_service_spec.rb +++ b/spec/services/bulk_invoice_service_spec.rb @@ -50,8 +50,11 @@ describe BulkInvoiceService do describe "#orders_from" do it "orders with completed desc" do - expect(service.send(:orders_from, [1, 2]).to_sql) - .to include('ORDER BY completed_at DESC') + order_old = create(:order_with_distributor, :completed, completed_at: 2.minutes.ago) + order_older = create(:order_with_distributor, :completed, completed_at: 3.minutes.ago) + + expect(service.send(:orders_from, [order_older.id, order_old.id]).pluck(:id)) + .to eq([order_old.id, order_older.id]) end end end From 680ee3dc860da5bf6ad4af1c1b444d788dffeb80 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Jun 2020 10:05:47 +1000 Subject: [PATCH 4/5] Check the bulk invoice rendering order As it turns out, our performance optimisation with `find_each` overrides or custom sorting. --- spec/services/bulk_invoice_service_spec.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/spec/services/bulk_invoice_service_spec.rb b/spec/services/bulk_invoice_service_spec.rb index 52fe085288..1ff3d9edab 100644 --- a/spec/services/bulk_invoice_service_spec.rb +++ b/spec/services/bulk_invoice_service_spec.rb @@ -49,12 +49,25 @@ describe BulkInvoiceService do 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, :completed, completed_at: 2.minutes.ago) + order_oldest = create(:order_with_distributor, :completed, completed_at: 4.minutes.ago) order_older = create(:order_with_distributor, :completed, completed_at: 3.minutes.ago) - expect(service.send(:orders_from, [order_older.id, order_old.id]).pluck(:id)) - .to eq([order_old.id, order_older.id]) + # This is the creation order provided `find_each` which invalidates + # our intended sorting by `completed_at`: + expect(renderer).to receive(:render_to_string).with(order_old).ordered.and_return("") + expect(renderer).to receive(:render_to_string).with(order_oldest).ordered.and_return("") + expect(renderer).to receive(:render_to_string).with(order_older).ordered.and_return("") + + order_ids = [order_oldest, order_old, order_older].map(&:id) + service.start_pdf_job_without_delay(order_ids) end end end From 7d53b12baf029cac51651b59f2f12ca2dc3fd53a Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Jun 2020 10:08:06 +1000 Subject: [PATCH 5/5] Preserve sorting by completion date --- app/services/bulk_invoice_service.rb | 2 +- spec/services/bulk_invoice_service_spec.rb | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/services/bulk_invoice_service.rb b/app/services/bulk_invoice_service.rb index b4c096b3c3..ab69df03ca 100644 --- a/app/services/bulk_invoice_service.rb +++ b/app/services/bulk_invoice_service.rb @@ -8,7 +8,7 @@ class BulkInvoiceService def start_pdf_job(order_ids) pdf = CombinePDF.new - orders_from(order_ids).find_each do |order| + orders_from(order_ids).each do |order| invoice = renderer.render_to_string(order) pdf << CombinePDF.parse(invoice) diff --git a/spec/services/bulk_invoice_service_spec.rb b/spec/services/bulk_invoice_service_spec.rb index 1ff3d9edab..687e680bcc 100644 --- a/spec/services/bulk_invoice_service_spec.rb +++ b/spec/services/bulk_invoice_service_spec.rb @@ -60,11 +60,9 @@ describe BulkInvoiceService do order_oldest = create(:order_with_distributor, :completed, completed_at: 4.minutes.ago) order_older = create(:order_with_distributor, :completed, completed_at: 3.minutes.ago) - # This is the creation order provided `find_each` which invalidates - # our intended sorting by `completed_at`: expect(renderer).to receive(:render_to_string).with(order_old).ordered.and_return("") - expect(renderer).to receive(:render_to_string).with(order_oldest).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) service.start_pdf_job_without_delay(order_ids)