Resolves #4771 add unicity on low inventory query items returned

[nikolaz90]

Resolves #4771

Description

This should fix repeated rows on the dashboard low inventory report.
This seems like the least drastic change to the LowInventoryQuery, though I was considering another approach with Item.where(...) but that would seem like rewriting a lot of the logic that already exists in View::Inventory and InventoryAggregate

Type of change

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

How Has This Been Tested?

This adds a couple of cases to the low_inventory_query_spec.rb file testing the length and contents of a LowInventoryQuery.call() when an Item is edited in two different storage_locations.

Screenshots

Before :

After :

[cielf]

Looks good from a functional pov. Over to @dorner for any technical comments

[dorner]

Actually I think adding a .uniq to Inventory.all_items is probably a better approach since this might be happening in other places.

[nikolaz90]

I tried this out

module View
  class Inventory
  ...
    def all_items
      @inventory.storage_locations.values.map { |loc| loc.items.values }.flatten.compact.uniq(&:item_id)
    end
  ...

and ran the specs. It looks as though the intended functionality of View::Inventory#all_items is to return one instance for every occurance in a storage_location and so allowing multiples.

  describe "#all_items" do
    it "should return all items across storage locations" do
      results = subject.all_items
      expect(results.size).to eq(5)
      expect(results.count { |i| i.storage_location_id == storage_location1.id }).to eq(3)
      expect(results.count { |i| i.storage_location_id == storage_location2.id }).to eq(2)
      expect(results.count { |i| i.item_id == item1.id }).to eq(2)
      expect(results.count { |i| i.item_id == item2.id }).to eq(2)
      expect(results.count { |i| i.item_id == item3.id }).to eq(1)
    end
  end

I know you said that this could be happening elsewhere unintentionally, but because the spec is so explicit, it feels like this is used intentionally elsewhere in the app.
Also, specs for DistributionItemizedBreakdownService and DonationItemizedBreakdownService broke.
Not sure what to do as CSV seem to be generated from these services and am unsure of the expected output.
Should I carry on with making the #all_items return unique Items and modify other parts of the app to align to that change ?
Thanks

Just for reference, here are the failures :

Failures:

  1) DistributionItemizedBreakdownService#fetch should include the break down of items distributed with onhand data
     Failure/Error: expect(subject).to eq(expected_output)

       expected: [{:below_onhand_minimum=>true, :current_onhand=>100, :distributed=>500, :name=>"A Diapers", :onhand_m...nd_minimum=>false, :current_onhand=>200, :distributed=>200, :name=>"B Diapers", :onhand_minimum=>5}]
            got: [{:below_onhand_minimum=>true, :current_onhand=>100, :distributed=>500, :name=>"A Diapers", :onhand_m...nd_minimum=>false, :current_onhand=>100, :distributed=>200, :name=>"B Diapers", :onhand_minimum=>5}]

       (compared using ==)

       Diff:
       @@ -4,7 +4,7 @@
          :name=>"A Diapers",
          :onhand_minimum=>9999},
         {:below_onhand_minimum=>false,
       -  :current_onhand=>200,
       +  :current_onhand=>100,
          :distributed=>200,
          :name=>"B Diapers",
          :onhand_minimum=>5}]

     # ./spec/services/distribution_itemized_breakdown_service_spec.rb:27:in `block (3 levels) in <top (required)>'

  2) DistributionItemizedBreakdownService#fetch_csv should output the expected output but in CSV format
     Failure/Error: expect(subject).to eq(expected_output_csv)

       expected: #<Encoding:UTF-8> "Item,Total Distribution,Total On Hand\nA Diapers,500,100\nB Diapers,200,200\n"
            got: #<Encoding:US-ASCII> "Item,Total Distribution,Total On Hand\nA Diapers,500,100\nB Diapers,200,100\n"

       (compared using ==)

       Diff:
       @@ -1,4 +1,4 @@
        Item,Total Distribution,Total On Hand
        A Diapers,500,100
       -B Diapers,200,200
       +B Diapers,200,100

     # ./spec/services/distribution_itemized_breakdown_service_spec.rb:42:in `block (3 levels) in <top (required)>'

  3) View::Inventory#all_items should return all items across storage locations
     Failure/Error: expect(results.count { |i| i.storage_location_id == storage_location2.id }).to eq(2)

       expected: 2
            got: 0

       (compared using ==)
     # ./spec/models/view/inventory_spec.rb:140:in `block (3 levels) in <top (required)>'

  4) DonationItemizedBreakdownService#fetch return the list of items donated with distributed
     Failure/Error: expect(sorted_subject).to eq(sorted_expected_output)

       expected: [{:current_onhand=>1200, :donated=>1000, :name=>"1304T Diapers"}, {:current_onhand=>600, :donated=>500, :name=>"1305T Diapers"}]
            got: [{:current_onhand=>600, :donated=>1000, :name=>"1304T Diapers"}, {:current_onhand=>600, :donated=>500, :name=>"1305T Diapers"}]

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -[{:current_onhand=>1200, :donated=>1000, :name=>"1304T Diapers"},
       +[{:current_onhand=>600, :donated=>1000, :name=>"1304T Diapers"},

     # ./spec/services/donation_itemized_breakdown_service_spec.rb:25:in `block (3 levels) in <top (required)>'

[dorner]

Ah yep, you're right. These aren't actually "items" but essentially inventory items. Technically you can rework this by referencing the item itself and item.db_item in the query, but I don't think it's worth the rework. This looks good as is.

[nikolaz90]

Ah I see. Ok thanks 👍

[github-actions]

@nikolaz90: Your PR Resolves #4771 add unicity on low inventory query items returned is part of today's Human Essentials production release: 2024.11.24.
Thank you very much for your contribution!