Open
Conversation
Collaborator
|
Note to self Before: graph LR
kit[Kit A] --> kit_item[Item Kit A]
kit --> line_item_1[Line Item 1, Qty 3]
kit --> line_item_2[Line Item 2, Qty 7]
line_item_1 --> item_x[Item X]
line_item_2 --> item_y[Item Y]
After: graph LR
kit[Kit A] --> kit_item[Item Kit A]
kit_item --> line_item_1[Line Item 1, Qty 3]
kit_item --> line_item_2[Line Item 2, Qty 7]
line_item_1 --> item_x[Item X]
line_item_2 --> item_y[Item Y]
|
awwaiid
requested changes
Feb 1, 2026
Collaborator
awwaiid
left a comment
There was a problem hiding this comment.
A few fixes needed, and the big question -- can we remove include Itemizable from the Kit model?
|
|
||
| def is_in_kit?(kits = nil) | ||
| if kits | ||
| kits.any? { |k| k.line_items.map(&:item_id).include?(id) } |
Collaborator
There was a problem hiding this comment.
Does this need to be k.item.line_items?
Collaborator
There was a problem hiding this comment.
I'm pretty sure it does need to change.
Toward this end -- how about we remove include Itemizable from the Kit model? That will stop anything from doing kit.line_items, which we shouldn't be doing.
| AND items.kit_id IS NOT NULL | ||
| AND kit_items.reporting_category = 'disposable_diapers' | ||
| AND kit_line_items.itemizable_type = 'Kit'; | ||
| AND kit_line_items.itemizable_type = 'Item'; |
Collaborator
There was a problem hiding this comment.
app/services/reports/adult_incontinence_report_service.rb def distributed_adult_incontinence_items_from_kits needs SQL updated like this
This file contains hidden or 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
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.
This PR is the first step in the kit redesign. It moves line items from kits to the kits' item. This will set us up to make
itemsthe only table that has line items, which will make the next step (turning kit into a subclass of item) easier.line_itemstoused_line_itemsassociation inside Item - we need some way of differentiating between line items that belong to the item (as part of a kit) and line items that the item is part of (e.g. distributions, donations). There are very few uses ofitem.line_itemsso it's easier to switch the name of this association.Note that this PR does not include a feature flag. Although this is a somewhat risky change, it is not difficult to switch back if we have to:
Itemcan only have originally been on a kit;Item, that item will have akit_id.Using this, we can easily set the type and ID back to the kit if we need to roll back. Trying to feature flag this would be difficult because we are talking about changing the source of an association. Simply changing the flag won't help - we'd need a redeploy anyway if something is going wrong. If anything, it's more risky to feature flag this because the flag would have to line up with the data in the database. Anything out of sync would break things worse than the rollback.