-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
4333 remove inventory items #4607
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # app/controllers/storage_locations_controller.rb # app/models/item.rb # app/services/item_create_service.rb # spec/controllers/donations_controller_spec.rb # spec/models/inventory_item_spec.rb # spec/models/organization_stats_spec.rb # spec/requests/distributions_requests_spec.rb # spec/requests/purchases_requests_spec.rb # spec/requests/storage_locations_requests_spec.rb # spec/services/allocate_kit_inventory_service_spec.rb # spec/services/deallocate_kit_inventory_service_spec.rb # spec/system/distribution_system_spec.rb
# Conflicts: # app/controllers/distributions_controller.rb # spec/rails_helper.rb
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.
Some initial notes; I'll need to read through in particular the adjustment (increase/decrease) cleanup in detail
if @change_by.positive? | ||
KitAllocateEvent.publish(@kit, @storage_location.id, @change_by) | ||
else | ||
KitDeallocateEvent.publish(@kit, @storage_location.id, -@change_by) | ||
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.
Much better
to_storage_location: storage_location.id, | ||
to_storage_location: storage_location, |
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.
Interesting. Good looking.
@@ -207,16 +196,6 @@ def self.csv_export_headers | |||
["Name", "Barcodes", "Base Item", "Quantity"] | |||
end | |||
|
|||
# TODO remove this method once read_events? is true everywhere |
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.
Very good
@@ -40,9 +37,6 @@ def items_below_minimum_quantity | |||
if @inventory |
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.
@dorner same, etc down
<% else %> | ||
<%= render partial: "inventory_item_row", | ||
collection: @storage_location.inventory_items.joins(:item).where(items: { active: true }), | ||
locals: { version_date: params[:version_date] } %> | ||
<% 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.
No longer need the else at all, and maybe don't need to elsif @legacy_inventory
block either?
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.
Heh I fell into that trap. We do need this because it tracks inventory from before event sourcing was introduced.
@dorner " It also removes Kit allocation and deallocation." |
Resolves #4333
Description
Definitely do not merge this.
This removes InventoryItems and all associated functionality, with the exception of the records themselves and the basic associations. It also removes Kit allocation and deallocation.
Need to get all tests passing, and then do some exhaustive manual testing afterwards.