Fix distribution filtering by category bug and refactor for performan…

…ce (#4590) (#4806)

* Improve distribution filtering and refactor for performance (#4590)

* Minor refactor

* Refactor with service object

* Return total distribion value not value of filtered items

* Fix default date range bug

* Fix inactive_items N+1

* Fix regression in item category filter

app/controllers/distributions_controller.rb

@@ -43,30 +43,33 @@ def index
 
     @distributions = current_organization
                      .distributions
-                     .includes(:partner, :storage_location, line_items: [:item])
-                     .order('issued_at DESC')
-                     .apply_filters(filter_params.except(:date_range), helpers.selected_range)
+                     .order(issued_at: :desc)
+                     .includes(:partner, :storage_location)
+                     .class_filter(scope_filters)
     @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)
+    @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 = DistributionTotalsService.new(current_organization.distributions, scope_filters)
+    @total_value_all_distributions = @distribution_totals.total_value
+    @total_items_all_distributions = @distribution_totals.total_quantity
+    paginated_ids = @paginated_distributions.ids
+    @total_value_paginated_distributions = @distribution_totals.total_value(paginated_ids)
+    @total_items_paginated_distributions = @distribution_totals.total_quantity(paginated_ids)
     @selected_item_category = filter_params[:by_item_category_id]
     @selected_partner = filter_params[:by_partner]
     @selected_status = filter_params[:by_state]
     @selected_location = filter_params[:by_location]
     # FIXME: one of these needs to be removed but it's unclear which at this point
     @statuses = Distribution.states.transform_keys(&:humanize)
+    @distributions_with_inactive_items = @distributions.joins(:inactive_items).pluck(:id)
 
     respond_to do |format|
       format.html
       format.csv do
-        send_data Exports::ExportDistributionsCSVService.new(distributions: @distributions, organization: current_organization, filters: filter_params).generate_csv, filename: "Distributions-#{Time.zone.today}.csv"
+        send_data Exports::ExportDistributionsCSVService.new(distributions: @distributions, organization: current_organization, filters: scope_filters).generate_csv, filename: "Distributions-#{Time.zone.today}.csv"
       end
     end
   end
@@ -285,16 +288,6 @@ 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')
-  end
-
-  def total_value(distributions)
-    distributions.sum(&:value_per_itemizable)
-  end
-
   def daily_items(pick_ups)
     item_groups = LineItem.where(itemizable_type: "Distribution", itemizable_id: pick_ups.pluck(:id)).group_by(&:item_id)
     item_groups.map do |_id, items|
@@ -306,11 +299,19 @@ def daily_items(pick_ups)
     end
   end
 
+  def scope_filters
+    filter_params
+      .except(:date_range)
+      .merge(during: helpers.selected_range)
+  end
+
   helper_method \
     def filter_params
     return {} unless params.key?(:filters)
 
-    params.require(:filters).permit(:by_item_id, :by_item_category_id, :by_partner, :by_state, :by_location, :date_range)
+    params
+      .require(:filters)
+      .permit(:by_item_id, :by_item_category_id, :by_partner, :by_state, :by_location, :date_range)
   end
 
   def perform_inventory_check

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,7 +18,7 @@ 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? }
     end
@@ -94,6 +94,8 @@ def total_value
     end
 
     has_many :items, through: :line_items
+    has_many :inactive_items, -> { inactive }, through: :line_items, source: :item
+
     accepts_nested_attributes_for :line_items,
                                   allow_destroy: true,
                                   reject_if: proc { |l| l[:item_id].blank? || l[:quantity].blank? }

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",

app/models/item.rb

