From adbfe3ae9d2876ab347040b602a7db01e8cca523 Mon Sep 17 00:00:00 2001 From: Paul Hoffer Date: Tue, 16 Jun 2026 21:47:23 -0700 Subject: [PATCH 1/5] add some new specs and testing --- .../export_distributions_csv_service.rb | 2 +- .../distributions_controller_spec.rb | 24 +++++++++++++++++++ .../export_distributions_csv_service_spec.rb | 22 +++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/services/exports/export_distributions_csv_service.rb b/app/services/exports/export_distributions_csv_service.rb index 137bb082c8..96567555ef 100644 --- a/app/services/exports/export_distributions_csv_service.rb +++ b/app/services/exports/export_distributions_csv_service.rb @@ -7,7 +7,7 @@ class ExportDistributionsCSVService include DistributionHelper include ItemsHelper - def initialize(distributions:, organization:, filters: []) + def initialize(distributions:, organization:, filters: {}) # Currently, the @distributions are already loaded by the controllers that are delegating exporting # to this service object; this is happening within the same request/response cycle, so it's already # in memory, so we can pass that collection in directly. Should this be moved to a background / async diff --git a/spec/controllers/distributions_controller_spec.rb b/spec/controllers/distributions_controller_spec.rb index 4a6ae7723e..a355637890 100644 --- a/spec/controllers/distributions_controller_spec.rb +++ b/spec/controllers/distributions_controller_spec.rb @@ -439,5 +439,29 @@ end end end + + describe "GET #index" do + context "when distribution has items updated for minimum quantity" do + let(:storage_location) { create(:storage_location, organization: organization) } + let(:distribution_count) { 5 } + let(:distributions) { create_list(:distribution, distribution_count, :with_items, item: items.sample, storage_location: storage_location, organization: organization) } + let(:items) { create_list(:item, 500, organization:, on_hand_minimum_quantity: 5) } + + it "responds 200" do + get :index + + expect(response).to have_http_status(:ok) + end + + it "downloads a CSV" do + distributions + get :index, format: :csv + + expect(response.header["Content-Type"]).to include "text/csv" + expect(response).to have_http_status(:ok) + expect(CSV.parse(response.body).size).to eq(distribution_count + 1) + end + end + end end end diff --git a/spec/services/exports/export_distributions_csv_service_spec.rb b/spec/services/exports/export_distributions_csv_service_spec.rb index 8d1631f422..29fcac9822 100644 --- a/spec/services/exports/export_distributions_csv_service_spec.rb +++ b/spec/services/exports/export_distributions_csv_service_spec.rb @@ -183,4 +183,26 @@ end end end + + # this is intended for easier performance testing, not to be run with rest of suite + # also, better for testing consistency to disable config.order = :random + # lastly, test setup time gets long, the printed duration is the actual generation time to pay attention to + xcontext "performance timing" do + [500, 1000, 2000, 5000, 10_000, 20_000, 30_000].reverse_each do |distribution_count| + it "processes #{distribution_count} distributions" do + partners = create_list(:partner, 500) + items = create_list(:item, 5000, organization:, on_hand_minimum_quantity: 5) + create_list(:distribution, distribution_count, :with_items, item: items.sample, storage_location: storage_location, organization: organization, partner: partners.sample) + TestInventory.create_inventory(organization, { + storage_location.id => items.map { [it.id, rand(20..69)] }.to_h + }) + t = Time.zone.now + csv = Exports::ExportDistributionsCSVService.new(distributions: organization.distributions.includes(:partner, :storage_location, line_items: :item), organization:).generate_csv + puts "=================================================================" + puts "Duration: #{(d = Time.zone.now - t).round(4)} for #{distribution_count} distributions: #{(d / distribution_count).round(4)}/dist" + File.write("dist-#{distribution_count}.csv", csv) + expect(true) + end + end + end end From b41deb016900e30c8ec0f4fd01072ebe5a06ba02 Mon Sep 17 00:00:00 2001 From: Paul Hoffer Date: Tue, 16 Jun 2026 17:25:22 -0700 Subject: [PATCH 2/5] switch to find_each --- app/services/exports/export_distributions_csv_service.rb | 2 +- .../exports/export_distributions_csv_service_spec.rb | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/services/exports/export_distributions_csv_service.rb b/app/services/exports/export_distributions_csv_service.rb index 96567555ef..05ca38c12d 100644 --- a/app/services/exports/export_distributions_csv_service.rb +++ b/app/services/exports/export_distributions_csv_service.rb @@ -32,7 +32,7 @@ def generate_csv_data csv_data = [] csv_data << base_headers + item_headers - distributions.each do |distribution| + distributions.find_each do |distribution| csv_data << build_row_data(distribution) end diff --git a/spec/services/exports/export_distributions_csv_service_spec.rb b/spec/services/exports/export_distributions_csv_service_spec.rb index 29fcac9822..6b3f6d48b5 100644 --- a/spec/services/exports/export_distributions_csv_service_spec.rb +++ b/spec/services/exports/export_distributions_csv_service_spec.rb @@ -4,9 +4,10 @@ let(:partner) { create(:partner, name: "first partner", email: "firstpartner@gmail.com", notes: "just a note.", organization_id: organization.id) } let(:include_in_kind_values) { false } let(:include_packages) { false } + let(:distributions_query) { Distribution.where(id: distributions.map(&:id)).includes(:partner, :storage_location, line_items: :item) } # match the query design used in DistributionsController and PartnersController describe '#generate_csv' do - subject { described_class.new(distributions: distributions, organization: organization, filters: filters).generate_csv } + subject { described_class.new(distributions: distributions_query, organization: organization, filters: filters).generate_csv } let(:duplicate_item) { create(:item, name: "Dupe Item", value_in_cents: 300, organization: organization, package_size: 2) } @@ -174,7 +175,7 @@ end context 'when there are no distributions but the report is requested' do - subject { described_class.new(distributions: [], organization: organization, filters: filters).generate_csv } + subject { described_class.new(distributions: Distribution.none, organization: organization, filters: filters).generate_csv } it 'returns a csv with only headers and no rows' do csv = <<~CSV Partner,Initial Allocation,Scheduled for,Source Inventory,Total Number of #{item_name},Total Value of #{item_name},Delivery Method,Shipping Cost,Status,Agency Representative,Comments,Reporting Category,Dupe Item From dcf3059b8d48728a05ab38e56ccce8eb2be35b6c Mon Sep 17 00:00:00 2001 From: Paul Hoffer Date: Tue, 16 Jun 2026 19:05:42 -0700 Subject: [PATCH 3/5] pre-group line_items instead of nested iteration --- app/services/exports/export_distributions_csv_service.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/services/exports/export_distributions_csv_service.rb b/app/services/exports/export_distributions_csv_service.rb index 05ca38c12d..aaadb9664a 100644 --- a/app/services/exports/export_distributions_csv_service.rb +++ b/app/services/exports/export_distributions_csv_service.rb @@ -149,10 +149,9 @@ def item_headers def build_row_data(distribution) row = base_table.values.map { |closure| closure.call(distribution) } + grouped_line_items = distribution.line_items.group_by(&:name) item_names.each do |item_name| - # We are doing this in-memory so that we can use the already-loaded line item records - line_items = distribution.line_items.select { |item| item.name == item_name } - + line_items = grouped_line_items.fetch(item_name, []) row << line_items.sum(&:quantity) row << Money.new(line_items.sum(&:value_per_line_item)) if @organization.include_in_kind_values_in_exported_files row << line_items.map(&:has_packages).compact.sum.round(2) if @organization.include_packages_in_distribution_export From 439ff17732eb60482028bc8eca3efe43a36b6e9c Mon Sep 17 00:00:00 2001 From: Paul Hoffer Date: Tue, 16 Jun 2026 19:31:41 -0700 Subject: [PATCH 4/5] re-use array for missing line item --- .../export_distributions_csv_service.rb | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/app/services/exports/export_distributions_csv_service.rb b/app/services/exports/export_distributions_csv_service.rb index aaadb9664a..7504059a14 100644 --- a/app/services/exports/export_distributions_csv_service.rb +++ b/app/services/exports/export_distributions_csv_service.rb @@ -147,17 +147,26 @@ def item_headers end def build_row_data(distribution) - row = base_table.values.map { |closure| closure.call(distribution) } + base_row = base_table.values.map { |closure| closure.call(distribution) } grouped_line_items = distribution.line_items.group_by(&:name) - item_names.each do |item_name| - line_items = grouped_line_items.fetch(item_name, []) - row << line_items.sum(&:quantity) - row << Money.new(line_items.sum(&:value_per_line_item)) if @organization.include_in_kind_values_in_exported_files - row << line_items.map(&:has_packages).compact.sum.round(2) if @organization.include_packages_in_distribution_export + base_row + item_names.flat_map do |item_name| + line_items = grouped_line_items[item_name] + next missing_item unless line_items + [ + line_items.sum(&:quantity), + (Money.new(line_items.sum(&:value_per_line_item)) if @organization.include_in_kind_values_in_exported_files), + (line_items.map(&:has_packages).compact.sum.round(2) if @organization.include_packages_in_distribution_export) + ].compact end + end - row + def missing_item + @missing_item ||= [ + 0, + ("0.00".freeze if @organization.include_in_kind_values_in_exported_files), + (0 if @organization.include_packages_in_distribution_export) + ].compact end end end From 664e292b2267f65f5901a4562a393f0588b4aa95 Mon Sep 17 00:00:00 2001 From: Paul Hoffer Date: Tue, 16 Jun 2026 21:52:51 -0700 Subject: [PATCH 5/5] update comment --- app/services/exports/export_distributions_csv_service.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/services/exports/export_distributions_csv_service.rb b/app/services/exports/export_distributions_csv_service.rb index 7504059a14..bbac1a199f 100644 --- a/app/services/exports/export_distributions_csv_service.rb +++ b/app/services/exports/export_distributions_csv_service.rb @@ -8,10 +8,10 @@ class ExportDistributionsCSVService include ItemsHelper def initialize(distributions:, organization:, filters: {}) - # Currently, the @distributions are already loaded by the controllers that are delegating exporting - # to this service object; this is happening within the same request/response cycle, so it's already - # in memory, so we can pass that collection in directly. Should this be moved to a background / async - # job, we will need to pass in a collection of IDs instead. + # Currently, `distributions` is a ActiveRecord::Relation from the controllers that are delegating exporting + # to this service object; this is happening within the same request/response cycle, so we pass the query in + # directly. Should this be moved to a background / async job, we will need to pass in a collection of IDs instead. + # The distribution model objects are not loaded until the `#find_each` in `generate_csv_data` # Also, adding in a `filters` parameter to make the filters that have been used available to this # service object. @distributions = distributions