Exports should show all items - not just the ones that have data in the filtered list

[cielf]

Summary

Exports should show all items - not just the ones that have data in the filtered list

Why

Stability of export -- columns shouldn't change depending on the content

Details

We noticed that donations and purchases are not showing all the items, (We think this might be a regression.) Distributions appears to be correct.

Check donations export, purchase export, transfers export, storage location export, requests export

Include inactive items

You can model this on the distributions export.

Criteria for completion

• [ ] All the item-based exports show all items, not just the ones that are used within the filtered list
• [ ] automated tests to confirm.

[piyush-pawar828]

Hey @cielf , I can have a look on this but could you please navigate me to reproduce the issue?

[cielf]

Intermediate-level issues presume a certain amount of familiarity with the system, and this one includes investigating them yourself.

There are export buttons in the main screens for each of the mentioned areas. If you don't have all the items used, in some of the areas, they are not showing all the items.

[bsbonus]

@cielf was looking at this item as a next issue...conceptually it's an inconsistency in the CSV export logic.

In the case of the Distributions export ( the working one) -- we pass the current_org into the call, so it internally can fetch all items the organization has and do a distinct on it.

# app/services/exports/export_distributions_csv_service.rb
def item_headers
      return @item_headers if @item_headers

      @item_headers = @organization.items.select("DISTINCT ON (LOWER(name)) items.name").order("LOWER(name) ASC").map(&:name)
end

In the other two, we're not passing in the current_org at all, so it's just doing something like this, since it doesn't have a way to get all possible unique items for the organization.

# app/services/exports/export_donations_csv_service.rb
def item_headers
      return @item_headers if @item_headers

      item_names = Set.new

      donations.each do |donation|
        donation.line_items.each do |line_item|
          item_names.add(line_item.item.name)
        end
      end

      @item_headers = item_names.sort
end

Aside from some of the "base" headers for these exports, there is going to be a lot of common code between these 3 reports in processing the "items" headers. I'd try to extract to a common "exports" helper or something...not sure what your convention is here, so looking for a little guidance. Move directly to the "helpers" folder? Lib? Exports/"common"?

[cielf]

Pointing @dorner at this, for answering questions as the keeper of coding standards etc.

[bsbonus]

@cielf @dorner After looking closer at this...it's kinda a mess between the csvs. They all are invoked & setup differently. I'll probably file a separate PR later to standardize, since so much of this is the same. None of it is evil code, just either doing extra work or unnecessarily departing from patterns the other ones have.

One open question is what to do with the app/services/exports/export_request_service.rb export. It fundamentally treats the columns differently because of the "enable_packs" feature flag.

if Flipper.enabled?(:enable_packs)
            item.request_units.each do |unit|
              item_names << "#{item.name} - #{unit.name.pluralize}"
            end

            # It's possible that the unit is no longer valid, so we'd
            # add that individually
            if item_request.request_unit.present?
              item_names << "#{item.name} - #{item_request.request_unit.pluralize}"
            end
          end

Is this something we'd keep unique to this export, or something we'd want in the other exports? Leave this one alone?

[cielf]

Ahh -- that's a new feature (hence the flag).

Packs are specific to requests -- we allow custom units for requests, because diaper pack sizes are not standardized, and some partners think in terms of packs, rather than diapers. The banks have to report in terms of diapers, though, so by the tie we get to distributions everythng is back being expressed in the individual items

So, yes, this is something that we would keep unique to this export.

[bsbonus]

@cielf Question on how you want to handle the Storage Location export.

Today, this is exporting based off the current snapshot of the Inventory levels for the various storage locations, which makes sense.

However, any item that has 0 quantity across all Storage Locations is excluded (inactive items probably fall here too). You can repro this by just creating a new item and not assigning quantity & then doing the Storage Location export. Not the end of the world, but if the goal is consistency for item headers, it seems like a minor bug. Maybe if there is 0 quantity they don't care?

Changing this is a little gross, because the data access pattern is different. This export uses the snapshot as the source of truth and does a lot of stuff in memory vs in the DB. The other ones don't care about inventory levels, so they never need the snapshot.

Conceptually, we fix this by bringing in all items (not just inventoried ones), like the other reports, but use the inventory quantities from the snapshot. This unfortunately is adding another full table scan to get all those items, which means doing this twice -- once for inventory data, once for all item data -- and then keeping all of that in memory.

I don't think the scale of data you have is a problem, but it's not an ideal data access pattern. Do you still want this change or something like this?

See existing code below:

def index
    @inventory = View::Inventory.new(current_organization.id)
    @selected_item_category = filter_params[:containing]
    @items = StorageLocation.items_inventoried(current_organization, @inventory)
    @include_inactive_storage_locations = params[:include_inactive_storage_locations]
    @storage_locations = current_organization.storage_locations.alphabetized

   <omitted for brevity>

    respond_to do |format|
      format.html
      format.csv do
        send_data StorageLocation.generate_csv_from_inventory(@storage_locations, @inventory), filename: "StorageLocations-#{Time.zone.today}.csv"
      end
    end
  end

[cielf]

Well, the intent of the issue is to have all the exports have the same set of item columns for ease of working with them outside the system.

I would also say that this is not a frequent use export -- so it's probably not going to be a big performance issue. So my take is that yeah, we still want to have it show all the items.

@dorner -- do you have anything you want to say about this?

[dorner]

@bsbonus could you use this method?

def items_for_location(storage_location_id, include_omitted: false)

include_omitted will give you all items, even those with no inventory in the storage location.

[dorner]

Alternatively, we can add that behavior to all_items to ensure it's available where needed.

[bsbonus]

@cielf let me pick this one up.

@dorner i'll check -- it might be?

In general, this is a pretty big changeset, so I'll push this as separate PRs. Lot of duplicate code between the exports as I called out earlier, but that could be refactored later if you really want. I kinda doubt these change much, so...might not be worth it.

[bsbonus]

Donations export PR: https://github.com/rubyforgood/human-essentials/pull/5200

[bsbonus]

Purchases Export PR: https://github.com/rubyforgood/human-essentials/pull/5201

More coming soon -- don't want to drown y'all in PR reviews...

[bsbonus]

Transfer export was already happy, but added coverage for it, b/c why not. https://github.com/rubyforgood/human-essentials/pull/5202

[bsbonus]

Storage export: https://github.com/rubyforgood/human-essentials/pull/5210/files

[cielf]

This is only partially completed.

[cielf]

(Note: PRs have been put in for everything -- but the one marked as closing the issue got merged before the others)

[awwaiid]

We are in-flight on most of the tasks. Here is the statuses:

• Donations export - PR open, needs CSV fixture
• Purchase export - PR open, needs CSV fixture
• Transfers export - DONE! Bonus tests added.
• Requests export - PR open, needs some work (has some failing tests)
• Storage location export - No PR, see above comments to see if it is actionable