From 1d8b942acd755ec6be9dfc47bd72f760a47d3458 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Apr 2024 14:03:25 +1000 Subject: [PATCH 1/6] Fix spec for invoice ordering --- spec/system/admin/orders_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 2a70eef01c..cbdfe44330 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -686,7 +686,7 @@ describe ' invoice_content = extract_pdf_content expect( - invoice_content + invoice_content.join ).to match(/#{surnames[0]}.*#{surnames[1]}.*#{surnames[2]}.*#{surnames[3]}/m) end end From 16c877f7cb0c3409ee4fc59d53d1614410148bae Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Apr 2024 14:51:55 +1000 Subject: [PATCH 2/6] Fix: preserve order of invoices in bulk print --- app/reflexes/admin/orders_reflex.rb | 7 ++++++- spec/system/admin/orders_spec.rb | 2 -- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/reflexes/admin/orders_reflex.rb b/app/reflexes/admin/orders_reflex.rb index 8e5f43eb65..ea07b3e9a5 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -44,8 +44,13 @@ module Admin html: render(partial: "spree/admin/orders/bulk/invoice_modal") ).broadcast + # Preserve order of bulk_ids. + # The ids are supplied in the sequence of the orders screen and may be + # sorted, for example by last name of the customer. + visible_order_ids = params[:bulk_ids].map(&:to_i) & visible_orders.pluck(:id) + BulkInvoiceJob.perform_later( - visible_orders.pluck(:id), + visible_order_ids, "tmp/invoices/#{Time.zone.now.to_i}-#{SecureRandom.hex(2)}.pdf", channel: SessionChannel.for_request(request), current_user_id: current_user.id diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index cbdfe44330..01aa5d5802 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -710,7 +710,6 @@ describe ' order4.name.gsub(/.* /, ""), order5.name.gsub(/.* /, "")].sort } before do - pending("#12340") page.find('a', text: "NAME").click # orders alphabetically (asc) sleep(0.5) # waits for column sorting page.find('#selectAll').click @@ -723,7 +722,6 @@ describe ' order4.name.gsub(/.* /, ""), order5.name.gsub(/.* /, "")].sort.reverse } before do - pending("#12340") page.find('a', text: "NAME").click # orders alphabetically (asc) sleep(0.5) # waits for column sorting page.find('a', text: "NAME").click # orders alphabetically (desc) From 48b447500f6fb1c126ffa183767205ab11d991eb Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Apr 2024 15:57:25 +1000 Subject: [PATCH 3/6] Move selection of invoicable orders to database It's more efficient and should allow for further optimisations. --- app/jobs/bulk_invoice_job.rb | 17 ++++++++++------ app/models/spree/order.rb | 5 +---- app/reflexes/admin/orders_reflex.rb | 6 +++--- spec/models/spree/order_spec.rb | 30 +++++++++++++---------------- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/app/jobs/bulk_invoice_job.rb b/app/jobs/bulk_invoice_job.rb index 517cba1c22..4eab661a66 100644 --- a/app/jobs/bulk_invoice_job.rb +++ b/app/jobs/bulk_invoice_job.rb @@ -7,9 +7,14 @@ class BulkInvoiceJob < ApplicationJob def perform(order_ids, filepath, options = {}) @options = options - orders = sorted_orders(order_ids) - orders.filter!(&:invoiceable?) if OpenFoodNetwork::FeatureToggle.enabled?(:invoices, - current_user) + + orders = Spree::Order.where(id: order_ids) + + if OpenFoodNetwork::FeatureToggle.enabled?(:invoices, current_user) + orders = orders.invoiceable + end + + orders = sort_orders(orders, order_ids) orders.each(&method(:generate_invoice)) ensure_directory_exists filepath @@ -22,9 +27,9 @@ class BulkInvoiceJob < ApplicationJob private # Ensures the records are returned in the same order the ids were originally given in - def sorted_orders(order_ids) - orders_by_id = Spree::Order.where(id: order_ids).to_a.index_by(&:id) - order_ids.map { |id| orders_by_id[id.to_i] } + def sort_orders(orders, order_ids) + orders_by_id = orders.to_a.index_by(&:id) + order_ids.map { |id| orders_by_id[id.to_i] }.compact end def renderer diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index fbb41c5abb..8301db41d3 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -165,6 +165,7 @@ module Spree scope :finalized, -> { where(state: FINALIZED_STATES) } scope :complete, -> { where.not(completed_at: nil) } scope :incomplete, -> { where(completed_at: nil) } + scope :invoiceable, -> { where(state: [:complete, :resumed]) } scope :by_state, lambda { |state| where(state:) } scope :not_state, lambda { |state| where.not(state:) } @@ -213,10 +214,6 @@ module Spree completed_at.present? end - def invoiceable? - complete? || resumed? - end - # Indicates whether or not the user is allowed to proceed to checkout. # Currently this is implemented as a check for whether or not there is at # least one LineItem in the Order. Feel free to override this logic in your diff --git a/app/reflexes/admin/orders_reflex.rb b/app/reflexes/admin/orders_reflex.rb index ea07b3e9a5..f7a562d745 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -32,7 +32,7 @@ module Admin end def bulk_invoice(params) - visible_orders = editable_orders.where(id: params[:bulk_ids]).filter(&:invoiceable?) + visible_orders = editable_orders.invoiceable.where(id: params[:bulk_ids]) if Spree::Config.enterprise_number_required_on_invoices? && !all_distributors_can_invoice?(visible_orders) render_business_number_required_error(visible_orders) @@ -87,8 +87,8 @@ module Admin def send_invoices(params) count = 0 - editable_orders.where(id: params[:bulk_ids]).find_each do |o| - next unless o.distributor.can_invoice? && o.invoiceable? + editable_orders.invoiceable.where(id: params[:bulk_ids]).find_each do |o| + next unless o.distributor.can_invoice? Spree::OrderMailer.invoice_email(o.id, current_user_id: current_user.id).deliver_later count += 1 diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 480cef7cab..9310c83b00 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -140,23 +140,6 @@ describe Spree::Order do end end - context "#invoiceable?" do - it "should return true if the order is completed" do - allow(order).to receive_messages(complete?: true) - expect(order.invoiceable?).to be_truthy - end - - it "should return true if the order is resumed" do - allow(order).to receive_messages(resumed?: true) - expect(order.invoiceable?).to be_truthy - end - - it "should return false if the order is neither completed nor resumed" do - allow(order).to receive_messages(complete?: false, resumed?: false) - expect(order.invoiceable?).to be_falsy - end - end - context '#changes_allowed?' do let(:order) { create(:order_ready_for_details) } let(:complete) { true } @@ -997,6 +980,19 @@ describe Spree::Order do end describe "scopes" do + describe "invoiceable" do + it "finds only active orders" do + order_complete = create(:order, state: :complete) + order_canceled = create(:order, state: :canceled) + order_resumed = create(:order, state: :resumed) + + expect(Spree::Order.invoiceable).to match_array [ + order_complete, + order_resumed, + ] + end + end + describe "not_state" do it "finds only orders not in specified state" do o = FactoryBot.create(:completed_order_with_totals, From 3382a62eb5bbe6f1e61b5858b8f8b17f7aa14b77 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Apr 2024 16:08:14 +1000 Subject: [PATCH 4/6] Simplify BulkInvoiceJob by removing checks The check for invoiceability is already done by the reflex triggering the job. Let's DRY the code, save time and be more flexible in the future. Also checking the order of actually generated PDF pages. --- app/jobs/bulk_invoice_job.rb | 14 ++---------- spec/jobs/bulk_invoice_job_spec.rb | 35 +++++++++++------------------- 2 files changed, 15 insertions(+), 34 deletions(-) diff --git a/app/jobs/bulk_invoice_job.rb b/app/jobs/bulk_invoice_job.rb index 4eab661a66..c77eb077f9 100644 --- a/app/jobs/bulk_invoice_job.rb +++ b/app/jobs/bulk_invoice_job.rb @@ -8,13 +8,9 @@ class BulkInvoiceJob < ApplicationJob def perform(order_ids, filepath, options = {}) @options = options - orders = Spree::Order.where(id: order_ids) + # The `find` method returns records in the same order as the given ids. + orders = Spree::Order.find(order_ids) - if OpenFoodNetwork::FeatureToggle.enabled?(:invoices, current_user) - orders = orders.invoiceable - end - - orders = sort_orders(orders, order_ids) orders.each(&method(:generate_invoice)) ensure_directory_exists filepath @@ -26,12 +22,6 @@ class BulkInvoiceJob < ApplicationJob private - # Ensures the records are returned in the same order the ids were originally given in - def sort_orders(orders, order_ids) - orders_by_id = orders.to_a.index_by(&:id) - order_ids.map { |id| orders_by_id[id.to_i] }.compact - end - def renderer @renderer ||= InvoiceRenderer.new end diff --git a/spec/jobs/bulk_invoice_job_spec.rb b/spec/jobs/bulk_invoice_job_spec.rb index ace4234103..907749033c 100644 --- a/spec/jobs/bulk_invoice_job_spec.rb +++ b/spec/jobs/bulk_invoice_job_spec.rb @@ -5,45 +5,36 @@ require 'spec_helper' describe BulkInvoiceJob do subject { BulkInvoiceJob.new(order_ids, "/tmp/file/path") } - describe "#sorted_orders" 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] } - - 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 - context "when invoices are enabled", feature: :invoices do describe "#perform" do let!(:order1) { create(:shipped_order) } - let!(:order2) { create(:order_with_line_items) } + let!(:order2) { create(:shipped_order) } let!(:order3) { create(:order_ready_to_ship) } - let(:order_ids) { [order1.id, order2.id, order3.id] } + let(:order_ids) { [order3.id, order1.id, order2.id] } let(:path){ "/tmp/file/path.pdf" } + before do allow(TermsOfServiceFile).to receive(:current_url).and_return("http://example.com/terms.pdf") order3.cancel order3.resume end - it "should generate invoices for invoiceable orders only" do + + it "should generate invoices for given order ids" do expect{ subject.perform(order_ids, path) }.to change{ order1.invoices.count }.from(0).to(1) - .and change{ order2.invoices.count }.by(0) + .and change{ order2.invoices.count }.from(0).to(1) .and change{ order3.invoices.count }.from(0).to(1) - File.open(path, "rb") do |io| + pages = File.open(path, "rb") do |io| reader = PDF::Reader.new(io) - content = reader.pages.map(&:text).join("\n") - expect(content).to include(order1.number) - expect(content).to include(order3.number) - expect(content).not_to include(order2.number) + reader.pages.map(&:text) end + + # Pages should be in the order of order ids given: + expect(pages[0]).to include(order3.number) + expect(pages[1]).to include(order1.number) + expect(pages[2]).to include(order2.number) end end end From b03bb30a8e20c2fd403e6a6ba92a7af95ba6a199 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Apr 2024 16:23:54 +1000 Subject: [PATCH 5/6] Move ABN checks to the database --- app/reflexes/admin/orders_reflex.rb | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/app/reflexes/admin/orders_reflex.rb b/app/reflexes/admin/orders_reflex.rb index f7a562d745..8010277ecc 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -33,10 +33,17 @@ module Admin def bulk_invoice(params) visible_orders = editable_orders.invoiceable.where(id: params[:bulk_ids]) - if Spree::Config.enterprise_number_required_on_invoices? && - !all_distributors_can_invoice?(visible_orders) - render_business_number_required_error(visible_orders) - return + + if Spree::Config.enterprise_number_required_on_invoices? + distributors_without_abn = Enterprise.where( + id: visible_orders.select(:distributor_id), + abn: nil, + ) + + if distributors_without_abn.exists? + render_business_number_required_error(distributors_without_abn) + return + end end cable_ready.append( @@ -124,9 +131,8 @@ module Admin Enterprise.where(id: distributor_ids, abn: nil).empty? end - def render_business_number_required_error(orders) - distributor_ids = orders.map(&:distributor_id) - distributor_names = Enterprise.where(id: distributor_ids, abn: nil).pluck(:name) + def render_business_number_required_error(distributors) + distributor_names = distributors.pluck(:name) flash[:error] = I18n.t(:must_have_valid_business_number, enterprise_name: distributor_names.join(", ")) From 34fc6283b87d6a0ed2f415effa3261cce0f68509 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 11 Apr 2024 11:46:52 +1000 Subject: [PATCH 6/6] Remove unused code --- app/reflexes/admin/orders_reflex.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/reflexes/admin/orders_reflex.rb b/app/reflexes/admin/orders_reflex.rb index 8010277ecc..b52136391e 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -126,11 +126,6 @@ module Admin params[:id] = @order.number end - def all_distributors_can_invoice?(orders) - distributor_ids = orders.map(&:distributor_id) - Enterprise.where(id: distributor_ids, abn: nil).empty? - end - def render_business_number_required_error(distributors) distributor_names = distributors.pluck(:name)