Skip to content

Fix the issue where only one error is displayed, even when multiple errors are present. #4911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/events/event_types/event_storage_location.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def reduce_inventory(item_id, quantity, validate: true)
if validate
current_quantity = items[item_id]&.quantity || 0
if current_quantity < quantity
raise InventoryError.new("Could not reduce quantity by #{quantity} - current quantity is #{current_quantity}",
raise InventoryActionError.new("Could not reduce quantity by #{quantity} - current quantity is #{current_quantity}",
item_id,
id)
end
Expand Down
15 changes: 15 additions & 0 deletions app/events/inventory_action_error.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
class InventoryActionError < StandardError
# @return [Integer]
attr_accessor :item_id
# @return [Integer]
attr_accessor :storage_location_id

# @param message [String]
# @param item_id [Integer]
# @param storage_location_id [Integer]
def initialize(message, item_id, storage_location_id)
super(message)
self.item_id = item_id
self.storage_location_id = storage_location_id
end
end
28 changes: 24 additions & 4 deletions app/events/inventory_aggregate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,29 +66,36 @@ def handle(event, inventory, validate: false, previous_event: nil)
# @param validate [Boolean]
# @param previous_event [Event]
def handle_inventory_event(payload, inventory, validate: true, previous_event: nil)
errors = []
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,
move_item(inventory: inventory,
item_id: line_item.item_id,
quantity: quantity,
from_location: line_item.from_storage_location,
to_location: line_item.to_storage_location,
validate: validate)
validate: validate,
errors: errors)
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,
move_item(inventory: inventory,
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)
validate: validate,
errors: errors)
end
end

raise InventoryError.new(errors.map(&:message).join("\n")) unless errors.empty?
end

# @param payload [EventTypes::InventoryPayload]
Expand All @@ -100,6 +107,19 @@ def handle_audit_event(payload, inventory)
location: line_item.to_storage_location)
end
end

def move_item(inventory:, item_id:, quantity:, from_location:, to_location:, validate:, errors:)
inventory.move_item(item_id: item_id,
quantity: quantity,
from_location: from_location,
to_location: to_location,
validate: validate)
rescue InventoryActionError => e
item = Item.find_by(id: e.item_id)&.name || "Item ID #{e.item_id}"
loc = StorageLocation.find_by(id: e.storage_location_id)&.name || "Storage Location ID #{e.storage_location_id}"
e.message << " for #{item} in #{loc}"
errors.push(e)
end
end

# ignore previous events
Expand Down
12 changes: 2 additions & 10 deletions app/events/inventory_error.rb
Original file line number Diff line number Diff line change
@@ -1,17 +1,9 @@
class InventoryError < StandardError
# @return [Event]
attr_accessor :event
# @return [Integer]
attr_accessor :item_id
# @return [Integer]
attr_accessor :storage_location_id

# @param message [String]
# @param item_id [Integer]
# @param storage_location_id [Integer]
def initialize(message, item_id, storage_location_id)
super(message)
self.item_id = item_id
self.storage_location_id = storage_location_id
def initialize(message)
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 probably just remove this definition entirely since it does nothing but call super.

super
end
end
3 changes: 0 additions & 3 deletions app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,6 @@ def self.most_recent_snapshot(organization_id)
def validate_inventory
InventoryAggregate.inventory_for(organization_id, validate: true)
rescue InventoryError => e
item = Item.find_by(id: e.item_id)&.name || "Item ID #{e.item_id}"
loc = StorageLocation.find_by(id: e.storage_location_id)&.name || "Storage Location ID #{e.storage_location_id}"
e.message << " for #{item} in #{loc}"
if e.event != self
e.message.prepend("Error occurred when re-running events: #{e.event.type} on #{e.event.created_at.to_date}: ")
e.message << " Please contact the Human Essentials admin staff for assistance."
Expand Down
19 changes: 17 additions & 2 deletions spec/events/inventory_aggregate_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -716,20 +716,35 @@
end

describe "validation" do
let(:donation) { FactoryBot.create(:donation, organization: organization, storage_location: storage_location1) }
let(:distribution) { FactoryBot.create(:distribution, organization: organization, storage_location: storage_location1) }
context "current event is incorrect" do
it "should raise a bare error" do
donation = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation.line_items << build(:line_item, quantity: 50, item: item1)
DonationEvent.publish(donation)

distribution = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location1)
distribution.line_items << build(:line_item, quantity: 100, item: item1, itemizable: distribution)
expect { DistributionEvent.publish(distribution) }.to raise_error do |e|
expect(e).to be_a(InventoryError)
expect(e.event).to be_a(DistributionEvent)
expect(e.message).to eq("Could not reduce quantity by 100 - current quantity is 50 for #{item1.name} in #{storage_location1.name}")
end
end

context "while there are multiple errors" do
it "should show all the errors" do
donation.line_items << build(:line_item, quantity: 50, item: item1) << build(:line_item, quantity: 50, item: item2)
DonationEvent.publish(donation)

distribution.line_items << build(:line_item, quantity: 100, item: item1) << build(:line_item, quantity: 100, item: item2)
expect { DistributionEvent.publish(distribution) }.to raise_error do |e|
expect(e).to be_a(InventoryError)
expect(e.event).to be_a(DistributionEvent)
expect(e.message).to eq("Could not reduce quantity by 100 - current quantity is 50 for #{item1.name} in #{storage_location1.name}\n" \
"Could not reduce quantity by 100 - current quantity is 50 for #{item2.name} in #{storage_location1.name}")
end
end
end
end

context "subsequent event is incorrect" do
Expand Down
Loading