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

Event Source pt. 2: Inventory Discrepancies #3917

Merged
merged 2 commits into from
Nov 5, 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
104 changes: 104 additions & 0 deletions app/events/event_differ.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
module Types
include Dry.Types()
end

module EventDiffer
# Used to indicate that a storage location exists in one source but not the other.
class LocationDiff < Dry::Struct
attribute :storage_location_id, Types::Integer
attribute :database, Types::Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if there is a different or more explicit name for this. Like... in_transactional_model or in_stateful_model or something. Food for thought

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm definitely down with brainstorming better names... the most explicit thing I could think of is "this is the thing that lives in the database" and "this is the thing that gets built via an aggregate". I'm not sure about something like "stateful" (an aggregate is state, it's just derived state) or transactional (things can live in the database without relying on transactions).

attribute :aggregate, Types::Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar for these, maybe in_event_aggregate


# @param options [Object]
# @return [Hash]
def as_json(options = nil)
super.merge(type: "location")
end
end

# Used to indicate that the quantity of an item in one source doesn't match the other.
class ItemDiff < Dry::Struct
attribute :storage_location_id, Types::Integer
attribute :item_id, Types::Integer
attribute :database, Types::Integer
attribute :aggregate, Types::Integer

# @param options [Object]
# @return [Hash]
def as_json(options = nil)
super.merge(type: "item")
end
end

class << self
# @param locations [Array<StorageLocation>]
# @param inventory [EventTypes::Inventory]
# @return [Array<LocationDiff>]
def check_location_ids(locations, inventory)
db_ids = locations.map(&:id)
inventory_ids = inventory.storage_locations.keys
diffs = []
(db_ids - inventory_ids).each do |id|
diffs.push(LocationDiff.new(storage_location_id: id, database: true, aggregate: false))
end
(inventory_ids - db_ids).each do |id|
diffs.push(LocationDiff.new(storage_location_id: id, database: false, aggregate: true))
end
diffs
end

# @param inventory_loc [EventTypes::EventStorageLocation]
# @param db_loc [StorageLocation]
# @return [Array<ItemDiff>]
def check_items(inventory_loc, db_loc)
diffs = []
diffs += check_item_ids(inventory_loc, db_loc)
db_loc.inventory_items.each do |db_item|
inventory_item = inventory_loc.items[db_item.item_id]
next if inventory_item.nil?

if inventory_item.quantity != db_item.quantity
diffs.push(ItemDiff.new(item_id: db_item.item_id,
storage_location_id: db_loc.id,
database: db_item.quantity,
aggregate: inventory_item.quantity))
end
end
diffs
end

# @param inventory_loc [EventTypes::EventStorageLocation]
# @param db_loc [StorageLocation]
# @return [Array<ItemDiff>]
def check_item_ids(inventory_loc, db_loc)
inventory_ids = inventory_loc.items.keys
db_ids = db_loc.inventory_items.map(&:item_id)
diffs = []
(db_ids - inventory_ids).each do |id|
item = db_loc.inventory_items.find { |f| f.item_id == id }
diffs.push(ItemDiff.new(item_id: id, storage_location_id: db_loc.id, database: item&.quantity, aggregate: 0))
end
(inventory_ids - db_ids).each do |id|
item = inventory_loc.items[id]
diffs.push(ItemDiff.new(item_id: id, storage_location_id: db_loc.id, database: 0, aggregate: item.quantity))
end
diffs
end

# @param inventory [EventTypes::Inventory]
# @return [Array<ItemDiff, LocationDiff>]
def check_difference(inventory)
diffs = []
org = Organization.find(inventory.organization_id)
locations = org.storage_locations.to_a
diffs += check_location_ids(locations, inventory)
locations.each do |db_loc|
inventory_loc = inventory.storage_locations[db_loc.id]
next if inventory_loc.nil?

diffs += check_items(inventory_loc, db_loc)
end
diffs
end
end
end
18 changes: 18 additions & 0 deletions app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,29 @@
# updated_at :datetime not null
# eventable_id :bigint
# organization_id :bigint
# user_id :bigint
#
class Event < ApplicationRecord
scope :for_organization, ->(organization_id) { where(organization_id: organization_id).order(:event_time) }

serialize :data, EventTypes::StructCoder.new(EventTypes::InventoryPayload)

belongs_to :eventable, polymorphic: true
belongs_to :user, optional: true

