Skip to content

OBPIH-7258 Group rows for individual cycle count on a product in inve…#5273

Merged
awalkowiak merged 2 commits intodevelopfrom
OBPIH-7258
May 28, 2025
Merged

OBPIH-7258 Group rows for individual cycle count on a product in inve…#5273
awalkowiak merged 2 commits intodevelopfrom
OBPIH-7258

Conversation

@jmiranda
Copy link
Member

…ntory transactions report

@github-actions github-actions bot added domain: backend Changes or discussions relating to the backend server flag: schema change Hilights a pull request that contains a change to the database schema flag: config change Hilights a pull request that contains a change to the app config labels May 23, 2025
@codecov
Copy link

codecov bot commented May 23, 2025

Codecov Report

❌ Patch coverage is 18.42105% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.31%. Comparing base (fd285c1) to head (a326b7d).
⚠️ Report is 120 commits behind head on develop.

Files with missing lines Patch % Lines
...g/pih/warehouse/inventory/CycleCountService.groovy 0.00% 16 Missing ⚠️
...g/pih/warehouse/inventory/CycleCountSummary.groovy 18.18% 9 Missing ⚠️
...g/pih/warehouse/api/CycleCountApiController.groovy 0.00% 5 Missing ⚠️
grails-app/init/org/pih/warehouse/BootStrap.groovy 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +750 to +758
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)
}
Copy link
Collaborator

@alannadolny alannadolny May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it work when we just add:

            if (command.startDate) {
                gte("dateRecorded", command.startDate)
            }
            if (command.endDate) {
                lte("dateRecorded", command.endDate)
            }

?

Comment on lines +22 to +23
group_concat(blind_count_variance_reason_code) as blind_count_variance_reason_code,
group_concat(blind_count_variance_comment) as blind_count_variance_comment,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot added the domain: frontend Changes or discussions relating to the frontend UI label May 26, 2025
Copy link
Collaborator

@awalkowiak awalkowiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am merging as is, and we can improve it while working on a frontend counterpart

@awalkowiak awalkowiak merged commit bceab2d into develop May 28, 2025
8 checks passed
@awalkowiak awalkowiak deleted the OBPIH-7258 branch May 28, 2025 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: backend Changes or discussions relating to the backend server domain: frontend Changes or discussions relating to the frontend UI flag: config change Hilights a pull request that contains a change to the app config flag: schema change Hilights a pull request that contains a change to the database schema

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants