Fix behaviour with errors on Distribution

[cielf]

Summary

If you have an error (too many of an item) on saving a new distribution, the number of items in inventory for each item is disappearing, and only one error is showing even if you have multiple overages. Fix it!

Why?

Speed up process of dealing with errors. Give enough information for banks to fix all the errors in one go.

Details

To recreate this issue: sign in as [email protected] Requests View a request Click Fulfill enter a distribution that has two items that have a quantity that is more than is in the inventory for your chosen storage location. Save.

You should see errors for both items (you'lll only see one) The number in brackets after the item (which is the inventory level for that item/storage location) has disappeared -- it should still be there.

Criteria for completion

• [ ] errors for all item overages are shown at the same time
• [ ] the inventory level is shown for items after save
• [ ] tests to confirm the above behaviour

[Naraveni]

I would like to take it

[cielf]

Please do!

[Naraveni]

Hey, I would like suggestions from a developer on this.

def handle_inventory_event(payload, inventory, validate: true, previous_event: nil) payload.items.each do |line_item| quantity = line_item.quantity if previous_event previous_item = previous_event.data.items.find { |i| i.same_item?(line_item) } quantity -= previous_item.quantity if previous_item end inventory.move_item(item_id: line_item.item_id, quantity: quantity, from_location: line_item.from_storage_location, to_location: line_item.to_storage_location, validate: validate) end # remove the quantity from any items that are now missing previous_event&.data&.items&.each do |previous_item| new_item = payload.items.find { |i| i.same_item?(previous_item) } if new_item.nil? inventory.move_item(item_id: previous_item.item_id, quantity: previous_item.quantity, from_location: previous_item.to_storage_location, to_location: previous_item.from_storage_location, validate: validate) end end

This is the function where we can handle multiple errors of Inventory Error if raised if something regarding quantity goes wrong

on DonationEvent, DistributionEvent, AdjustmentEvent, PurchaseEvent, TransferEvent, DistributionDestroyEvent, DonationDestroyEvent, PurchaseDestroyEvent, TransferDestroyEvent, UpdateExistingEvent do |event, inventory, validate: false, previous_event: nil| handle_inventory_event(event.data, inventory, validate: validate, previous_event: previous_event) rescue InventoryError => e e.event = event raise e end

this is where the raise error is caught.

As per the issues I thought of collecting all the error through a rescue block the combining the messages and throwing standard Error. But doing that so may bring more error as few of the events are utilizing the attributes of Inventory Error to validate and raise the error.

This may take more changes or bring out more issues.

May be I am over thinking can anyone suggest if It can be dont in a more easy way.

Thank You

@cielf @

[cielf]

@dorner or @awwaiid Can you chime in on this?

[dorner]

I think rescuing those errors per event in handle_inventory_event and collating them together might be fine. Ideally you'd keep track of the first error and just keep appending to its message with any subsequent errors.

[github-actions[bot]]

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[bot]]

Automatically unassigned after 7 days of inactivity.

[cielf]

I think this might be fixed by a PR that is currently in flight #4797

[coalest]

The item quantities disappearing should be fixed by PR #4797, but not the distribution error behavior.

That part is due to the way we are currently handling inventory events. We attempt to perform the distribution on the inventory aggregate, but if an error is raised we abort and return the error rather than continuing and returning all the errors/problems with the distribution.

[cielf]

Right.

[nozomirin]

I am trying to work on this issue. When running the tests, the donation correction function seems to behave unexpectedly. Changing the storage location does not properly transfer the items to the target location. Instead of moving the donation, the function adds the donation to the target location and doubles the donation in the source location.

[cielf]

I just ran through a case of editing a donation and changing the storage location on staging (which has the main branch on it) The numbers went down in the original and up in the new location as expected.

So, more details, please?

[nozomirin]

I observed an issue with the location storage yesterday, where the source item's quantities were doubled. However, after updating the source code and setting it up again today, it appears to be working correctly now.

[nozomirin]

What error message format do you want? It currently looks like following:

Sorry, we weren't able to save the distribution.
Could not reduce quantity by 16 - current quantity is 0 for Pads in Bulk Storage Location
Could not reduce quantity by 26 - current quantity is 0 for Diaper Rash Cream/Powder in Bulk Storage Location
Could not reduce quantity by 15 - current quantity is 0 for Cloth Diapers (AIO's/Pocket) in Bulk Storage Location

[nozomirin]

InventoryError only handles one line item, but you need all errors in one. My solution is to create a new InventoryAggregateError. When the InventoryErrors are raised, they are collected and then raised as an InventoryAggregateError. However, this solution requires changes in multiple places where InventoryError is currently handled. Is this acceptable?

[cielf]

@dorner -- could you weigh in on that?

[dorner]

I'd rather change InventoryError to be the aggregate and rename the immediate single error (e.g. InventoryActionError). Only one place looks at the item on InventoryError and does anything with it. I'm assuming you'd always re-raise the aggregate error, so most places wouldn't need to change since only the Event class itself would do anything with the non-aggregate error.

[nozomirin]

I just ran through a case of editing a donation and changing the storage location on staging (which has the main branch on it) The numbers went down in the original and up in the new location as expected.

So, more details, please?

1. Record the specific line item in both locations. (menu->Inventory->Storage Locations)
2. Create a donation in location 1.
3. Change the location of the donation to location 2.
4. Location 1 gets a double donation, and location 2 gets one donation.

[cielf]

I checked on staging, and the sequence of new donation, then change the location is working as expected -- in that location 2's level of the item is original level + amount of donation, and location 1 has the original level at the end.

So main looks right to me. Was this situation before you made any changes?

[nozomirin]

Oh, sorry. It turns out the code I copied to create a function was not exactly the same as the other part. It's fine now. Sorry for wasting your time.