diff --git a/app/controllers/distributions_by_county_controller.rb b/app/controllers/distributions_by_county_controller.rb index 715dc7256d..d1ca1ee9e7 100644 --- a/app/controllers/distributions_by_county_controller.rb +++ b/app/controllers/distributions_by_county_controller.rb @@ -4,7 +4,12 @@ class DistributionsByCountyController < ApplicationController def report setup_date_range_picker - distributions = current_organization.distributions.includes(:partner).during(helpers.selected_range) - @breakdown = DistributionByCountyReportService.new.get_breakdown(distributions) + start_date = helpers.selected_range.first.iso8601 + end_date = helpers.selected_range.last.iso8601 + @breakdown = DistributionSummaryByCountyQuery.call( + organization_id: current_organization.id, + start_date: start_date, + end_date: end_date + ) end end diff --git a/app/queries/distribution_summary_by_county_query.rb b/app/queries/distribution_summary_by_county_query.rb new file mode 100644 index 0000000000..afe9dc157d --- /dev/null +++ b/app/queries/distribution_summary_by_county_query.rb @@ -0,0 +1,90 @@ +class DistributionSummaryByCountyQuery + CountySummary = Data.define(:name, :quantity, :value) + + # No need to send comments in the query + SQL_MULTILINE_COMMENTS = /\/\*.*?\*\// + + DISTRIBUTION_BY_COUNTY_SQL = <<~SQL.squish.gsub(SQL_MULTILINE_COMMENTS, "").freeze + /* Calculate total item quantity and value per distribution */ + WITH distribution_totals AS + ( + SELECT DISTINCT d.id, + d.partner_id, + COALESCE(SUM(li.quantity) OVER (PARTITION BY d.id), 0) AS quantity, + COALESCE(SUM(COALESCE(i.value_in_cents, 0) * li.quantity) OVER (PARTITION BY d.id), 0) AS value + FROM distributions d + JOIN line_items li ON li.itemizable_id = d.id AND li.itemizable_type = 'Distribution' + JOIN items i ON i.id = li.item_id + WHERE d.issued_at BETWEEN :start_date AND :end_date + AND d.organization_id = :organization_id + GROUP BY d.id, li.id, i.id + ), + /* Match distribution totals with client share and counties. + If distribution has no associated county, set county name to "Unspecified" + and set region to ZZZ so it will be last when sorted */ + totals_by_county AS + ( + SELECT dt.id, + dt.quantity, + dt.value, + COALESCE(psa.client_share::float / 100, 1) AS percentage, + COALESCE(c.name, 'Unspecified') county_name, + COALESCE(c.region, 'ZZZ') county_region + FROM distribution_totals dt + LEFT JOIN partners p ON p.id = dt.partner_id + LEFT JOIN partner_profiles pp ON pp.partner_id = p.id + LEFT JOIN partner_served_areas psa ON psa.partner_profile_id = pp.id + LEFT JOIN counties c ON c.id = psa.county_id + UNION + /* Previous behavior was to add a row for unspecified counties + even if all distributions have an associated county */ + SELECT 0 AS id, + 0 AS quantity, + 0 AS value, + 1 AS percentage, + 'Unspecified' AS county_name, + 'ZZZ' AS county_region + ) + /* Distribution value and quantities per county share may not be whole numbers, + so we cast to an integer for rounding purposes */ + SELECT tbc.county_name AS name, + SUM((tbc.quantity * percentage)::int) AS quantity, + SUM((tbc.value * percentage)::int) AS value + FROM totals_by_county tbc + GROUP BY county_name, county_region + ORDER BY county_region ASC; + SQL + + class << self + def call(organization_id:, start_date: nil, end_date: nil) + params = { + organization_id: organization_id, + start_date: start_date || "1000-01-01", + end_date: end_date || "3000-01-01" + } + + execute(to_sql(DISTRIBUTION_BY_COUNTY_SQL, **params)).to_a.map(&to_county_summary) + end + + private + + def execute(sql) + ActiveRecord::Base.connection.execute(sql) + end + + def to_sql(query, organization_id:, start_date:, end_date:) + ActiveRecord::Base.sanitize_sql_array( + [ + query, + organization_id: organization_id, + start_date: start_date, + end_date: end_date + ] + ) + end + + def to_county_summary + ->(params) { CountySummary.new(**params) } + end + end +end diff --git a/app/services/distribution_by_county_report_service.rb b/app/services/distribution_by_county_report_service.rb deleted file mode 100644 index c20d263923..0000000000 --- a/app/services/distribution_by_county_report_service.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -class DistributionByCountyReportService - Breakdown = Struct.new(:name, :region, :num_items, :amount) - def get_breakdown(distributions) - breakdowns = {} - breakdowns["Unspecified"] = Breakdown.new("Unspecified", "ZZZ", 0, 0.00) - distributions.each do |distribution| - served_areas = distribution.partner.profile.served_areas - num_items_for_distribution = distribution.line_items.total - value_of_distribution = distribution.line_items.total_value - if served_areas.size == 0 - breakdowns["Unspecified"].num_items += num_items_for_distribution - breakdowns["Unspecified"].amount += value_of_distribution - else - served_areas.each do |served_area| - name = served_area.county.name - percentage = served_area.client_share / 100.0 - if !breakdowns[name] - breakdowns[name] = Breakdown.new(name, served_area.county.region, - (num_items_for_distribution * percentage).round(0), value_of_distribution * percentage) - else - breakdowns[name].num_items = breakdowns[name].num_items + (num_items_for_distribution * percentage).round(0) - breakdowns[name].amount = breakdowns[name].amount + value_of_distribution * percentage - end - end - end - end - - breakdown_array = breakdowns.sort_by { |k, v| [v.region, k] } - @breakdown = breakdown_array.map { |a| a[1] } - end -end diff --git a/app/views/distributions_by_county/report.html.erb b/app/views/distributions_by_county/report.html.erb index a20488ca5e..ea8f760327 100644 --- a/app/views/distributions_by_county/report.html.erb +++ b/app/views/distributions_by_county/report.html.erb @@ -51,8 +51,8 @@ <% @breakdown.each do |bd| %> <%= bd.name %> - <%= number_with_delimiter(bd.num_items) %> - <%= dollar_presentation(bd.amount) %> + <%= number_with_delimiter(bd.quantity) %> + <%= dollar_presentation(bd.value) %> <% end %> diff --git a/spec/factories/partners.rb b/spec/factories/partners.rb index 894dd6ff62..35289fc2a8 100644 --- a/spec/factories/partners.rb +++ b/spec/factories/partners.rb @@ -24,16 +24,16 @@ send_reminders { true } organization_id { Organization.try(:first).try(:id) || create(:organization).id } + transient do + without_profile { false } + end + trait :approved do status { :approved } end trait :uninvited do status { :uninvited } - - transient do - without_profile { false } - end end trait :awaiting_review do diff --git a/spec/queries/distribution_summary_by_county_query_spec.rb b/spec/queries/distribution_summary_by_county_query_spec.rb new file mode 100644 index 0000000000..ec34cf0e6f --- /dev/null +++ b/spec/queries/distribution_summary_by_county_query_spec.rb @@ -0,0 +1,67 @@ +RSpec.describe DistributionSummaryByCountyQuery do + let(:year) { Time.current.year } + let(:issued_at_last_year) { Time.current.change(year: year - 1).to_datetime } + let(:distributions) { [] } + let(:organization_id) { organization.id } + let(:start_date) { nil } + let(:end_date) { nil } + let(:params) { {organization_id:, start_date:, end_date:} } + + include_examples "distribution_by_county" + + before do + create(:storage_location, organization: organization) + end + + describe "get_breakdown" do + it "will have 100% unspecified shows if no served_areas" do + create(:distribution, :with_items, item: item_1, organization: user.organization) + breakdown = DistributionSummaryByCountyQuery.call(**params) + expect(breakdown.size).to eq(1) + expect(breakdown[0].quantity).to eq(100) + expect(breakdown[0].value).to be_within(0.01).of(105000.0) + end + + it "divides the item numbers and values according to the partner profile" do + create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_1) + breakdown = DistributionSummaryByCountyQuery.call(**params) + expect(breakdown.size).to eq(5) + expect(breakdown[4].quantity).to eq(0) + expect(breakdown[4].value).to be_within(0.01).of(0) + 3.times do |i| + expect(breakdown[i].quantity).to eq(25) + expect(breakdown[i].value).to be_within(0.01).of(26250.0) + end + end + + it "handles multiple partners with overlapping service areas properly" do + create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_1, issued_at: issued_at_present) + create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_2, issued_at: issued_at_present) + breakdown = DistributionSummaryByCountyQuery.call(**params) + num_with_45 = 0 + num_with_20 = 0 + num_with_0 = 0 + # The result will have at least 1 45 and at least 1 20, and 1 0. Anything else will be either 45 or 25 or 20 + breakdown.each do |sa| + if sa.quantity == 45 + expect(sa.value).to be_within(0.01).of(47250.0) + num_with_45 += 1 + end + + if sa.quantity == 25 + expect(sa.value).to be_within(0.01).of(26250.0) + end + if sa.quantity == 20 + expect(sa.value).to be_within(0.01).of(21000.0) + num_with_20 += 1 + end + if sa.quantity == 0 + expect(sa.value).to be_within(0.01).of(0) + end + end + expect(num_with_45).to be > 0 + expect(num_with_20).to be > 0 + expect(num_with_0).to eq 0 + end + end +end diff --git a/spec/services/distributions_by_county_report_service_spec.rb b/spec/services/distributions_by_county_report_service_spec.rb deleted file mode 100644 index 2a7a20e657..0000000000 --- a/spec/services/distributions_by_county_report_service_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -RSpec.describe DistributionByCountyReportService, type: :service do - let(:year) { Time.current.year } - let(:issued_at_last_year) { Time.current.change(year: year - 1).to_datetime } - let(:distributions) { [] } - - include_examples "distribution_by_county" - - before do - create(:storage_location, organization: organization) - end - - describe "get_breakdown" do - it "will have 100% unspecified shows if no served_areas" do - distribution_1 = create(:distribution, :with_items, item: item_1, organization: user.organization) - breakdown = DistributionByCountyReportService.new.get_breakdown([distribution_1]) - expect(breakdown.size).to eq(1) - expect(breakdown[0].num_items).to eq(100) - expect(breakdown[0].amount).to be_within(0.01).of(105000.0) - end - - it "divides the item numbers and values according to the partner profile" do - distribution_1 = create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_1) - breakdown = DistributionByCountyReportService.new.get_breakdown([distribution_1]) - expect(breakdown.size).to eq(5) - expect(breakdown[4].num_items).to eq(0) - expect(breakdown[4].amount).to be_within(0.01).of(0) - 3.times do |i| - expect(breakdown[i].num_items).to eq(25) - expect(breakdown[i].amount).to be_within(0.01).of(26250.0) - end - end - - it "handles multiple partners with overlapping service areas properly" do - distribution_p1 = create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_1, issued_at: issued_at_present) - distribution_p2 = create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_2, issued_at: issued_at_present) - breakdown = DistributionByCountyReportService.new.get_breakdown([distribution_p1, distribution_p2]) - num_with_45 = 0 - num_with_20 = 0 - num_with_0 = 0 - # The result will have at least 1 45 and at least 1 20, and 1 0. Anything else will be either 45 or 25 or 20 - breakdown.each do |sa| - if sa.num_items == 45 - expect(sa.amount).to be_within(0.01).of(47250.0) - num_with_45 += 1 - end - - if sa.num_items == 25 - expect(sa.amount).to be_within(0.01).of(26250.0) - end - if sa.num_items == 20 - expect(sa.amount).to be_within(0.01).of(21000.0) - num_with_20 += 1 - end - if sa.num_items == 0 - expect(sa.amount).to be_within(0.01).of(0) - end - end - expect(num_with_45).to be > 0 - expect(num_with_20).to be > 0 - expect(num_with_0).to eq 0 - end - end -end