Refactor after review

Author: coalest

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

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

app/views/distributions_by_county/report.html.erb

@@ -50,9 +50,9 @@
               <tbody>
               <% @breakdown.each do |bd| %>
                 <tr>
-                  <td><%= bd["name"] %></td>
-                  <td class="numeric"><%= number_with_delimiter(bd["quantity"]) %></td>
-                  <td class="numeric"><%= dollar_presentation(bd["amount"]) %></td>
+                  <td><%= bd.name %></td>
+                  <td class="numeric"><%= number_with_delimiter(bd.quantity) %></td>
+                  <td class="numeric"><%= dollar_presentation(bd.value) %></td>
                 </tr>
               <% end %>
 

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