Adds query for all an organization's items to the CSV export service, updates spec to test new behavior

[katelovescode]

Checklist:

• I have performed a self-review of my own code,
• I have commented my code, particularly in hard-to-understand areas,

  I don't think there are any comments necessary but I'm happy to add them if the group would like me to

• I have made corresponding changes to the documentation,

  There didn't seem to be any documentation about this specific export, other than "Partners can export distribution PDFs" - happy to fill in if the group wants

• I have added tests that prove my fix is effective or that my feature works,
• New and existing unit tests pass locally with my changes ("bundle exec rake"),
• Title include "WIP" if work is in progress.

Resolves #3388

Description

• What motivated this change (if not already described in an issue)?

  See issue Include columns for all the distributions items in the distribution export #3388

• What alternative solutions did you consider?

  One discussion that was had is what order to put the columns in; we decided on "created_at" as a sort column because that way when our users are exporting after adding new items, it doesn't change the column the existing items are in.

• What are the tradeoffs for your solution?

  Larger (column-wise) CSVs, but I don't necessarily think that's a trade-off. There might be a performance hit, I did some query benchmarking and it seemed fine but it's hard to tell unless it's in production or if we have some better performance monitoring tools.

One concern I have is that Bullet is yelling about eager loading items. I looked through the stack trace and tried to investigate which includes was the culprit, but removing the likeliest ones didn't solve the issue. I know Bullet can be finicky, but I'm including the stack trace below. My theory is that it's included somewhere in a scope and we don't need it all the time, so Bullet yells on some queries that use that scope. Not sure if that's important to fix or not. Also it's referencing the GET /diaper_bank/distrbutions.csv endpoint, which by all accounts doesn't exist, so I'm a little baffled.

Item Load (15.9ms)  SELECT "items".* FROM "items" WHERE "items"."organization_id" = $1  [["organization_id", 1]]
23:11:21 web.1    |   ↳ app/services/exports/export_distributions_csv_service.rb:118:in `uniq'
23:11:21 web.1    |   Rendering text template
23:11:21 web.1    |   Rendered text template (Duration: 0.1ms | Allocations: 17)
23:11:21 web.1    | Sent data Distributions-2024-01-31.csv (1.3ms)
23:11:21 web.1    | Completed 200 OK in 526ms (Views: 1.1ms | ActiveRecord: 90.8ms | Allocations: 189078)

23:11:21 web.1    | GET /diaper_bank/distributions.csv?filters%5Bby_item_category_id%5D=&filters%5Bby_item_id%5D=&filters%5Bby_location%5D=&filters%5Bby_partner%5D=&filters%5Bby_state%5D=&filters%5Bdate_range%5D=January+30%2C+2024+-+January+30%2C+2024
23:11:21 web.1    | AVOID eager loading detected
23:11:21 web.1    |   Distribution => [:items]
23:11:21 web.1    |   Remove from your query: .includes([:items])
23:11:21 web.1    | Call stack
23:11:21 web.1    |   /Projects/human-essentials/app/services/exports/export_distributions_csv_service.rb:126:in `build_row_data'
23:11:21 web.1    |   /Projects/human-essentials/app/services/exports/export_distributions_csv_service.rb:29:in `block in generate_csv_data'
23:11:21 web.1    |   /Projects/human-essentials/app/services/exports/export_distributions_csv_service.rb:28:in `generate_csv_data'
23:11:21 web.1    |   /Projects/human-essentials/app/services/exports/export_distributions_csv_service.rb:17:in `generate_csv'
23:11:21 web.1    |   /Projects/human-essentials/app/controllers/distributions_controller.rb:69:in `block (2 levels) in index'
23:11:21 web.1    |   /Projects/human-essentials/app/controllers/distributions_controller.rb:66:in `index'

List any dependencies that are required for this change. (gems, js libraries, etc.)

N/A

Type of change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

• Ran local server and downloaded distributions to ensure it showed all columns, with 0s for items that weren't on any of the requested distributions. An easy way to reproduce is to filter to one day, and then see any columns with 0s in them.

• Also rspec tests for the behavior.

[dorner]

This isn't doing what you think it is :) It's first loading all items into memory and then sorting and mapping them. I think you wanted

@item_headers = @organization.items.order(:created_at).distinct.select([:created_at, :name]).map(&:name)

which does it purely in the database.

[dorner]

For posterity: My comment came across as condescending. A better wording would be something like: "This is doing queries in the app instead of in the database; it would be much faster to do the query this way."

[dorner]

Why was this added? It doesn't seem relevant to the PR.

[katelovescode]

Without creating a distro for the partner, the export method that's called in the csv format fails with nil distributions. Instead let me put a nil catch in the export.

[dorner]

This line can be removed.

[katelovescode]

@dorner ready for re-review, but there's a TODO with a question about how we want to handle errors if someone tries to create a CSV for a list of 0 distributions, so this is still WIP, let me know how to proceed.

[dorner]

The zero case should still be valid - we should export a CSV with the header row correctly but no rows beneath it.

[katelovescode]

The zero case should still be valid - we should export a CSV with the header row correctly but no rows beneath it.

Good call! I will fix that.

[katelovescode]

The zero case should still be valid - we should export a CSV with the header row correctly but no rows beneath it.

@dorner - should be all set, can you take a look?

[dorner]

Looks good - thanks!

[dorner]

@cielf did you want to take a look before merging?

[cielf]

Will do -- I've got a pile of reviews to go through tomorrow, I think. FYI @katlovescode -- we usually split the review tasks up so that @dorner makes sure it's technically sound, and then I do some light manual testing to make sure that it's doing what we meant it to do when we wrote up the issue - that nothing got lost in translation.

[cielf]

LGTM. Merging. Thanks @katelovescode !

[github-actions]

@katelovescode: Your PR Adds query for all an organization's items to the CSV export service, updates spec to test new behavior is part of today's Human Essentials production release: 2024.02.18.
Thank you very much for your contribution!