OBPIH-7041: prepend zone to bin location in CC tables#5236
OBPIH-7041: prepend zone to bin location in CC tables#5236
Conversation
ewaterman
added
the
status: work in progress
github-actions
bot
added
type: feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5236 +/- ##
============================================
+ Coverage 8.27% 8.31% +0.04%
- Complexity 963 988 +25
============================================
Files 638 638
Lines 43246 43246
Branches 10511 10511
============================================
+ Hits 3579 3597 +18
+ Misses 39116 39096 -20
- Partials 551 553 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| JOIN location facility ON pa.location_id = facility.id | ||
| LEFT OUTER JOIN location bin_location ON pa.bin_location_id = bin_location.id | ||
| LEFT OUTER JOIN location zone ON bin_location.zone_id = zone.id |
There was a problem hiding this comment.
Have you checked the performance with/without your changes? There are a lot of new joins, so I am curious how it looks right now 🤔
There was a problem hiding this comment.
it's hard for me to test on my local since the performace is all over the place. It doesn't seem to be significantly slower than it was before but it was already really quite slow so I hope the task for materializing the views will help with that
ewaterman
removed
the
status: work in progress
| product: product, | ||
| inventoryItem: inventoryItem, | ||
| binLocation: location?.toBaseJson(), | ||
| binLocation: location?.toJson(), |
There was a problem hiding this comment.
I think it would be better to add zone name and zone id to the toBaseJson() instead using toJson, because it checks some activity codes, etc, which might degrade performance in huge cycle counts.
There was a problem hiding this comment.
That's a fair point. I didn't want to add it to the base json because I didn't want to require the other users of that method to also pull in the zone location and degrade their performance.
I see we have this, which only adds zone information if the location type is a bin, so I think this is a good option:
binLocation: Location.toJson(location)
Side note, to me this indicates we need different location DTOs for different use cases. Or let CycleCountItem write its own toJson for bin location. The Location domain shouldn't need to hold all these different feature-specific toJson methods.
kchelstowski
left a comment
kchelstowski
left a comment
There was a problem hiding this comment.
Performance would be my only concern, so if it's fine, then I'm fine with the PR.
Remember though to use === comparison in JS.
Co-authored-by: Kacper Chełstowski <[email protected]>
4 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-7041
Description: Prepend zone to bin location name in all cycle count table:
📷 Screenshots & Recordings (optional)
Bins now have zone (ex: "Container 1") prepended to their display name:
And on the count step as well:
