Skip to content
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

Fix event timing #3954

Merged
merged 4 commits into from
Dec 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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/adjustment_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def self.publish(adjustment)
create(
eventable: adjustment,
organization_id: adjustment.organization_id,
event_time: Time.zone.now,
event_time: adjustment.created_at,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The passed in adjustment from AdjustmentCreateService, weirdly, could be an exisiting Adjustment rather than a freshly created one. Maybe so this gets a bit weird with that case; I'm not sure if adjustment.updated_at would be better or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adjustments are never updated, just created.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK for this one, I guess AdjustmentCreateService initialize we should sometime take out the first bit where it can be handed an Adjustment -- I verified that nothing does call it with one.

data: EventTypes::InventoryPayload.new(
items: EventTypes::EventLineItem.from_line_items(adjustment.line_items, to: adjustment.storage_location_id)
)
Expand Down
2 changes: 1 addition & 1 deletion app/events/audit_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ def self.publish(audit)
create(
eventable: audit,
organization_id: audit.organization_id,
event_time: Time.zone.now,
event_time: audit.updated_at,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah -- this one uses updated_at and others use created_at for the eventable it's tracking, how did you intend which is which?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because audits have a state (they have to be finalized) and they can't be edited, so updated_at is the correct thing. For the others, they can be edited but they are always "valid", so created_at is correct.

data: EventTypes::AuditPayload.new(
storage_location_id: audit.storage_location_id,
items: EventTypes::EventLineItem.from_line_items(audit.line_items, to: audit.storage_location_id)
Expand Down
2 changes: 1 addition & 1 deletion app/events/distribution_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def self.publish(distribution)
create(
eventable: distribution,
organization_id: distribution.organization_id,
event_time: Time.zone.now,
event_time: distribution.created_at,
data: EventTypes::InventoryPayload.new(
items: EventTypes::EventLineItem.from_line_items(distribution.line_items, from: distribution.storage_location_id)
)
Expand Down
2 changes: 1 addition & 1 deletion app/events/donation_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def self.publish(donation)
create(
eventable: donation,
organization_id: donation.organization_id,
event_time: Time.zone.now,
event_time: donation.created_at,
data: EventTypes::InventoryPayload.new(
items: EventTypes::EventLineItem.from_line_items(donation.line_items, to: donation.storage_location_id)
)
Expand Down
31 changes: 20 additions & 11 deletions app/events/inventory_aggregate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,37 @@ def on(*event_types, &block)
end
end

# @param organization_id
# @param organization_id [Integer]
# @param first_event [Integer]
# @param last_event [Integer]
# @param validate [Boolean]
# @return [EventTypes::Inventory]
def inventory_for(organization_id)
def inventory_for(organization_id, first_event: nil, last_event: nil, validate: false)
events = Event.for_organization(organization_id)
if first_event
events = events.where("id >= ?", first_event)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these go off of one of the timestamps instead of id? It will probably work using id most of the time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I don't have a good use for this just yet, it's for the eventual "start from last snapshot" behavior. We can double check if once that's in.

end
if last_event
events = events.where("id <= ?", last_event)
end
inventory = EventTypes::Inventory.from(organization_id)
events.group_by { |e| [e.eventable_type, e.eventable_id] }.each do |_, event_batch|
last_event = event_batch.max_by(&:event_time)
handle(last_event, inventory)
last_grouped_event = event_batch.max_by(&:updated_at)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conceptually, will there ever be events that have a created_at != updated_at?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope.

handle(last_grouped_event, inventory, validate: validate)
end
inventory
end

# @param event [Event]
# @param inventory [Inventory]
def handle(event, inventory)
# @param validate [Boolean]
def handle(event, inventory, validate: false)
handler = @handlers[event.class]
if handler.nil?
Rails.logger.warn("No handler found for #{event.class}, skipping")
return
end
handler.call(event, inventory)
handler.call(event, inventory, validate: validate)
end

# @param payload [EventTypes::InventoryPayload]
Expand All @@ -46,7 +56,6 @@ def handle_inventory_event(payload, inventory, validate: true)

# @param payload [EventTypes::InventoryPayload]
# @param inventory [EventTypes::Inventory]
# @param validate [Boolean]
def handle_audit_event(payload, inventory)
payload.items.each do |line_item|
inventory.set_item_quantity(item_id: line_item.item_id,
Expand All @@ -59,15 +68,15 @@ def handle_audit_event(payload, inventory)
on DonationEvent, DistributionEvent, AdjustmentEvent, PurchaseEvent,
TransferEvent, DistributionDestroyEvent, DonationDestroyEvent,
PurchaseDestroyEvent, TransferDestroyEvent,
KitAllocateEvent, KitDeallocateEvent do |event, inventory|
handle_inventory_event(event.data, inventory, validate: false)
KitAllocateEvent, KitDeallocateEvent do |event, inventory, validate: false|
handle_inventory_event(event.data, inventory, validate: validate)
end

on AuditEvent do |event, inventory|
on AuditEvent do |event, inventory, validate: false|
handle_audit_event(event.data, inventory)
end

on SnapshotEvent do |event, inventory|
on SnapshotEvent do |event, inventory, validate: false|
inventory.storage_locations.clear
inventory.storage_locations.merge!(event.data.storage_locations)
end
Expand Down
2 changes: 1 addition & 1 deletion app/events/purchase_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def self.publish(purchase)
create(
eventable: purchase,
organization_id: purchase.organization_id,
event_time: Time.zone.now,
event_time: purchase.created_at,
data: EventTypes::InventoryPayload.new(
items: EventTypes::EventLineItem.from_line_items(purchase.line_items, to: purchase.storage_location_id)
)
Expand Down
2 changes: 1 addition & 1 deletion app/events/transfer_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def self.publish(transfer)
create(
eventable: transfer,
organization_id: transfer.organization_id,
event_time: Time.zone.now,
event_time: transfer.created_at,
data: EventTypes::InventoryPayload.new(
items: EventTypes::EventLineItem.from_line_items(transfer.line_items,
from: transfer.from.id,
Expand Down
9 changes: 9 additions & 0 deletions db/migrate/20231201194409_fix_event_times.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class FixEventTimes < ActiveRecord::Migration[7.0]
def change
Event.where(type: %i(DistributionEvent DonationEvent PurchaseEvent)).find_each do |event|
next if event.eventable.nil?

event.update_attribute(:event_time, event.eventable.created_at)
end
end
end
2 changes: 1 addition & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2023_11_17_141301) do
ActiveRecord::Schema[7.0].define(version: 2023_12_01_194409) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

Expand Down
132 changes: 87 additions & 45 deletions spec/events/inventory_aggregate_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -447,50 +447,92 @@
end
end

it "should process multiple events" do
donation = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation.line_items << build(:line_item, quantity: 50, item: item1)
donation.line_items << build(:line_item, quantity: 30, item: item2)
DonationEvent.publish(donation)

donation2 = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation2.line_items << build(:line_item, quantity: 30, item: item1)
DonationEvent.publish(donation2)

donation3 = FactoryBot.create(:donation, organization: organization, storage_location: storage_location2)
donation3.line_items << build(:line_item, quantity: 50, item: item2)
DonationEvent.publish(donation3)

# correction event
donation3.line_items = [build(:line_item, quantity: 40, item: item2)]
DonationEvent.publish(donation3)

dist = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location1)
dist.line_items << build(:line_item, quantity: 10, item: item1)
DistributionEvent.publish(dist)

dist2 = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location2)
dist2.line_items << build(:line_item, quantity: 15, item: item2)
DistributionEvent.publish(dist2)

inventory = described_class.inventory_for(organization.id)
expect(inventory).to eq(EventTypes::Inventory.new(
organization_id: organization.id,
storage_locations: {
storage_location1.id => EventTypes::EventStorageLocation.new(
id: storage_location1.id,
items: {
item1.id => EventTypes::EventItem.new(item_id: item1.id, quantity: 70),
item2.id => EventTypes::EventItem.new(item_id: item2.id, quantity: 30)
}
),
storage_location2.id => EventTypes::EventStorageLocation.new(
id: storage_location2.id,
items: {
item2.id => EventTypes::EventItem.new(item_id: item2.id, quantity: 25)
}
)
}
))
describe "multiple events" do
it "should process multiple events" do
donation = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation.line_items << build(:line_item, quantity: 50, item: item1)
donation.line_items << build(:line_item, quantity: 30, item: item2)
DonationEvent.publish(donation)

donation2 = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation2.line_items << build(:line_item, quantity: 30, item: item1)
DonationEvent.publish(donation2)

donation3 = FactoryBot.create(:donation, organization: organization, storage_location: storage_location2)
donation3.line_items << build(:line_item, quantity: 50, item: item2)
DonationEvent.publish(donation3)

# correction event
donation3.line_items = [build(:line_item, quantity: 40, item: item2)]
DonationEvent.publish(donation3)

dist = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location1)
dist.line_items << build(:line_item, quantity: 10, item: item1)
DistributionEvent.publish(dist)

dist2 = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location2)
dist2.line_items << build(:line_item, quantity: 15, item: item2)
DistributionEvent.publish(dist2)

inventory = described_class.inventory_for(organization.id)
expect(inventory).to eq(EventTypes::Inventory.new(
organization_id: organization.id,
storage_locations: {
storage_location1.id => EventTypes::EventStorageLocation.new(
id: storage_location1.id,
items: {
item1.id => EventTypes::EventItem.new(item_id: item1.id, quantity: 70),
item2.id => EventTypes::EventItem.new(item_id: item2.id, quantity: 30)
}
),
storage_location2.id => EventTypes::EventStorageLocation.new(
id: storage_location2.id,
items: {
item2.id => EventTypes::EventItem.new(item_id: item2.id, quantity: 25)
}
)
}
))
end

it "should validate incorrect events" do
donation = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation.line_items << build(:line_item, quantity: 10, item: item1)
DonationEvent.publish(donation)

dist = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location1)
dist.line_items << build(:line_item, quantity: 20, item: item1)
DistributionEvent.publish(dist)

expect { described_class.inventory_for(organization.id, validate: true) }
.to raise_error("Could not reduce quantity by 20 for item #{item1.id} in storage location #{storage_location1.id} - current quantity is 10")
end

it "should handle timing correctly" do
donation = FactoryBot.create(:donation, organization: organization, storage_location: storage_location1)
donation.line_items << build(:line_item, quantity: 30, item: item1)
DonationEvent.publish(donation)

dist = FactoryBot.create(:distribution, organization: organization, storage_location: storage_location1)
dist.line_items << build(:line_item, quantity: 10, item: item1)
DistributionEvent.publish(dist)

# correction event
donation.line_items[0].quantity = 20
DonationEvent.publish(donation)

inventory = described_class.inventory_for(organization.id, validate: true)
expect(inventory).to eq(EventTypes::Inventory.new(
organization_id: organization.id,
storage_locations: {
storage_location1.id => EventTypes::EventStorageLocation.new(
id: storage_location1.id,
items: {
item1.id => EventTypes::EventItem.new(item_id: item1.id, quantity: 10)
}
)
}
))
end
end
end
Loading