Improve product drive index page performance #4881
Conversation
coalest
force-pushed
the
improve-product-drive-index-page-performance
branch
from
a34bf61 to
596f5a6
Compare
|
Seems identical in light user experience testing |
|
Link to previous PR: #4845 |
| @@ -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, | |||
There was a problem hiding this comment.
Is there a reason we're using that date range to get the initial list and then passing it again into the service?
There was a problem hiding this comment.
It's a little confusing I know. We are doing two things with this date range:
- We only want to show product drives with the given date range.
- 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.
There was a problem hiding this comment.
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 = item_category_id | ||
| end | ||
|
|
||
| def call |
There was a problem hiding this comment.
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).
| # 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>>] |
There was a problem hiding this comment.
Can we use a Data object instead of a Hash?
It seems like this is trying to do two things:
- Calculate the results
- 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.
There was a problem hiding this comment.
Refactored the class a bit. Now returns a hash of ids mapped to data summary objects. Let me know what you think.
…ve-index-page-performance
Doesn't resolve any ticket.
Description
The product drives index page was mentioned to be slow in production.
Turns out it we were running two aggregate queries for every product drive (for the item quantity and number of unique items).
So listing 10 product drives meant 20 queries just to calculate the totals.
Also this page isn't paginated, so that makes the issue worse. (Although it looks like it used to be?)
Type of change
How Has This Been Tested?
Passes test suite and tested locally.
Screenshots
The performance beforehand scales linearly with number of product drives, so the more i add the worse it got.
These screenshots are when ~10 product drives were being returned.
Before:
After:
