-
-
Notifications
You must be signed in to change notification settings - Fork 594
5667 add more robust inventory tracking approach #5696
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| class DistributionCompleteEvent < Event | ||
| serialize :data, coder: EventTypes::StructCoder.new(EventTypes::DistributionPayload) | ||
|
|
||
| # @param distribution [Distribution] | ||
| def self.publish(distribution) | ||
| create( | ||
| eventable: distribution, | ||
| group_id: "dist-complete-#{distribution.id}-#{SecureRandom.hex}", | ||
| organization_id: distribution.organization_id, | ||
| event_time: Time.zone.now, | ||
| data: EventTypes::DistributionPayload.new( | ||
| reserves_inventory: false, | ||
| items: EventTypes::EventLineItem.from_line_items(distribution.line_items, from: distribution.storage_location_id) | ||
| ) | ||
| ) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| module Types | ||
| include Dry.Types() | ||
| end | ||
|
|
||
| module EventTypes | ||
| class DistributionPayload < InventoryPayload | ||
| attribute :reserves_inventory, Types::Bool.default(false) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Later on we're doing a lot of math depending on the value in this boolean. Rather than having a boolean that dictates what the interpretation of "quantity" means, I'd rather set both values (reserved and physical) in the payload itself. It means we have to be careful to set only one or the other value at a time, but the reading piece becomes much simpler. |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,11 @@ class EventItem < Dry::Struct | |
| transform_keys(&:to_sym) | ||
| attribute :item_id, Types::Integer | ||
| attribute :quantity, Types::Integer | ||
| attribute :reserved_quantity, Types::Integer.default(0) | ||
| attribute? :storage_location_id, Types::Integer | ||
|
|
||
| def physical_quantity | ||
| quantity + reserved_quantity | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If quantity is neither physical nor reserved... what is it? |
||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| class DistributionCompleteService < DistributionService | ||
| def initialize(distribution_id) | ||
| @distribution_id = distribution_id | ||
| end | ||
|
|
||
| def call | ||
| perform_distribution_service do | ||
| raise "Distribution #{distribution_id} is already complete" if distribution.complete? | ||
|
|
||
| DistributionCompleteEvent.publish(distribution) | ||
| distribution.complete! | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| RSpec.describe DistributionEvent do | ||
| let(:organization) { create(:organization) } | ||
| let(:storage_location) { create(:storage_location, organization: organization) } | ||
| let(:item) { create(:item, organization: organization) } | ||
| let(:distribution) do | ||
| dist = create(:distribution, organization: organization, storage_location: storage_location) | ||
| dist.line_items << build(:line_item, quantity: 30, item: item, itemizable: dist) | ||
| dist | ||
| end | ||
|
|
||
| before { TestInventory.create_inventory(organization, {storage_location.id => {item.id => 100}}) } | ||
|
|
||
| describe ".publish" do | ||
| subject { described_class.publish(distribution).data.reserves_inventory } | ||
|
|
||
| context "when the feature is enabled for the organization" do | ||
| before { Flipper.enable(:reserved_inventory) } | ||
|
|
||
| it { is_expected.to be true } | ||
|
|
||
| context "when the distribution is already complete" do | ||
| before do | ||
| distribution.complete! | ||
| end | ||
|
|
||
| it { is_expected.to be false } | ||
| end | ||
|
|
||
| end | ||
|
|
||
| context "when the feature is not enabled for the organization" do | ||
| before do | ||
| expect(Flipper.enabled?(:reserved_inventory)).to eq false | ||
| end | ||
|
|
||
| it { is_expected.to be false } | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| RSpec.describe EventTypes::EventItem do | ||
| let(:quantity) { 70 } | ||
| let(:reserved_quantity) { 30 } | ||
| subject(:base_item) { described_class.new(item_id: 1, **{ quantity: quantity, reserved_quantity: reserved_quantity }.compact_blank) } | ||
|
|
||
| describe "#physical_quantity" do | ||
| subject { base_item.physical_quantity } | ||
|
|
||
| it "sums available and reserved" do | ||
| expect(subject).to eq(quantity + reserved_quantity) | ||
| end | ||
| end | ||
|
|
||
| describe "#reserved_quantity" do | ||
| subject { base_item.reserved_quantity } | ||
|
|
||
| context "when no reserve quantity is provided" do | ||
| let(:reserved_quantity) { nil } | ||
|
|
||
| it "defaults to zero so payloads predating the attribute still load" do | ||
| expect(subject).to eq(0) | ||
| expect(base_item.physical_quantity).to eq(70) | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is the idea that if the distribution isn't scheduled, it gets added immediately to the physical quantity? Is that a good assumption?