[BUG] Storage Location Export doesn't format product quantity columns for each storage location properly.

[cielf]

Summary

New storage location does not work properly in the storage location export

Why fix?

Results given to user are wrong

Details

a bank user reported that the storage location export was not formatted properly.
I checked it out by adding two new items, and adding one each to the two existing storage locations. That seems to work fine.
Then I added a new storage location, and a new item, adding it only to the new storage location.

The info for that item appeared in the column for the first item, rather than being added.

This is consistent with what the user saw, and it looks like you don't need to add a brand new item -- the info for all the items for the new storage location end up in the leftmost columns.

The desired behaviour is that the storage location export works properly for all storage locations, whether they are recently added or not.

Criteria for completion

• storage location export works properly for all storage locations, including new ones, and ones that were added recently but don't currently work
• tests to demonstrate that it works properly for new storage locations
• Note on the PR that the team should check out lincolnhygienenetwork.org (who experienced the issue)

[atbalaji]

Hi @cielf , Is this issue still valid. When I exported using new storage location the results were proper in their respective columns. If this is still relevant, I would like to look at this issue.

[cielf]

I just checked it on staging, and they are not appearing correctly.

Someone (perhaps you?) had added a new test storage location, so I used it.

I added a new item

I then put in a purchase of that new item, using the test storage location.

When exporting the storage locations, if you then look at that storage location, which does not have all the items, but should have the last one you added, the values for the item that was just entered are not in the correct column, but are all the way over to the left.

You wouldn't see the effect if you just used the first couple of items in the list.

But yes, please do look at this issue. I'm assigning you.

[github-actions]

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

[github-actions]

Automatically unassigned after 7 days of inactivity.

[danielabar]

I'd like to attempt this one. Just trying to reproduce it now.

Is there a user guide? I was trying to figure out how to add an item to a storage location.

[cielf]