before_create do
self.user_id = PaperTrail.request&.whodunnit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, as a mechanism to get the current user, I see. Seems fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if there are other things to tie to, like a request id that goes into the logs. Other stuff to make tracing of what went wrong easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking into that... I can try to add more metadata to PaperTrail, perhaps. Otherwise you need to mess with thread variables and it gets messy. The truth is that the operation itself should give us most if not everything that we need - there are very few ways (usually one) that any individual event can be created. If we find some pain dealing with the current way, we can always add more info later.

end

after_create_commit do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is appropriate for this to immediately identify and record issues when they happen, but flagging they we have to beware of the resulting slowdown. For individual actions I don't think we'd ever notice, but we should be on the lookout for batch operations. Also once there is a diff then we'll always have one (for the org/location) and this'll find things every time unless the problems cancel each other out (a finance friend of mine calls this "The God of Offsets").

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, we definitely should monitor if there's a huge drop in efficiency. I do want to put in a followup PR that relies more heavily on snapshots (which should be done more often) which should speed this up.

And yeah I know that the diffs will keep piling on. The goal is not for this to be a permanent feature, but for us to monitor over time and verify that the events are the accurate interpretation (and to do fixes until they are). Once we're confident in that, we switch over to using them as the source of truth and the diffs no longer need to happen.

inventory = InventoryAggregate.inventory_for(organization_id)
diffs = EventDiffer.check_difference(inventory)
if diffs.any?
InventoryDiscrepancy.create!(
event_id: id,
organization_id: organization_id,
diff: diffs
)
end
end
end
15 changes: 15 additions & 0 deletions app/models/inventory_discrepancy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# == Schema Information
#
# Table name: inventory_discrepancies
#
# id :bigint not null, primary key
# diff :json
# created_at :datetime not null
# updated_at :datetime not null
# event_id :bigint not null
# organization_id :bigint not null
#
class InventoryDiscrepancy < ApplicationRecord
belongs_to :event
belongs_to :organization
end
12 changes: 12 additions & 0 deletions db/migrate/20231020193728_create_inventory_discrepancies.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class CreateInventoryDiscrepancies < ActiveRecord::Migration[7.0]
def change
create_table :inventory_discrepancies do |t|
t.references :organization, null: false, foreign_key: true
t.references :event, null: false, foreign_key: true, name: 'event_id'
t.json :diff
t.timestamps

t.index [:organization_id, :created_at]
end
end
end
7 changes: 7 additions & 0 deletions db/migrate/20231022140444_add_user_to_events.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class AddUserToEvents < ActiveRecord::Migration[7.0]
disable_ddl_transaction!

def change
add_reference :events, :user, index: { algorithm: :concurrently }
end
end
18 changes: 16 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

# This file is auto-generated from the current state of the database. Instead
# of editing this file, please use the migrations feature of Active Record to
# incrementally modify your database, and then regenerate this schema definition.
Expand All @@ -11,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

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

Expand Down Expand Up @@ -284,8 +283,10 @@
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.bigint "organization_id"
t.bigint "user_id"
t.index ["organization_id", "event_time"], name: "index_events_on_organization_id_and_event_time"
t.index ["organization_id"], name: "index_events_on_organization_id"
t.index ["user_id"], name: "index_events_on_user_id"
end

create_table "families", force: :cascade do |t|
Expand Down Expand Up @@ -329,6 +330,17 @@
t.index ["feature_key", "key", "value"], name: "index_flipper_gates_on_feature_key_and_key_and_value", unique: true
end

create_table "inventory_discrepancies", force: :cascade do |t|
t.bigint "organization_id", null: false
t.bigint "event_id", null: false
t.json "diff"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["event_id"], name: "index_inventory_discrepancies_on_event_id"
t.index ["organization_id", "created_at"], name: "index_inventory_discrepancies_on_organization_id_and_created_at"
t.index ["organization_id"], name: "index_inventory_discrepancies_on_organization_id"
end

create_table "inventory_items", id: :serial, force: :cascade do |t|
t.integer "storage_location_id"
t.integer "item_id"
Expand Down Expand Up @@ -852,6 +864,8 @@
add_foreign_key "donations", "product_drives"
add_foreign_key "donations", "storage_locations"
add_foreign_key "families", "partners"
add_foreign_key "inventory_discrepancies", "events"
add_foreign_key "inventory_discrepancies", "organizations"
add_foreign_key "item_categories", "organizations"
add_foreign_key "item_categories_partner_groups", "item_categories"
add_foreign_key "item_categories_partner_groups", "partner_groups"
Expand Down
75 changes: 75 additions & 0 deletions spec/events/event_differ_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
RSpec.describe EventDiffer do
let(:organization) { create(:organization) }
let(:storage_location1) { create(:storage_location, organization: organization) }
let(:storage_location2) { create(:storage_location, organization: organization) }
let!(:storage_location3) { create(:storage_location, organization: organization) }
let(:item1) { create(:item, organization: organization) }
let(:item2) { create(:item, organization: organization) }
let(:item3) { create(:item, organization: organization) }

