OBPIH-7258 Group rows for individual cycle count on a product in inve…#5273
OBPIH-7258 Group rows for individual cycle count on a product in inve…#5273awalkowiak merged 2 commits intodevelopfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5273 +/- ##
============================================
- Coverage 8.38% 8.31% -0.07%
+ Complexity 996 985 -11
============================================
Files 646 647 +1
Lines 43397 43433 +36
Branches 10535 10543 +8
============================================
- Hits 3637 3613 -24
- Misses 39195 39261 +66
+ Partials 565 559 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (command.startDate && command.endDate) { | ||
| between("dateRecorded", command.startDate, command.endDate) | ||
| } | ||
| else if (command.startDate) { | ||
| gte("dateRecorded", command.startDate) | ||
| } | ||
| else if (command.endDate) { | ||
| lte("dateRecorded", command.endDate) | ||
| } |
There was a problem hiding this comment.
Won't it work when we just add:
if (command.startDate) {
gte("dateRecorded", command.startDate)
}
if (command.endDate) {
lte("dateRecorded", command.endDate)
}
?
…ntory transactions report
| group_concat(blind_count_variance_reason_code) as blind_count_variance_reason_code, | ||
| group_concat(blind_count_variance_comment) as blind_count_variance_comment, |
There was a problem hiding this comment.
Why don't we allow the frontend to do the work of grouping? Concatenating records is not convenient in the case of the frontend. It pushes us to split the records to be able to map them correctly (what in case when one of the root causes includes the separator that was used for the concatenation here?) I found something that can be helpful in that case in the MySQL docs.
https://dev.mysql.com/doc/refman/8.4/en/aggregate-functions.html#function_json-arrayagg
There was a problem hiding this comment.
The problem is that we don't want the view to return multiple rows here (which was the only recourse besides doing an aggregation like GROUP_CONCAT). That would mean that we'd have a bunch of duplicate rows that we'd need to somehow de-duplicate or aggregate at the API or client level. Or we'd need an N+1 query to get the comments / variance reason codes for each row.
I was going to have the API convert the string to an array when I initially developed the view, but I decided to satisfy Manon's "this is fine as one big string for now" requirement now and deal with it later. Ignoring the JSON ARRAY approach for a second, the only solution that I could think of would require us to choose a delimiter for the GROUP_CONCAT that we know would never be used in the comment content (comma is used too often, semi-colon or pipe might be ok but we'd have some conflicts/bugs eventually). So again, due to the can of 🪱 s nature of the problem and Manon's "one big string is fine for now" requirement, I decided not to deal with it.
Getting to your proposed solution ...
I found something that can be helpful in that case in the MySQL docs.
This is awesome, thank you. I wasn't aware of the JSON ARRAY function at the time so I will see if it works. I'm slightly worried that Grails/GORM won't handle this JSON array properly so I'm not confident it'll actually work out of the box. We might need to add our own user type for it which will need to be prioritized against all of the other work. But we'll see.
awalkowiak
left a comment
There was a problem hiding this comment.
I am merging as is, and we can improve it while working on a frontend counterpart
…ntory transactions report