Merge pull request #4841 from coalest/improve-distributions-by-country-report-performance

Perf: refactor DistributionsByCountry#report using raw SQL query

Author: cielf

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

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

app/services/distribution_by_county_report_service.rb

@@ -51,8 +51,8 @@
               <% @breakdown.each do |bd| %>
                 <tr>
                   <td><%= bd.name %></td>
-                  <td class="numeric"><%= number_with_delimiter(bd.num_items) %></td>
-                  <td class="numeric"><%= dollar_presentation(bd.amount) %></td>
+                  <td class="numeric"><%= number_with_delimiter(bd.quantity) %></td>
+                  <td class="numeric"><%= dollar_presentation(bd.value) %></td>
                 </tr>
               <% end %>
 

app/views/distributions_by_county/report.html.erb

@@ -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

spec/factories/partners.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