From f99e5d184abbf540a67bf0ffc7eedbaa5de6fc99 Mon Sep 17 00:00:00 2001 From: Cory Streiff Date: Tue, 10 Dec 2024 17:21:08 +0100 Subject: [PATCH 1/6] Perf: refactor DistributionsByCountry#report using raw SQL query --- .../distributions_by_county_controller.rb | 9 ++- .../distribution_summary_by_county_query.rb | 78 +++++++++++++++++++ .../distribution_by_county_report_service.rb | 33 -------- .../distributions_by_county/report.html.erb | 6 +- ...stribution_summary_by_county_query_spec.rb | 67 ++++++++++++++++ ...ributions_by_county_report_service_spec.rb | 63 --------------- 6 files changed, 155 insertions(+), 101 deletions(-) create mode 100644 app/queries/distribution_summary_by_county_query.rb delete mode 100644 app/services/distribution_by_county_report_service.rb create mode 100644 spec/queries/distribution_summary_by_county_query_spec.rb delete mode 100644 spec/services/distributions_by_county_report_service_spec.rb diff --git a/app/controllers/distributions_by_county_controller.rb b/app/controllers/distributions_by_county_controller.rb index 715dc7256d..613a7e4b65 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.new( + organization_id: current_organization.id, + start_date: start_date, + end_date: end_date + ).call 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..f8dbc1b278 --- /dev/null +++ b/app/queries/distribution_summary_by_county_query.rb @@ -0,0 +1,78 @@ +class DistributionSummaryByCountyQuery + DISTRIBUTION_BY_COUNTY_SQL = <<~SQL.squish.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 amount + 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.amount, + 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 + JOIN partner_profiles pp ON pp.id = dt.partner_id + LEFT JOIN partner_served_areas psa ON psa.partner_profile_id = pp.partner_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 amount, + 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.amount * percentage)::int) AS amount + FROM totals_by_county tbc + GROUP BY county_name, county_region + ORDER BY county_region ASC; + SQL + + def initialize(organization_id:, start_date: nil, end_date: nil) + @organization_id = organization_id + @start_date = start_date || "1000-01-01" + @end_date = end_date || "3000-01-01" + end + + def call + execute(to_sql(DISTRIBUTION_BY_COUNTY_SQL)).to_a + end + + private + + def execute(sql) + ActiveRecord::Base.connection.execute(sql) + end + + def to_sql(query) + ActiveRecord::Base.sanitize_sql_array( + [ + query, + organization_id: @organization_id, + start_date: @start_date, + end_date: @end_date + ] + ) + 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..05d019abff 100644 --- a/app/views/distributions_by_county/report.html.erb +++ b/app/views/distributions_by_county/report.html.erb @@ -50,9 +50,9 @@ <% @breakdown.each do |bd| %> - <%= bd.name %> - <%= number_with_delimiter(bd.num_items) %> - <%= dollar_presentation(bd.amount) %> + <%= bd["name"] %> + <%= number_with_delimiter(bd["quantity"]) %> + <%= dollar_presentation(bd["amount"]) %> <% end %> 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..a2b411f8ce --- /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.new(**params).call + expect(breakdown.size).to eq(1) + expect(breakdown[0]["quantity"]).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 + create(:distribution, :with_items, item: item_1, organization: user.organization, partner: partner_1) + breakdown = DistributionSummaryByCountyQuery.new(**params).call + expect(breakdown.size).to eq(5) + expect(breakdown[4]["quantity"]).to eq(0) + expect(breakdown[4]["amount"]).to be_within(0.01).of(0) + 3.times do |i| + expect(breakdown[i]["quantity"]).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 + 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.new(**params).call + 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["amount"]).to be_within(0.01).of(47250.0) + num_with_45 += 1 + end + + if sa["quantity"] == 25 + expect(sa["amount"]).to be_within(0.01).of(26250.0) + end + if sa["quantity"] == 20 + expect(sa["amount"]).to be_within(0.01).of(21000.0) + num_with_20 += 1 + end + if sa["quantity"] == 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 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 From 245a805a608e9e0b35b66291775a97b4ee779003 Mon Sep 17 00:00:00 2001 From: Cory Streiff Date: Tue, 10 Dec 2024 18:01:27 +0100 Subject: [PATCH 2/6] Fix query spec --- .../distribution_summary_by_county_query.rb | 2 +- spec/factories/partners.rb | 8 ++-- ...stribution_summary_by_county_query_spec.rb | 44 ++++++++++++------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/app/queries/distribution_summary_by_county_query.rb b/app/queries/distribution_summary_by_county_query.rb index f8dbc1b278..2466812f0b 100644 --- a/app/queries/distribution_summary_by_county_query.rb +++ b/app/queries/distribution_summary_by_county_query.rb @@ -26,7 +26,7 @@ class DistributionSummaryByCountyQuery COALESCE(c.name, 'Unspecified') county_name, COALESCE(c.region, 'ZZZ') county_region FROM distribution_totals dt - JOIN partner_profiles pp ON pp.id = dt.partner_id + LEFT JOIN partner_profiles pp ON pp.id = dt.partner_id LEFT JOIN partner_served_areas psa ON psa.partner_profile_id = pp.partner_id LEFT JOIN counties c ON c.id = psa.county_id UNION diff --git a/spec/factories/partners.rb b/spec/factories/partners.rb index ba34f27bbe..16f484fabd 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 index a2b411f8ce..be27f0e8f4 100644 --- a/spec/queries/distribution_summary_by_county_query_spec.rb +++ b/spec/queries/distribution_summary_by_county_query_spec.rb @@ -1,21 +1,33 @@ 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:} } + let(:organization) { create(:organization, name: "Some Unique Name") } + let(:item_1) { create(:item, value_in_cents: 1050, organization: organization) } + let(:partner_1) do + create(:partner, organization:, without_profile: true) do |p| + p.profile = create(:partner_profile, partner: p, organization:) do |pp| + pp.served_areas = create_list(:partners_served_area, 4, partner_profile: pp, client_share: 25) do |sa| + sa.county = create(:county) + end + end + end + end + let(:partner_2) do + create(:partner, organization:, without_profile: true) do |p| + p.profile = create(:partner_profile, partner: p, organization:) do |pp| + pp.served_areas = create_list(:partners_served_area, 5, partner_profile: pp, client_share: 20) do |sa, i| + # create one overlapping service area + sa.county = i.zero? ? partner_1.profile.served_areas[0].county : create(:county) + end + end + end + end - include_examples "distribution_by_county" + let(:now) { Time.current.to_datetime } - before do - create(:storage_location, organization: organization) - end + let(:params) { {organization_id: organization.id, start_date: nil, end_date: nil} } - describe "get_breakdown" do + describe "call" do it "will have 100% unspecified shows if no served_areas" do - create(:distribution, :with_items, item: item_1, organization: user.organization) + create(:distribution, :with_items, item: item_1, organization: organization) breakdown = DistributionSummaryByCountyQuery.new(**params).call expect(breakdown.size).to eq(1) expect(breakdown[0]["quantity"]).to eq(100) @@ -23,7 +35,7 @@ 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) + create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_1) breakdown = DistributionSummaryByCountyQuery.new(**params).call expect(breakdown.size).to eq(5) expect(breakdown[4]["quantity"]).to eq(0) @@ -35,8 +47,8 @@ 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) + create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_1, issued_at: now) + create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_2, issued_at: now) breakdown = DistributionSummaryByCountyQuery.new(**params).call num_with_45 = 0 num_with_20 = 0 From c86f218e7e0eff7c8fc27da4c798836586e6175b Mon Sep 17 00:00:00 2001 From: Cory Streiff Date: Wed, 11 Dec 2024 11:40:45 +0100 Subject: [PATCH 3/6] Refactor after review --- .../distributions_by_county_controller.rb | 4 +- .../distribution_summary_by_county_query.rb | 63 +++++++++++-------- .../distributions_by_county/report.html.erb | 6 +- ...stribution_summary_by_county_query_spec.rb | 34 +++++----- 4 files changed, 59 insertions(+), 48 deletions(-) diff --git a/app/controllers/distributions_by_county_controller.rb b/app/controllers/distributions_by_county_controller.rb index 613a7e4b65..d1ca1ee9e7 100644 --- a/app/controllers/distributions_by_county_controller.rb +++ b/app/controllers/distributions_by_county_controller.rb @@ -6,10 +6,10 @@ def report setup_date_range_picker start_date = helpers.selected_range.first.iso8601 end_date = helpers.selected_range.last.iso8601 - @breakdown = DistributionSummaryByCountyQuery.new( + @breakdown = DistributionSummaryByCountyQuery.call( organization_id: current_organization.id, start_date: start_date, end_date: end_date - ).call + ) end end diff --git a/app/queries/distribution_summary_by_county_query.rb b/app/queries/distribution_summary_by_county_query.rb index 2466812f0b..d883fb21b5 100644 --- a/app/queries/distribution_summary_by_county_query.rb +++ b/app/queries/distribution_summary_by_county_query.rb @@ -1,12 +1,17 @@ class DistributionSummaryByCountyQuery - DISTRIBUTION_BY_COUNTY_SQL = <<~SQL.squish.freeze + 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 amount + 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 @@ -21,7 +26,7 @@ class DistributionSummaryByCountyQuery ( SELECT dt.id, dt.quantity, - dt.amount, + dt.value, COALESCE(psa.client_share::float / 100, 1) AS percentage, COALESCE(c.name, 'Unspecified') county_name, COALESCE(c.region, 'ZZZ') county_region @@ -34,7 +39,7 @@ class DistributionSummaryByCountyQuery even if all distributions have an associated county */ SELECT 0 AS id, 0 AS quantity, - 0 AS amount, + 0 AS value, 1 AS percentage, 'Unspecified' AS county_name, 'ZZZ' AS county_region @@ -43,36 +48,42 @@ class DistributionSummaryByCountyQuery so we cast to an integer for rounding purposes */ SELECT tbc.county_name AS name, SUM((tbc.quantity * percentage)::int) AS quantity, - SUM((tbc.amount * percentage)::int) AS amount + SUM((tbc.value * percentage)::int) AS value FROM totals_by_county tbc GROUP BY county_name, county_region ORDER BY county_region ASC; SQL - def initialize(organization_id:, start_date: nil, end_date: nil) - @organization_id = organization_id - @start_date = start_date || "1000-01-01" - @end_date = end_date || "3000-01-01" - end + class << self + def call(organization_id:, start_date:, end_date:) + params = { + organization_id: organization_id, + start_date: start_date || "1000-01-01", + end_date: end_date || "3000-01-01" + } - def call - execute(to_sql(DISTRIBUTION_BY_COUNTY_SQL)).to_a - end + execute(to_sql(DISTRIBUTION_BY_COUNTY_SQL, **params)).to_a.map(&to_county_summary) + end - private + private - def execute(sql) - ActiveRecord::Base.connection.execute(sql) - end + def execute(sql) + ActiveRecord::Base.connection.execute(sql) + end - def to_sql(query) - ActiveRecord::Base.sanitize_sql_array( - [ - query, - organization_id: @organization_id, - start_date: @start_date, - end_date: @end_date - ] - ) + 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/views/distributions_by_county/report.html.erb b/app/views/distributions_by_county/report.html.erb index 05d019abff..ea8f760327 100644 --- a/app/views/distributions_by_county/report.html.erb +++ b/app/views/distributions_by_county/report.html.erb @@ -50,9 +50,9 @@ <% @breakdown.each do |bd| %> - <%= bd["name"] %> - <%= number_with_delimiter(bd["quantity"]) %> - <%= dollar_presentation(bd["amount"]) %> + <%= bd.name %> + <%= number_with_delimiter(bd.quantity) %> + <%= dollar_presentation(bd.value) %> <% end %> diff --git a/spec/queries/distribution_summary_by_county_query_spec.rb b/spec/queries/distribution_summary_by_county_query_spec.rb index be27f0e8f4..0d484f5bdd 100644 --- a/spec/queries/distribution_summary_by_county_query_spec.rb +++ b/spec/queries/distribution_summary_by_county_query_spec.rb @@ -28,47 +28,47 @@ describe "call" do it "will have 100% unspecified shows if no served_areas" do create(:distribution, :with_items, item: item_1, organization: organization) - breakdown = DistributionSummaryByCountyQuery.new(**params).call + breakdown = DistributionSummaryByCountyQuery.call(**params) expect(breakdown.size).to eq(1) - expect(breakdown[0]["quantity"]).to eq(100) - expect(breakdown[0]["amount"]).to be_within(0.01).of(105000.0) + 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: organization, partner: partner_1) - breakdown = DistributionSummaryByCountyQuery.new(**params).call + breakdown = DistributionSummaryByCountyQuery.call(**params) expect(breakdown.size).to eq(5) - expect(breakdown[4]["quantity"]).to eq(0) - expect(breakdown[4]["amount"]).to be_within(0.01).of(0) + 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]["amount"]).to be_within(0.01).of(26250.0) + 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: organization, partner: partner_1, issued_at: now) create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_2, issued_at: now) - breakdown = DistributionSummaryByCountyQuery.new(**params).call + 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["amount"]).to be_within(0.01).of(47250.0) + 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["amount"]).to be_within(0.01).of(26250.0) + if sa.quantity == 25 + expect(sa.value).to be_within(0.01).of(26250.0) end - if sa["quantity"] == 20 - expect(sa["amount"]).to be_within(0.01).of(21000.0) + 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["amount"]).to be_within(0.01).of(0) + if sa.quantity == 0 + expect(sa.value).to be_within(0.01).of(0) end end expect(num_with_45).to be > 0 From 257cf8f3b189424498e591baeb04707e12a71495 Mon Sep 17 00:00:00 2001 From: Cory Streiff Date: Wed, 11 Dec 2024 12:02:38 +0100 Subject: [PATCH 4/6] Fix flaky query spec --- ...stribution_summary_by_county_query_spec.rb | 111 +++++++++--------- 1 file changed, 58 insertions(+), 53 deletions(-) diff --git a/spec/queries/distribution_summary_by_county_query_spec.rb b/spec/queries/distribution_summary_by_county_query_spec.rb index 0d484f5bdd..f0aee54cc3 100644 --- a/spec/queries/distribution_summary_by_county_query_spec.rb +++ b/spec/queries/distribution_summary_by_county_query_spec.rb @@ -1,26 +1,6 @@ RSpec.describe DistributionSummaryByCountyQuery do let(:organization) { create(:organization, name: "Some Unique Name") } let(:item_1) { create(:item, value_in_cents: 1050, organization: organization) } - let(:partner_1) do - create(:partner, organization:, without_profile: true) do |p| - p.profile = create(:partner_profile, partner: p, organization:) do |pp| - pp.served_areas = create_list(:partners_served_area, 4, partner_profile: pp, client_share: 25) do |sa| - sa.county = create(:county) - end - end - end - end - let(:partner_2) do - create(:partner, organization:, without_profile: true) do |p| - p.profile = create(:partner_profile, partner: p, organization:) do |pp| - pp.served_areas = create_list(:partners_served_area, 5, partner_profile: pp, client_share: 20) do |sa, i| - # create one overlapping service area - sa.county = i.zero? ? partner_1.profile.served_areas[0].county : create(:county) - end - end - end - end - let(:now) { Time.current.to_datetime } let(:params) { {organization_id: organization.id, start_date: nil, end_date: nil} } @@ -34,46 +14,71 @@ 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: 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) + context "with matching partner served areas" do + let!(:partner_1) do + create(:partner, organization:, without_profile: true) do |p| + p.profile = create(:partner_profile, partner: p, organization:) do |pp| + pp.served_areas = create_list(:partners_served_area, 4, partner_profile: pp, client_share: 25) do |sa| + sa.county = create(:county) + end + end + end end - end - it "handles multiple partners with overlapping service areas properly" do - create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_1, issued_at: now) - create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_2, issued_at: now) - 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 + it "divides the item numbers and values according to the partner profile" do + create(:distribution, :with_items, item: item_1, organization: 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 - 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 + context "with overlapping served areas" do + let!(:partner_2) do + create(:partner, organization:, without_profile: true) do |p| + p.profile = create(:partner_profile, partner: p, organization:) do |pp| + pp.served_areas = create_list(:partners_served_area, 5, partner_profile: pp, client_share: 20) do |sa, i| + # create one overlapping service area + sa.county = i.zero? ? partner_1.profile.served_areas[0].county : create(:county) + end + end + end end - if sa.quantity == 0 - expect(sa.value).to be_within(0.01).of(0) + + it "handles multiple partners with overlapping service areas properly" do + create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_1, issued_at: now) + create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_2, issued_at: now) + 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 - expect(num_with_45).to be > 0 - expect(num_with_20).to be > 0 - expect(num_with_0).to eq 0 end end end From 6b8d7c12026fa0a5dc99d8b053e7f139eed37f0a Mon Sep 17 00:00:00 2001 From: Cory Streiff Date: Mon, 16 Dec 2024 11:29:35 +0100 Subject: [PATCH 5/6] Fix join clause in SQL query --- app/queries/distribution_summary_by_county_query.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/queries/distribution_summary_by_county_query.rb b/app/queries/distribution_summary_by_county_query.rb index d883fb21b5..9cb1135087 100644 --- a/app/queries/distribution_summary_by_county_query.rb +++ b/app/queries/distribution_summary_by_county_query.rb @@ -31,8 +31,9 @@ class DistributionSummaryByCountyQuery COALESCE(c.name, 'Unspecified') county_name, COALESCE(c.region, 'ZZZ') county_region FROM distribution_totals dt - LEFT JOIN partner_profiles pp ON pp.id = dt.partner_id - LEFT JOIN partner_served_areas psa ON psa.partner_profile_id = pp.partner_id + 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 From f14d05d63440738ffd185c1563f3887b9955f28c Mon Sep 17 00:00:00 2001 From: Cory Streiff Date: Mon, 16 Dec 2024 12:06:00 +0100 Subject: [PATCH 6/6] Change query spec to be closer to original spec --- .../distribution_summary_by_county_query.rb | 2 +- ...stribution_summary_by_county_query_spec.rb | 111 ++++++++---------- 2 files changed, 48 insertions(+), 65 deletions(-) diff --git a/app/queries/distribution_summary_by_county_query.rb b/app/queries/distribution_summary_by_county_query.rb index 9cb1135087..afe9dc157d 100644 --- a/app/queries/distribution_summary_by_county_query.rb +++ b/app/queries/distribution_summary_by_county_query.rb @@ -56,7 +56,7 @@ class DistributionSummaryByCountyQuery SQL class << self - def call(organization_id:, start_date:, end_date:) + def call(organization_id:, start_date: nil, end_date: nil) params = { organization_id: organization_id, start_date: start_date || "1000-01-01", diff --git a/spec/queries/distribution_summary_by_county_query_spec.rb b/spec/queries/distribution_summary_by_county_query_spec.rb index f0aee54cc3..ec34cf0e6f 100644 --- a/spec/queries/distribution_summary_by_county_query_spec.rb +++ b/spec/queries/distribution_summary_by_county_query_spec.rb @@ -1,84 +1,67 @@ RSpec.describe DistributionSummaryByCountyQuery do - let(:organization) { create(:organization, name: "Some Unique Name") } - let(:item_1) { create(:item, value_in_cents: 1050, organization: organization) } - let(:now) { Time.current.to_datetime } + 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:} } - let(:params) { {organization_id: organization.id, start_date: nil, end_date: nil} } + include_examples "distribution_by_county" - describe "call" do + 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: organization) + 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 - context "with matching partner served areas" do - let!(:partner_1) do - create(:partner, organization:, without_profile: true) do |p| - p.profile = create(:partner_profile, partner: p, organization:) do |pp| - pp.served_areas = create_list(:partners_served_area, 4, partner_profile: pp, client_share: 25) do |sa| - sa.county = create(:county) - end - end - 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 "divides the item numbers and values according to the partner profile" do - create(:distribution, :with_items, item: item_1, organization: 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) + 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 - end - context "with overlapping served areas" do - let!(:partner_2) do - create(:partner, organization:, without_profile: true) do |p| - p.profile = create(:partner_profile, partner: p, organization:) do |pp| - pp.served_areas = create_list(:partners_served_area, 5, partner_profile: pp, client_share: 20) do |sa, i| - # create one overlapping service area - sa.county = i.zero? ? partner_1.profile.served_areas[0].county : create(:county) - end - end - end + if sa.quantity == 25 + expect(sa.value).to be_within(0.01).of(26250.0) end - - it "handles multiple partners with overlapping service areas properly" do - create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_1, issued_at: now) - create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_2, issued_at: now) - 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 + 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