@@ -53,6 +53,7 @@ class Item < ApplicationRecord
   # :housing_a_kit are items which house a kit, NOT items is_in_kit
   scope :housing_a_kit, -> { 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/services/distribution_totals_service.rb

@@ -0,0 +1,65 @@
+class DistributionTotalsService
+  def initialize(distributions, filter_params)
+    @filter_params = filter_params
+    @distribution_quantities = calculate_quantities(distributions)
+    @distribution_values = calculate_values(distributions)
+  end
+
+  def total_quantity(filter_ids = [])
+    totals = filter_ids.present? ? @distribution_quantities.slice(*filter_ids) : @distribution_quantities
+    totals.sum { |_, quantity| quantity }
+  end
+
+  def total_value(filter_ids = [])
+    totals = filter_ids.present? ? @distribution_values.slice(*filter_ids) : @distribution_values
+    totals.sum { |_, value| value }
+  end
+
+  def fetch_value(id)
+    @distribution_values.fetch(id)
+  end
+
+  def fetch_quantity(id)
+    @distribution_quantities.fetch(id)
+  end
+
+  private
+
+  attr_reader :filter_params
+
+  # Returns hash of total quantity of items per distribution
+  # Quantity of items after item filtering (id/category)
+  #
+  # @return [Hash<Integer, Integer>]
+  def calculate_quantities(distributions)
+    distributions
+      .class_filter(filter_params)
+      .left_joins(line_items: [:item])
+      .group("distributions.id, line_items.id, items.id")
+      .pluck(
+        Arel.sql(
+          "distributions.id,
+          COALESCE(SUM(line_items.quantity) OVER (PARTITION BY distributions.id), 0) AS quantity"
+        )
+      )
+      .to_h
+  end
+
+  # Returns hash of total value of items per distribution WIHOUT item id/category filter
+  # Value of entire distribution (not reduced by filtered items)
+  #
+  # @return [Hash<Integer, Integer>]
+  def calculate_values(distributions)
+    Distribution
+      .where(id: distributions.class_filter(filter_params))
+      .left_joins(line_items: [:item])
+      .group("distributions.id, line_items.id, items.id")
+      .pluck(
+        Arel.sql(
+          "distributions.id,
+          COALESCE(SUM(COALESCE(items.value_in_cents, 0) * line_items.quantity) OVER (PARTITION BY distributions.id), 0) AS value"
+        )
+      )
+      .to_h
+  end
+end

app/views/distributions/_distribution_row.html.erb

@@ -5,33 +5,25 @@
   <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"><%= @distribution_totals.fetch_quantity(distribution_row.id) %></td>
+  <td class="numeric"><%= dollar_value(@distribution_totals.fetch_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>
   <td><%= distribution_row.state&.humanize %></td>
 
+  <% distribution_has_inactive_item = @distributions_with_inactive_items.include?(distribution_row.id) %>
   <td class="text-right">
     <%= view_button_to distribution_path(distribution_row) %>
-    <% if (!distribution_row.complete? && !distribution_row.future?) || current_user.has_role?(Role::ORG_ADMIN, current_organization) %>
-      <%= edit_button_to edit_distribution_path(distribution_row), enabled: !distribution_row.has_inactive_item? %>
+    <% if (!distribution_row.complete? && !distribution_row.future?) || current_user.has_cached_role?(Role::ORG_ADMIN, current_organization) %>
+      <%= edit_button_to edit_distribution_path(distribution_row), enabled: !distribution_has_inactive_item %>
     <% end %>
     <%= print_button_to print_distribution_path(distribution_row, format: :pdf) %>
     <%= delete_button_to distribution_path(distribution_row),
           { confirm: "Are you sure you want to reclaim this distribution to #{distribution_row.partner.name}?",
             text: "Reclaim",
-            icon: "undo", enabled: !distribution_row.has_inactive_item? } %>
-    <% if distribution_row.has_inactive_item? %>
+            icon: "undo", enabled: !distribution_has_inactive_item } %>
+    <% if distribution_has_inactive_item %>
       <div class="low_priority_warning">Has Inactive Items</div>
     <% end %>
   </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"
@@ -114,6 +114,7 @@
                 <!-- End Quantity -->
 
                 <th class="numeric">Total Value</th>
+
                 <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,91 @@
           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 quantity of filtered item in distribution
+          # displays total value of distribution
+          expect(item_quantity.text).to eq("10")
+          expect(item_value.text).to eq("$20.00")
+        end
+
+        it "changes the total quantity header" 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("Total Value")
+        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 quantity of filtered item in distribution
+          # displays total value of distribution
+          expect(item_quantity.text).to eq("10")
+          expect(item_value.text).to eq("$20.00")
+        end
+
+        it "changes the total quantity header" 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("Total Value")
+        end
+
+        it "doesn't show duplicate distributions" do
+          # Add another item in given category so that a JOIN clauses would produce duplicates
+          item.update(item_category: item_category_2, value_in_cents: 50)
+
+          get distributions_path, params: params
+
+          page = Nokogiri::HTML(response.body)
+          distribution_rows = page.css("table tbody tr")
+
+          expect(distribution_rows.count).to eq(1)
+        end
+      end
     end
 
     describe "POST #create" do