Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve product drive index page performance #4881

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Improve product drive index page performance #4881

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
596f5a6
Perf: Refactor ProductDrive#index with service
coalest Dec 21, 2024
1bf1b63
Fix summary calculations by applying date range filter to donations
coalest Jan 13, 2025
d8f1f4e
Refactory service call to be class method
coalest Jan 13, 2025
b927a12
Refactor hash into Data object
coalest Jan 13, 2025
3c68ccb
Merge remote-tracking branch 'upstream/main' into improve-product-dri…
coalest Jan 13, 2025
c69ba55
Refactor service class
coalest Jan 13, 2025
6eadfea
Remove unneeded class instance variable
coalest Jan 13, 2025
fed367f
Merge remote-tracking branch 'upstream/main' into improve-product-dri…
coalest Feb 13, 2025
185fbbf
Remove two tags queries for csv export
coalest Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/controllers/product_drives_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ def index
setup_date_range_picker
@product_drives = current_organization
.product_drives
.includes(donations: {line_items: :item})
.class_filter(filter_params)
.within_date_range(@selected_date_range)
.order(start_date: :desc)
Expand All @@ -15,6 +14,11 @@ def index
@item_categories = current_organization.item_categories
@selected_name_filter = filter_params[:by_name]
@selected_item_category = filter_params[:by_item_category_id]
@summary = ProductDriveSummaryService.new(
product_drives: @product_drives,
within_date_range: @selected_date_range,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're using that date range to get the initial list and then passing it again into the service?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little confusing I know. We are doing two things with this date range:

  1. We only want to show product drives with the given date range.
  2. The totals are reduced by the date range filter (and the item category filter). So we do need to pass in the date range for the total calculations.

So a product drive could overlap with a given date range, but have no donations during that range, so the quanitity/value/item count would all show as zero.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I was previously incorrectly filtering by the date range on the product drives a second time in the service, which indeed didn't make any sense.

item_category_id: @selected_item_category
).call

respond_to do |format|
format.html
Expand Down
2 changes: 1 addition & 1 deletion app/models/product_drive.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class ProductDrive < ApplicationRecord

scope :by_name, ->(name_filter) { where(name: name_filter) }
scope :by_item_category_id, ->(item_category_id) {
joins(donations: {line_items: :item})
includes(donations: {line_items: :item})
.where(item: { item_category_id: item_category_id })
}

Expand Down
52 changes: 52 additions & 0 deletions app/services/product_drive_summary_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
class ProductDriveSummaryService
def initialize(product_drives:, within_date_range:, item_category_id:)
@product_drives = product_drives
@within_date_range = within_date_range
@item_category_id = item_category_id
end

def call
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we turn this into a class method rather than requiring an instance? I find it much easier to work with (e.g. for stubbing methods).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

calculate_summary
self
end

def fetch_quantity(product_drive_id)
@summary.dig(product_drive_id, :quantity)
end

def fetch_unique_item_count(product_drive_id)
@summary.dig(product_drive_id, :unique_item_count)
end

def fetch_value(product_drive_id)
@summary.dig(product_drive_id, :value)
end

private

# Returns hash of total quantity, unique item count and value per product drive
# Example return: { 1 => { quantity: 15, unique_item_count: 2, value: 100 }, ...}
#
# @return [Hash<Hash<Symbol, Integer>>]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a Data object instead of a Hash? 

It seems like this is trying to do two things:

  1. Calculate the results
  2. Work with the results that were returned

IMO the service class should just do the calculation with a class method, and return an object that you can use to interrogate the results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the class a bit. Now returns a hash of ids mapped to data summary objects. Let me know what you think.

def calculate_summary
@summary ||= begin
query = ProductDrive
.left_joins(donations: {line_items: [:item]})
.where(id: @product_drives.ids)
.within_date_range(@within_date_range)
.group("product_drives.id")
.distinct

query = query.where(items: {item_category_id: @item_category_id}) if @item_category_id.present?

query.pluck(Arel.sql(
"product_drives.id AS id,
COALESCE(SUM(line_items.quantity), 0) AS quantity,
COUNT(DISTINCT line_items.item_id) AS unique_item_count,
COALESCE(SUM(COALESCE(items.value_in_cents, 0) * line_items.quantity), 0) AS value"
)).to_h do |(id, quantity, unique_item_count, value)|
[id, {quantity:, unique_item_count:, value:}]
end
end
end
end
6 changes: 3 additions & 3 deletions app/views/product_drives/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@
<td><%= product_drive.start_date.strftime("%m-%d-%Y") %></td>
<td><%= product_drive.end_date&.strftime("%m-%d-%Y") %></td>
<td><%= is_virtual(product_drive: product_drive) %></td>
<td class="text-right"><%= product_drive.donation_quantity_by_date(selected_range, @selected_item_category) %></td>
<td class="text-right"><%= product_drive.distinct_items_count_by_date(selected_range, @selected_item_category) %></td>
<td class="text-right"><strong><%= dollar_value(product_drive.in_kind_value) %></strong></td>
<td class="text-right"><%= @summary.fetch_quantity(product_drive.id) %></td>
<td class="text-right"><%= @summary.fetch_unique_item_count(product_drive.id) %></td>
<td class="text-right"><strong><%= dollar_value(@summary.fetch_value(product_drive.id)) %></strong></td>
<td class="text-right"><%= view_button_to product_drive_path(product_drive.id) %></td>
</tr>
<% end %>
Expand Down
Loading