Improve distribution filtering and refactor for performance (rubyforg…

…ood#4590)

app/controllers/distributions_controller.rb

@@ -43,19 +43,20 @@ def index
 
     @distributions = current_organization
                      .distributions
-                     .includes(:partner, :storage_location, line_items: [:item])
-                     .order('issued_at DESC')
+                     .includes(:partner, :storage_location)
                      .apply_filters(filter_params.except(:date_range), helpers.selected_range)
-    @paginated_distributions = @distributions.page(params[:page])
-    @items = current_organization.items.alphabetized
-    @item_categories = current_organization.item_categories
-    @storage_locations = current_organization.storage_locations.active_locations.alphabetized
-    @partners = @distributions.collect(&:partner).uniq.sort_by(&:name)
+    @paginated_distributions = @distributions.order('issued_at DESC').page(params[:page])
+    @items = current_organization.items.alphabetized.select(:id, :name)
+    @item_categories = current_organization.item_categories.select(:id, :name)
+    @storage_locations = current_organization.storage_locations.active_locations.alphabetized.select(:id, :name)
+    @partners = Partner.joins(:distributions).where(distributions: @distributions).distinct.order(:name).select(:id, :name)
     @selected_item = filter_params[:by_item_id].presence
-    @total_value_all_distributions = total_value(@distributions)
-    @total_items_all_distributions = total_items(@distributions, @selected_item)
-    @total_value_paginated_distributions = total_value(@paginated_distributions)
-    @total_items_paginated_distributions = total_items(@paginated_distributions, @selected_item)
+    @distribution_totals = @distributions.unscope(:order, :includes).to_totals_hash
+    @total_value_all_distributions = total_value(@distribution_totals)
+    @total_items_all_distributions = total_quantity(@distribution_totals)
+    paginated_ids = @paginated_distributions.ids
+    @total_value_paginated_distributions = total_value(@distribution_totals.slice(*paginated_ids))
+    @total_items_paginated_distributions = total_quantity(@distribution_totals.slice(*paginated_ids))
     @selected_item_category = filter_params[:by_item_category_id]
     @selected_partner = filter_params[:by_partner]
     @selected_status = filter_params[:by_state]
@@ -285,14 +286,22 @@ def request_id
     params.dig(:distribution, :request_attributes, :id)
   end
 
-  def total_items(distributions, item)
-    query = LineItem.where(itemizable_type: "Distribution", itemizable_id: distributions.pluck(:id))
-    query = query.where(item_id: item.to_i) if item
-    query.sum('quantity')
+  helper_method :fetch_distribution_value
+  def fetch_distribution_value(id)
+    @distribution_totals.dig(id, :value)
+  end
+
+  helper_method :fetch_distribution_quantity
+  def fetch_distribution_quantity(id)
+    @distribution_totals.dig(id, :quantity)
+  end
+
+  def total_value(distribution_totals)
+    distribution_totals.sum { |id, totals| totals[:value] }
   end
 
-  def total_value(distributions)
-    distributions.sum(&:value_per_itemizable)
+  def total_quantity(distribution_totals)
+    distribution_totals.sum { |id, totals| totals[:quantity] }
   end
 
   def daily_items(pick_ups)

app/helpers/distribution_helper.rb

@@ -19,21 +19,6 @@ def hashed_calendar_path
     calendar_distributions_url(hash: crypt.encrypt_and_sign(current_organization.id))
   end
 
-  def quantity_by_item_id(distribution, item_id)
-    item_id = Integer(item_id)
-    quantities = distribution.line_items.quantities_by_name
-
-    single_item = quantities.values.find { |li| item_id == li[:item_id] } || {}
-    single_item[:quantity]
-  end
-
-  def quantity_by_item_category_id(distribution, item_category_id)
-    item_category_id = Integer(item_category_id)
-    quantities = distribution.line_items.quantities_by_category
-
-    quantities[item_category_id]
-  end
-
   def distribution_shipping_cost(shipping_cost)
     (shipping_cost && shipping_cost != 0) ? number_to_currency(shipping_cost) : ""
   end

app/models/concerns/itemizable.rb

@@ -18,9 +18,13 @@ def has_inactive_item?
       inactive_items.any?
     end
 
-    # @return [Array<Item>]
+    # @return [Array<Item>] or [Item::ActiveRecord_Relation]
     def inactive_items
-      line_items.map(&:item).select { |i| !i.active? }
+      if line_items.loaded?
+        line_items.map(&:item).select { |i| !i.active? }
+      else
+        items.inactive
+      end
     end
 
     has_many :line_items, as: :itemizable, inverse_of: :itemizable do

app/models/distribution.rb

@@ -48,9 +48,9 @@ class Distribution < ApplicationRecord
   enum delivery_method: { pick_up: 0, delivery: 1, shipped: 2 }
   scope :active, -> { joins(:line_items).joins(:items).where(items: { active: true }) }
   # add item_id scope to allow filtering distributions by item
-  scope :by_item_id, ->(item_id) { joins(:items).where(items: { id: item_id }) }
+  scope :by_item_id, ->(item_id) { includes(:items).where(items: { id: item_id }) }
   # partner scope to allow filtering by partner
-  scope :by_item_category_id, ->(item_category_id) { joins(:items).where(items: { item_category_id: item_category_id }) }
+  scope :by_item_category_id, ->(item_category_id) { includes(:items).where(items: { item_category_id: item_category_id }) }
   scope :by_partner, ->(partner_id) { where(partner_id: partner_id) }
   # location scope to allow filtering distributions by location
   scope :by_location, ->(storage_location_id) { where(storage_location_id: storage_location_id) }
@@ -65,9 +65,7 @@ class Distribution < ApplicationRecord
       .apply_filters(filters, date_range)
   }
   scope :apply_filters, ->(filters, date_range) {
-    includes(:partner, :storage_location, :line_items, :items)
-      .order(issued_at: :desc)
-      .class_filter(filters.merge(during: date_range))
+    class_filter(filters.merge(during: date_range))
   }
   scope :this_week, -> do
     where("issued_at > :start_date AND issued_at <= :end_date",
@@ -76,6 +74,24 @@ class Distribution < ApplicationRecord
 
   delegate :name, to: :partner, prefix: true
 
+  # Returns hash of total quantity and value of items per distribution
+  # Ex: {7=>{quantity: 13309, value: 43000}, 22=>{quantity: 0, value: 0}, ...)
+  #
+  # @return [Hash<Integer, Hash<Symbol, Integer>>]
+  def self.to_totals_hash
+    left_joins(line_items: [:item])
+      .group("distributions.id, line_items.id, items.id")
+      .pluck(
+        Arel.sql(
+          "distributions.id,
+          sum(line_items.quantity) OVER (PARTITION BY distributions.id) AS quantity,
+          sum(COALESCE(items.value_in_cents, 0) * line_items.quantity) OVER (PARTITION BY distributions.id) AS value"
+        )
+      ).to_h do |(id, quantity, value)|
+        [id, {quantity: quantity || 0, value: value || 0}]
+      end
+  end
+
   def distributed_at
     if is_midnight(issued_at)
       issued_at.to_fs(:distribution_date)

app/models/item.rb

@@ -53,6 +53,7 @@ class Item < ApplicationRecord
   # Add spec for these
   scope :kits, -> { where.not(kit_id: nil) }
   scope :loose, -> { where(kit_id: nil) }
+  scope :inactive, -> { where.not(active: true) }
 
   scope :visible, -> { where(visible_to_partners: true) }
   scope :alphabetized, -> { order(:name) }

app/views/distributions/_distribution_row.html.erb

@@ -5,17 +5,8 @@
   <td class="date"><%= (distribution_row.issued_at.presence || distribution_row.created_at).strftime("%m/%d/%Y") %></td>
   <td><%= distribution_row.storage_location.name %></td>
 
-  <!-- Quantity -->
-  <% if filter_params[:by_item_id].present? %>
-    <td class="numeric"><%= quantity_by_item_id(distribution_row, filter_params[:by_item_id]) %></td>
-  <% elsif filter_params[:by_item_category_id].present? %>
-    <td class="numeric"><%= quantity_by_item_category_id(distribution_row, filter_params[:by_item_category_id]) %></td>
-  <% else %>
-    <td class="numeric"><%= distribution_row.line_items.total %></td>
-  <% end %>
-  <!-- End Quantity -->
-
-  <td class="numeric"><%= dollar_value(distribution_row.value_per_itemizable) %></td>
+  <td class="numeric"><%= fetch_distribution_quantity(distribution_row.id) %></td>
+  <td class="numeric"><%= dollar_value(fetch_distribution_value(distribution_row.id)) %></td>
   <td><%= distribution_row.delivery_method.humanize %></td>
   <td><%= distribution_shipping_cost(distribution_row.shipping_cost) %></td>
   <td><%= distribution_row.comment %></td>

app/views/distributions/index.html.erb

@@ -69,7 +69,7 @@
                 <%= clear_filter_button %>
                 <span class="float-right">
                   <%=
-                    if @distributions.length > 0
+                    if @distributions.any?
                       download_button_to(
                         distributions_path(format: :csv, filters: filter_params.merge(date_range: date_range_params)),
                         text: "Export Distributions"
@@ -113,7 +113,16 @@
                 <% end %>
                 <!-- End Quantity -->
 
-                <th class="numeric">Total Value</th>
+                <!-- Value -->
+                <% if filter_params[:by_item_id].present? %>
+                  <th class="numeric">Value of <%= @items.find { |i| i.id == filter_params[:by_item_id].to_i }&.name %></th>
+                <% elsif filter_params[:by_item_category_id].present? %>
+                  <th class="numeric">Value of <%= @item_categories.find { |ic| ic.id == filter_params[:by_item_category_id].to_i }&.name %></th>
+                <% else %>
+                  <th class="numeric">Total Value</th>
+                <% end %>
+                <!-- End Value -->
+
                 <th>Delivery Method</th>
                 <th>Shipping Cost</th>
                 <th>Comments</th>

spec/requests/distributions_requests_spec.rb

@@ -62,7 +62,7 @@
     end
 
     describe "GET #index" do
-      let(:item) { create(:item, organization: organization) }
+      let(:item) { create(:item, value_in_cents: 100, organization: organization) }
       let!(:distribution) { create(:distribution, :with_items, :past, item: item, item_quantity: 10, organization: organization) }
 
       it "returns http success" do
@@ -103,6 +103,77 @@
           expect(response.body).to match(/Has Inactive Items/)
         end
       end
+
+      context "when filtering by item id" do
+        let!(:item_2) { create(:item, value_in_cents: 100, organization: organization) }
+        let(:params) { { filters: { by_item_id: item.id } } }
+
+        before do
+          distribution.line_items << create(:line_item, item: item_2, quantity: 10)
+        end
+
+        it "shows value and quantity for that item in distributions" do
+          get distributions_path, params: params
+
+          page = Nokogiri::HTML(response.body)
+          item_quantity, item_value = page.css("table tbody tr td.numeric")
+
+          # total value/quantity of distribution
+          expect(distribution.total_quantity).to eq(20)
+          expect(distribution.value_per_itemizable).to eq(2000)
+
+          # displays value/quantity of filtered item in distribution
+          expect(item_quantity.text).to eq("10")
+          expect(item_value.text).to eq("$10.00")
+        end
+
+        it "changes the total quantity/value headers" do
+          get distributions_path, params: params
+
+          page = Nokogiri::HTML(response.body)
+          item_total_header, item_value_header = page.css("table thead tr th.numeric")
+
+          expect(item_total_header.text).to eq("Total #{item.name}")
+          expect(item_value_header.text).to eq("Value of #{item.name}")
+        end
+      end
+
+      context "when filtering by item category id" do
+        let!(:item_category) { create(:item_category, organization:) }
+        let!(:item_category_2) { create(:item_category, organization:) }
+        let!(:item_2) { create(:item, item_category: item_category_2, value_in_cents: 100, organization: organization) }
+        let(:params) { { filters: { by_item_category_id: item.item_category_id } } }
+
+        before do
+          item.update(item_category: item_category)
+          distribution.line_items << create(:line_item, item: item_2, quantity: 10)
+        end
+
+        it "shows value and quantity for that item category in distributions" do
+          get distributions_path, params: params
+
+          page = Nokogiri::HTML(response.body)
+          item_quantity, item_value = page.css("table tbody tr td.numeric")
+
+          # total value/quantity of distribution
+          expect(distribution.total_quantity).to eq(20)
+          expect(distribution.value_per_itemizable).to eq(2000)
+
+          # displays value/quantity of filtered item in distribution
+          expect(item_quantity.text).to eq("10")
+          expect(item_value.text).to eq("$10.00")
+        end
+
+        it "changes the total quantity/value headers" do
+          get distributions_path, params: params
+
+          page = Nokogiri::HTML(response.body)
+          item_total_header, item_value_header = page.css("table thead tr th.numeric")
+
+          expect(item_total_header.text).to eq("Total in #{item_category.name}")
+          expect(item_value_header.text).to eq("Value of #{item_category.name}")
+        end
+      end
     end
 
     describe "POST #create" do