There is a youtube series covering how to set up a bank. https://www.youtube.com/watch?v=w53iaJtlQUo
[Edit: it's a little out of date, but still pretty good.]

Steps to add an item:
sign in as [email protected]
Inventory | Items & Inventory
New Item
Fill in the mandatory info and save
Then you can add some items to a storage location by recording a donation:
Donations | New Donation
-- I would just use a Misc Donation for the source in this case, fwiw.
Save

Then you should see the item under the storage location you chose.

[danielabar]

I'm able to reproduce the issue, by starting from a seeded/fixture dev db that has two storage locations with many items, then adding a new storage location, adding one new item to the new storage location.

What's interesting is that when I add the new item to the new storage location, it also adds the new item with a 0 count to all the existing storage locations. However, the new storage location only has the new item. I think this is where the problem starts.

There's a kind of asymmetry in that a new storage location only has items that were explicitly added, but existing storage locations get items added even though the user didn't explicitly add them (with a 0 count).

In app/controllers/storage_locations_controller.rb#index, for the csv format, it builds an array of arrays active_inventory_item_names containing unique item names for each storage location.

For the two existing storage locations, the lists contain all the item names including my new item name, but for the new storage location, it only has my new item, something like this:

[
  ["Adult Briefs", "Bed Pads", ..., "TestItem1", ...],
  ["Adult Briefs", "Bed Pads", ..., "TestItem1", ...],
  ["TestItem1"]
]

When this gets passed to the csv generator, it lines up the quantity for TestItem1 in the same column with Adult Briefs and so the csv is incorrect.

Potential solutions?

1. When a new storage location is created, have it automatically add all items with 0 count (but could be slow and maybe noisy for the user to look at, especially if many of those items may never be stored at that location). But it would ensure the items always "line up".
2. Update the generation of active_inventory_item_names in the controller to generate empty strings for storage locations that don't have the exact same items so that the item names in the arrays always "line up". But could get tricky - what if each storage location has a completely different assortment or only slightly overlapping list of items? Is it expected that usually different storage locations have mostly the same items?
3. Update the generate_csv method in the concern app/models/concerns/exportable.rb to handle this (maybe a little riskier if that's widely used).
4. Change the csv output so that it doesn't rely on item names lining up. But if users have built any scripts that further process the data, that could break. For example
   LocationName1
   item,quantity
   item,quantity
   ...
   LocationName2
   item,quantity
   item,quantity
   ...

[cielf]

Hmmm. That might be a side effect of the fact that we are always starting with a new seed when we're working on local.

[cielf]

I can think of cases where different storage locations would have different items --- kits might be made up specifically for one grant that is applicable to one area -- and so the storage location closest to that area would likely have those kits, but other storage locations wouldn't.

[cielf]

We'd want to have a solution at the end of the day that has a column for each item, including ones that not all the storage locations have. We'll want a stable solution that's not going to change format unless they add new items.

[cielf]

I think (without confirming) that the generate_csv in exportable is used several times -- at least for donations, purchases, distributions, storage locations, and I think also donation sites, product drives, items, and requests.

[cielf]

I think we recently made a change to the distributions export so that its item list is consistent no matter what timespan is used -- might be worth checking what we did there.

[cielf]

The problem with 1. would be that there may be storage locations out there without all the items that some other SL has -- this is fairly likely because of a pattern we see of having a warehouse / head office that has everything, but also some satellite storage locations, which may be smaller and not have the more esoteric items.

[danielabar]

Took a look at the distribution export, that uses app/services/exports/export_distributions_csv_service.rb instead of generate_csv from the exportable concern. The key to it seems to be that it gets all items for the organization:

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

These get indexed, then when building that part of the csv, initializes a row with all 0 values, then populates the item.quantity if applicable in that index/column.

I could create something similar like app/services/exports/storage_location_csv_service.rb, then modify the StorageLocation controller to use the service?

But actually, StorageLocation controller has two different methods for exporting a csv, depending on a feature toggle:

If read_events on: StorageLocation.generate_csv_from_inventory(@storage_locations, @inventory)
Otherwise: StorageLocation.generate_csv(@storage_locations, active_inventory_item_names)

My earlier analysis was on generate_csv (feature flag not on for local dev?).

Just for the experiment, I modified the Event.read_events? method to return true (not sure how to setup feature toggle locally), and in this case, the generated csv does seem correct. It shows 0's for all the items that the new storage location doesn't have, and the new item quantity in the correct column.

Aside from the fact that StorageLocations.generate_csv_from_inventory doesn't have the bug, what's the difference in behaviour for read_events toggle? (wondering if this method works correctly, could it always be used?)

[cielf]

WE are in the middle of a reworking of how we calculate and show the inventory -- changing to event sourcing (I can point you at the wiki on this if you like) -- read_events is a flipper flag that controls that. We're probably a couple of months out from the final release of that.

As for how you toggle read_events locally -- it's a flipper feature, so you sign in to flipper at localhost:3000/flipper using username: admin and password: password, add the feature_flag read_events, and then fully enable it.

[cielf]

The question is, then, whether, if it works with read_events on, should we go ahead with the change, or just let it be until the read_events is on for everyone.... I'll add checking the behaviour with read_events on to my task list for tomorrow --

[danielabar]

Just an observation, I wondered why there wasn't a test failing due to this bug. Turns out there is a test for the storage locations csv export spec/requests/storage_locations_requests_spec.rb in csv context it "includes headers followed by alphabetized item names" do

But, it tests the case where there are two storage locations with overlapping items, and it only verifies the header.

Just for the experiment I updated the test to add another item and storage location, and only place that item in that additional storage location. Then inspected the response.body (loaded into Libre Office as csv for easier reading), and indeed, the bug can be seen in the test output:

Also noticed the other cells where that item is not at that location are empty, not sure if that should be 0? (possibly a separate bug?).

[cielf]

Ok... we should have a test that shows that it works properly for the 0 case, IMO. Why we don't -- an error in judgement by a past reviewer, I expect.

But/and -- I confirmed that it does work properly under read_events.

The entire planning team is MIA for various reasons today, so I don't have a good estimate of when read_events is going in -- My best guess is about a month.

With that in mind, it does seem like a bit of wasted effort if the fix without read_events is complicated. Do you want to just put in the test for the 0 case under read_events?