From 46cdbd58a5fbc74826295b4d8f11bc2fa118b570 Mon Sep 17 00:00:00 2001 From: Cory Streiff <90390502+coalest@users.noreply.github.com> Date: Tue, 10 Dec 2024 02:35:44 +0100 Subject: [PATCH] Fix distribution filtering by category bug and refactor for performance (#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 | 47 +++++----- app/helpers/distribution_helper.rb | 15 ---- app/models/concerns/itemizable.rb | 4 +- app/models/distribution.rb | 8 +- app/models/item.rb | 1 + app/services/distribution_totals_service.rb | 65 ++++++++++++++ .../distributions/_distribution_row.html.erb | 22 ++--- app/views/distributions/index.html.erb | 3 +- spec/requests/distributions_requests_spec.rb | 87 ++++++++++++++++++- 9 files changed, 191 insertions(+), 61 deletions(-) create mode 100644 app/services/distribution_totals_service.rb diff --git a/app/controllers/distributions_controller.rb b/app/controllers/distributions_controller.rb index 6a3a55146f..d2776bf981 100644 --- a/app/controllers/distributions_controller.rb +++ b/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 diff --git a/app/helpers/distribution_helper.rb b/app/helpers/distribution_helper.rb index c1fc027ee0..5ee6f2d879 100644 --- a/app/helpers/distribution_helper.rb +++ b/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 diff --git a/app/models/concerns/itemizable.rb b/app/models/concerns/itemizable.rb index 2a12b10518..004f0d3128 100644 --- a/app/models/concerns/itemizable.rb +++ b/app/models/concerns/itemizable.rb @@ -18,7 +18,7 @@ def has_inactive_item? inactive_items.any? end - # @return [Array] + # @return [Array] 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? } diff --git a/app/models/distribution.rb b/app/models/distribution.rb index 43243a32d5..1d11c25d7b 100644 --- a/app/models/distribution.rb +++ b/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", diff --git a/app/models/item.rb b/app/models/item.rb index 80059b0565..8f6b1eaec1 100644 --- a/app/models/item.rb +++ b/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) } diff --git a/app/services/distribution_totals_service.rb b/app/services/distribution_totals_service.rb new file mode 100644 index 0000000000..28ad15d58e --- /dev/null +++ b/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] + 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] + 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 diff --git a/app/views/distributions/_distribution_row.html.erb b/app/views/distributions/_distribution_row.html.erb index 6e1c99ee7b..57dbd6660c 100644 --- a/app/views/distributions/_distribution_row.html.erb +++ b/app/views/distributions/_distribution_row.html.erb @@ -5,33 +5,25 @@ <%= (distribution_row.issued_at.presence || distribution_row.created_at).strftime("%m/%d/%Y") %> <%= distribution_row.storage_location.name %> - - <% if filter_params[:by_item_id].present? %> - <%= quantity_by_item_id(distribution_row, filter_params[:by_item_id]) %> - <% elsif filter_params[:by_item_category_id].present? %> - <%= quantity_by_item_category_id(distribution_row, filter_params[:by_item_category_id]) %> - <% else %> - <%= distribution_row.line_items.total %> - <% end %> - - - <%= dollar_value(distribution_row.value_per_itemizable) %> + <%= @distribution_totals.fetch_quantity(distribution_row.id) %> + <%= dollar_value(@distribution_totals.fetch_value(distribution_row.id)) %> <%= distribution_row.delivery_method.humanize %> <%= distribution_shipping_cost(distribution_row.shipping_cost) %> <%= distribution_row.comment %> <%= distribution_row.state&.humanize %> + <% distribution_has_inactive_item = @distributions_with_inactive_items.include?(distribution_row.id) %> <%= 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 %>
Has Inactive Items
<% end %> diff --git a/app/views/distributions/index.html.erb b/app/views/distributions/index.html.erb index 42da475824..e702720aae 100644 --- a/app/views/distributions/index.html.erb +++ b/app/views/distributions/index.html.erb @@ -69,7 +69,7 @@ <%= clear_filter_button %> <%= - 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 @@ Total Value + Delivery Method Shipping Cost Comments diff --git a/spec/requests/distributions_requests_spec.rb b/spec/requests/distributions_requests_spec.rb index 54e70166c7..8ce7533ed6 100644 --- a/spec/requests/distributions_requests_spec.rb +++ b/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