diff --git a/app/jobs/bulk_invoice_job.rb b/app/jobs/bulk_invoice_job.rb index 517cba1c22..c77eb077f9 100644 --- a/app/jobs/bulk_invoice_job.rb +++ b/app/jobs/bulk_invoice_job.rb @@ -7,9 +7,10 @@ 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) + + # The `find` method returns records in the same order as the given ids. + orders = Spree::Order.find(order_ids) + orders.each(&method(:generate_invoice)) ensure_directory_exists filepath @@ -21,12 +22,6 @@ 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] } - end - def renderer @renderer ||= InvoiceRenderer.new end diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 5964af833b..561aabacac 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 8e5f43eb65..b52136391e 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -32,11 +32,18 @@ module Admin end def bulk_invoice(params) - visible_orders = editable_orders.where(id: params[:bulk_ids]).filter(&:invoiceable?) - if Spree::Config.enterprise_number_required_on_invoices? && - !all_distributors_can_invoice?(visible_orders) - render_business_number_required_error(visible_orders) - return + visible_orders = editable_orders.invoiceable.where(id: params[:bulk_ids]) + + 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( @@ -44,8 +51,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 @@ -82,8 +94,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 @@ -114,14 +126,8 @@ 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(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(", ")) 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 diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index e3efd69a4b..3a0dfb7d3c 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, diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 2a70eef01c..01aa5d5802 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 @@ -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)