before(:each) do
create(:inventory_item, item: item1, storage_location: storage_location1, quantity: 50)
create(:inventory_item, item: item2, storage_location: storage_location1, quantity: 50)
create(:inventory_item, item: item1, storage_location: storage_location2, quantity: 50)
end

it "should return a full diff" do
aggregate = 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: 50), # no diff
# missing item2
item3.id => EventTypes::EventItem.new(item_id: item2.id, quantity: 70) # added item3
}
),
storage_location2.id => EventTypes::EventStorageLocation.new(
id: storage_location2.id,
items: {
item1.id => EventTypes::EventItem.new(item_id: item1.id, quantity: 60), # diff in quantity
item2.id => EventTypes::EventItem.new(item_id: item2.id, quantity: 40) # added item2
}
),
# missing storage_location3
# added storage location that doesn't exist
StorageLocation.count + 1 => EventTypes::EventStorageLocation.new(
id: StorageLocation.count + 1,
items: {}
)
}
)
results = EventDiffer.check_difference(aggregate)
expect(results.as_json).to contain_exactly(
{"aggregate" => false,
"database" => true,
"storage_location_id" => storage_location3.id,
:type => "location"},
{"aggregate" => true,
"database" => false,
"storage_location_id" => StorageLocation.count + 1,
:type => "location"},
{"aggregate" => 0,
"database" => 50,
"item_id" => item2.id,
"storage_location_id" => storage_location1.id,
:type => "item"},
{"aggregate" => 70,
"database" => 0,
"item_id" => item3.id,
"storage_location_id" => storage_location1.id,
:type => "item"},
{"aggregate" => 40,
"database" => 0,
"item_id" => item2.id,
"storage_location_id" => storage_location2.id,
:type => "item"},
{"aggregate" => 60,
"database" => 50,
"item_id" => item1.id,
"storage_location_id" => storage_location2.id,
:type => "item"}
)
end
end
18 changes: 18 additions & 0 deletions spec/events/inventory_aggregate_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,24 @@
let(:item2) { FactoryBot.create(:item, organization: organization) }
let(:item3) { FactoryBot.create(:item, organization: organization) }

describe "discrepancy", skip_transaction: true do
let(:donation) { create(:donation, organization: organization) }

before(:each) do
allow(InventoryAggregate).to receive(:inventory_for).and_return([])
allow(EventDiffer).to receive(:check_difference).and_return([{foo: "bar"}, {baz: "spam"}])
end

it "should save a discrepancy" do
DonationEvent.publish(donation)
expect(InventoryDiscrepancy.count).to eq(1)
disc = InventoryDiscrepancy.last
expect(disc.event).to eq(DonationEvent.last)
expect(disc.diff).to eq([{"foo" => "bar"}, {"baz" => "spam"}])
expect(disc.organization).to eq(organization)
end
end

describe "individual events" do
let(:inventory) do
EventTypes::Inventory.new(
Expand Down
8 changes: 8 additions & 0 deletions spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@

# Make FactoryBot easier.
config.include FactoryBot::Syntax::Methods
config.around(:each, skip_transaction: true) do |example|
config.use_transactional_fixtures = false
example.run
config.use_transactional_fixtures = true

DatabaseCleaner.clean_with(:truncation)
seed_base_data_for_tests
end

#
# --------------------
Expand Down
4 changes: 3 additions & 1 deletion spec/system/donation_system_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@
visit @url_prefix + "/donations/new"
end

it "Allows donations to be created IN THE PAST" do
# using this to also test user ID for events - it needs to be an actual controller action
it "Allows donations to be created IN THE PAST", versioning: true do
select Donation::SOURCES[:misc], from: "donation_source"
select StorageLocation.first.name, from: "donation_storage_location_id"
select Item.alphabetized.first.name, from: "donation_line_items_attributes_0_item_id"
Expand All @@ -175,6 +176,7 @@
click_button "Save"
end.to change { Donation.count }.by(1)

expect(DonationEvent.last.user).to eq(@user)
expect(Donation.last.issued_at).to eq(Time.zone.parse("2001-01-01"))
end

Expand Down
Loading