Skip to content

Conversation

@bsbonus
Copy link
Contributor

@bsbonus bsbonus commented May 29, 2025

…t just ones with inventory

Resolves #5202 partially

Description

Updates the Storage Location export to include all products, not just inventoried ones. This means brining in inactive products, not just ones with 0 inv.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  1. Ensure your data has at least 1 inactive product and product w/0 inventory
  2. Navigate to Inventory --> Storage Locations page
  3. Export storage location data before fix --> observe the number of columns
  4. Export storage location data after fix
  5. Observe the additional columns in the fixed version :)

Screenshots

storage_location_after_fix.csv
storage_location_before_fix.csv

def self.generate_csv_from_inventory(storage_locations, inventory)
all_items = inventory.all_items.uniq(&:item_id).sort_by(&:name)
additional_headers = all_items.map(&:name).uniq
def self.generate_csv_from_inventory(storage_locations, inventory, current_organization)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could get moved to just being an options hash, which is my general preference vs array of args. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorner -- do you have an opinion on this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use keyword arguments instead of an option hash.

@cielf cielf requested a review from dorner May 30, 2025 13:48
@cielf
Copy link
Collaborator

cielf commented May 30, 2025

FWIW, what we have now passes my functional testing.

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous comments re hardcoded CSVs apply here as well.

# Yes it's another full table scan, but it's a small dataset and product wants the exports to consistently include all items (active, inactive, etc).
# This means we have to look for inactive items or items without inventory.
# note the remapping of item.id to item_id is to enable the uniq call to happen once across the two arrays.
all_organization_items = current_organization.items.select("DISTINCT ON (LOWER(name)) items.name, items.id as item_id").order("LOWER(name) ASC")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a feature that should be built into the View::Inventory class. It's come up a few times. We should be able to specify "include non inventory items" in the all_items method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants