3611: Refactor of storage location inventory updates #3614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We don't allow updates of adjustments. so they're definitely not a problem. I'm not sure I follow, yet, how audits and transfers aren't an issue. |
awwaiid
reviewed
May 24, 2023
| @@ -129,6 +129,10 @@ def self.import_inventory(filename, org, loc) | |||
| adjustment.storage_location.increase_inventory(adjustment) | |||
| end | |||
|
|
|||
| def remove_empty_items | |||
| inventory_items.where(quantity: 0).delete_all | |||
There was a problem hiding this comment.
Beware, delete_all skips callbacks. Is that ok here?
There was a problem hiding this comment.
Yep. Confirmed inventory items has no callbacks.
awwaiid
reviewed
May 24, 2023
Comment on lines
14
to
15
| increase_method = (type == :increase) ? :increase_inventory : :decrease_inventory | ||
| decrease_method = (type == :increase) ? :decrease_inventory : :increase_inventory |
There was a problem hiding this comment.
Since the names flip around depending on the direction, how about we change to a name that describes the intent a bit better. Here's an idea:
Suggested change
| increase_method = (type == :increase) ? :increase_inventory : :decrease_inventory | |
| decrease_method = (type == :increase) ? :decrease_inventory : :increase_inventory | |
| apply_change_method = (type == :increase) ? :increase_inventory : :decrease_inventory | |
| undo_change_method = (type == :increase) ? :decrease_inventory : :increase_inventory |
awwaiid
reviewed
May 24, 2023
Comment on lines
20
to
25
| update_storage_location(itemizable: itemizable, | ||
| increase_method: increase_method, | ||
| decrease_method: decrease_method, | ||
| params: params, | ||
| from_location: from_location, | ||
| to_location: to_location) |
There was a problem hiding this comment.
So here is what that rename would be here:
Suggested change
| update_storage_location(itemizable: itemizable, | |
| increase_method: increase_method, | |
| decrease_method: decrease_method, | |
| params: params, | |
| from_location: from_location, | |
| to_location: to_location) | |
| update_storage_location(itemizable: itemizable, | |
| apply_change_method: apply_change_method, | |
| undo_change_method: undo_change_method, | |
| params: params, | |
| from_location: from_location, | |
| to_location: to_location) |
awwaiid
reviewed
May 24, 2023
Comment on lines
29
to
49
| # @param itemizable [Itemizable] | ||
| # @param increase_method [Symbol] | ||
| # @param decrease_method [Symbol] | ||
| # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`. | ||
| # @param from_location [StorageLocation] | ||
| # @param to_location [StorageLocation] | ||
| def self.update_storage_location(itemizable:, increase_method:, decrease_method:, | ||
| params:, from_location:, to_location:) | ||
| from_location.public_send(decrease_method, itemizable.to_a) | ||
| # Delete the line items -- they'll be replaced later | ||
| itemizable.line_items.delete_all | ||
| # Update the current model with the new parameters | ||
| itemizable.update!(params) | ||
| itemizable.reload | ||
| # Apply the new changes to the storage location inventory | ||
| to_location.public_send(increase_method, itemizable.to_a) | ||
|
|
||
| from_location.remove_empty_items | ||
| to_location.remove_empty_items | ||
| end | ||
| end |
There was a problem hiding this comment.
And here
Suggested change
| # @param itemizable [Itemizable] | |
| # @param increase_method [Symbol] | |
| # @param decrease_method [Symbol] | |
| # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`. | |
| # @param from_location [StorageLocation] | |
| # @param to_location [StorageLocation] | |
| def self.update_storage_location(itemizable:, increase_method:, decrease_method:, | |
| params:, from_location:, to_location:) | |
| from_location.public_send(decrease_method, itemizable.to_a) | |
| # Delete the line items -- they'll be replaced later | |
| itemizable.line_items.delete_all | |
| # Update the current model with the new parameters | |
| itemizable.update!(params) | |
| itemizable.reload | |
| # Apply the new changes to the storage location inventory | |
| to_location.public_send(increase_method, itemizable.to_a) | |
| from_location.remove_empty_items | |
| to_location.remove_empty_items | |
| end | |
| end | |
| # @param itemizable [Itemizable] | |
| # @param apply_change_method [Symbol] | |
| # @param undo_change_method [Symbol] | |
| # @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`. | |
| # @param from_location [StorageLocation] | |
| # @param to_location [StorageLocation] | |
| def self.update_storage_location(itemizable:, apply_change_method:, undo_change_method:, | |
| params:, from_location:, to_location:) | |
| from_location.public_send(undo_change_method, itemizable.to_a) | |
| # Delete the line items -- they'll be replaced later | |
| itemizable.line_items.delete_all | |
| # Update the current model with the new parameters | |
| itemizable.update!(params) | |
| itemizable.reload | |
| # Apply the new changes to the storage location inventory | |
| to_location.public_send(apply_change_method, itemizable.to_a) | |
| from_location.remove_empty_items | |
| to_location.remove_empty_items | |
| end | |
| end |
|
I note that decrease will throw |
awwaiid
requested changes
May 24, 2023
I think we have an outstanding issue around catching that appropriately |
spec/factories/storage_locations.rb
Outdated
| item_quantity { 100 } | ||
| item { nil } | ||
| end | ||
|
|
||
| after(:create) do |storage_location, evaluator| | ||
| if evaluator.item.nil? | ||
| if evaluator.item.nil? && evaluator.item_count != 0 |
There was a problem hiding this comment.
Suggested change
| if evaluator.item.nil? && evaluator.item_count != 0 | |
| if evaluator.item.nil? && !evaluator.item_count.zero? |
Just some syntactic sugar
|
@awwaiid thanks for the suggestions - added in the fixes! Please check again. |
awwaiid
approved these changes
Jun 4, 2023
|
Fixed! |
|
Any reason we haven't merged this yet? |
|
Don't think so! Let's go for it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #3611
Description
This refactors the updating of storage location inventory to move it to a single place that works for distributions, purchases and donations. The centralized place uses the fix discovered in #3601 to address the race condition we found with distributions.
(Note - there are also adjustments, audits and transfers that affect inventory, but these do not make any changes to the line items themselves - in these cases the line items represent the transfer between locations rather than an update to an event that represents an increase or decrease in inventory.)
Type of change
Internal change
How Has This Been Tested?
Local and unit tests.