Perf: refactor DistributionsByCountry#report using raw SQL query

[coalest]

Doesn't resolve any issue.

Description

One of the pages that was reported to be slow in production is the distributions_by_country#report page.

I tried out two approaches:

1. Fixing the N+1's found in the controller action.
2. Rewriting the service logic in raw SQL.

Approach number 2 seems faster, but potentially less maintainable. Approach number 1 still helps dramatically though.

Request times mentioned below are just for comparison purposes (I still just have the seed data locally).

Type of change

• Internal refactor (behavior should be identical for users)

How Has This Been Tested?

• Transferred the old service specs to be query specs.
• Tested locally

Screenshots

Before any changes (~280 ms):

After fixing the N+1's using by adding associations to includes (~120 ms):

View diff of what fixing N+1's look like

diff --git a/app/controllers/distributions_by_county_controller.rb b/app/controllers/distributions_by_county_controller.rb
index 715dc725..28ab4fe5 100644
--- a/app/controllers/distributions_by_county_controller.rb
+++ b/app/controllers/distributions_by_county_controller.rb
@@ -4,7 +4,11 @@ class DistributionsByCountyController < ApplicationController

 def report
   setup_date_range_picker
-    distributions = current_organization.distributions.includes(:partner).during(helpers.selected_range)
+    distributions = current_organization
+      .distributions
+      .includes(partner: { profile: {served_areas: [:county] }})
+      .includes(line_items: [:item])
+      .during(helpers.selected_range)
   @breakdown = DistributionByCountyReportService.new.get_breakdown(distributions)
 end
end

After rewriting with raw SQL (~60 ms):

[cielf]

i should review the functionality once any changes re approach are made. (No point until then)

[dorner]

Love the approach overall!

[dorner]

Is there a reason this needs to be an instance instead of just having a class function? It's easier to test, stub and work with.

[dorner]

Also can we return a data class instead of a hash?

[coalest]

Is there a reason this needs to be an instance instead of just having a class function? It's easier to test, stub and work with.

No real reason, current queries in this folder vary (sometimes call is an instance method sometimes a class method).

Changed it to be a class method.

[coalest]

Also can we return a data class instead of a hash?

Fixed.

[dorner]

Overall looks good to me, although I'm a bit concerned at the test changes. Performance-only fixes shouldn't need to change tests - by definition the way you know it didn't break anything is when the exact same tests pass as before. Can you explain why they were changed?

[coalest]

I didn't touch the related system or request specs for this report/page. But since I moved the core logic from a service to a query object, so I figured some refactoring of the test was ok. I left the actual assertions the same, but I refactored the setup.

The old setup was including setup from elsewhere (include_examples "distribution_by_county"), and it creates some records which weren't needed (user/org_admins). I prefer writing specs where you can see all of the setup in the file rather than DRYing it up which tends to complicate things.

[dorner]

@coalest I'd really appreciate it if for this PR you could leave the tests as is so we can validate the success of the goal of this PR, which is improving performance without changing anything else.