OBPIH-6898 Cycle count start recount API by ewaterman · Pull Request #5051 · openboxes/openboxes

ewaterman · 2025-02-14T15:49:22Z

✨ Description of Change

Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6898

Description: Implement the start recount API and call it in the frontend. Note that the frontend does not yet navigate to the recount page when start resolution is clicked. That will be added in a separate PR. This API will, like the start count API, insert new cycle count items for each item of the requested product as per product availability, setting a new QoH snapshot for each item.

📷 Screenshots & Recordings (optional)

ewaterman · 2025-02-14T15:55:09Z

src/main/groovy/org/pih/warehouse/inventory/CycleCountStartCommand.groovy


    CycleCountRequest cycleCountRequest
-
-    Integer countIndex


since I split the start count and start recount into separate APIs, we don't even need to specify countIndex on the start count API anymore, we can just assume countIndex==0

codecov · 2025-02-14T15:56:39Z

Codecov Report

Attention: Patch coverage is 4.22535% with 68 lines in your changes missing coverage. Please review.

Project coverage is 7.83%. Comparing base (74f6131) to head (b019730).
Report is 195 commits behind head on develop.

Files with missing lines	Patch %	Lines
...g/pih/warehouse/inventory/CycleCountService.groovy	0.00%	27 Missing ⚠️
...use/inventory/CycleCountStartRecountCommand.groovy	0.00%	14 Missing ⚠️
...nventory/CycleCountStartRecountBatchCommand.groovy	0.00%	10 Missing ⚠️
...g/pih/warehouse/core/dtos/BatchCommandUtils.groovy	0.00%	9 Missing ⚠️
...g/pih/warehouse/api/CycleCountApiController.groovy	0.00%	6 Missing ⚠️
...rg/pih/warehouse/inventory/CycleCountStatus.groovy	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #5051      +/-   ##
============================================
- Coverage       7.83%   7.83%   -0.01%     
  Complexity       884     884              
============================================
  Files            626     629       +3     
  Lines          42871   42911      +40     
  Branches       10385   10392       +7     
============================================
+ Hits            3359    3362       +3     
- Misses         39011   39048      +37     
  Partials         501     501

☔ 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.

ewaterman · 2025-02-14T15:58:08Z

src/main/groovy/org/pih/warehouse/core/dtos/BatchCommandUtils.groovy

+import org.springframework.validation.BeanPropertyBindingResult
+import org.springframework.validation.Errors
+
+class BatchCommandUtils {


we've been adding a lot of batch commands so I figured I'd move this validation logic to a common place to avoid the duplicate code

thank you for that. I considered doing the same thing if I had to validate the batch in one more API.

alannadolny · 2025-02-17T10:31:51Z

src/main/groovy/org/pih/warehouse/core/dtos/BatchCommandUtils.groovy

+        }
+
+        // We need to build the errors manually so that we can group them all together.
+        Errors errors = new BeanPropertyBindingResult(batch, batchPropertyName)


isn't it worth creating an enum for batchPropertyName?

this is the name of the List field in the parent command so I don't know if an enum makes sense here

alannadolny · 2025-02-17T10:31:58Z

grails-app/i18n/messages.properties

-cycleCountStartCommand.countIndex.mustEqualZero=Count index must equal 0 for a new cycle count
+cycleCountStartRecountCommand.cycleCountRequest.noCycleCountFound=
+cycleCountStartRecountCommand.cycleCountRequest.invalidCycleCountStatus=Recount cannot be started when cycle count status is ({3})
+cycleCountStartRecountCommand.countIndex.invalid=Count index ({3}) is invalid for recount


Users will see this error. Is the "count index" understandable for the users?

yeah we don't expose this field to users so if this error is triggered it means we have a bug and there's nothing the user can do to fix it. I don't think there's a message we could include here that would actually help users. We'd just need to fix the bug. It shouldn't happen though since we hard code the count index to 1 for recounts.

kchelstowski · 2025-02-18T11:29:43Z

grails-app/services/org/pih/warehouse/inventory/CycleCountService.groovy

+
+        // If there are already items for the requested count index, simply return the count as it is since the recount
+        // has already been started. We do this (instead of throwing an error) because it's convenient for the frontend.
+        if (cycleCount.cycleCountItems.any(){ it.countIndex == command.countIndex }) {


I think the () after .any are unnecessary. It should be enough to do: .any { it.countIndex == command.countIndex }

kchelstowski · 2025-02-18T11:31:45Z

grails-app/services/org/pih/warehouse/inventory/CycleCountService.groovy

+            cycleCount.addToCycleCountItems(cycleCountItem)
+        }
+
+        if (!cycleCount.save()) {


I think it would be better to call .validate(), so to say:

if (!cycleCount.validate())

I am not a big fan of calling .save() explicitly on things that are already known for the hibernate persistence context (are not brand new instances).
Remember though, that you would have to save the initialized cycle count items above explicitly, because they are brand new items that are now saved thanks to the cascading behavior.

in general I agree that simply calling validate is enough. If in this case it's as you say we need to call save on each of the items, I'm not sure what approach I prefer. Calling save only when needed (hopefully) avoids potential GORM weirdness, but I do like simply calling save on the parent cycle count to reinforce the idea that saving happens from the parent down.

I'll mess around with it

I wasn't able to figure out what the performance difference was for each approach (maybe there isn't one) but in this specific scenario where we're initializing a bunch of items in a loop, I think I prefer having just the one save that we call at the end vs needing to call save on each of the items individually.

but yes I agree with you that validate is typically the better option for existing entities.

I won't force the change, but I would vote for saving each of the items individually, instead of relying on the cascade behavior. The reason I'm so opposed to the cascading and calling .save() explicilty while updating is that once I faced a similar thing in another project and that made the request run over 4 minutes. When I removed the cascading and called save only on new instances, the performance improved from 4 minutes to 2 seconds 🙈 .
Anyway it's rather my loose thought and something we can worry about when we face a problem related to that.

oh weird. Yeah I do like the idea of not relying on the cascade. Something we can potentially revisit as you say. Especially given all the scary-ness with the hasMany, we should definitely keep a close eye on the performance of these APIs.

kchelstowski · 2025-02-18T11:33:16Z

grails-app/services/org/pih/warehouse/inventory/CycleCountService.groovy

+        return CycleCountDto.toDto(cycleCount)
+    }
+
+    private CycleCountItem initCycleCountItem(


maybe it would be better to move this method to the CycleCountItem domain? What do you think?

I don't know if I want to move it to the domain because the init logic is specific to how we do the count. A new flow in the future might do things a different way and need to init via different fields so we could end up with a bunch of different constructors in the domain, which would be confusing. I tend to like to leave feature-specific flows in the feature service classes.

kchelstowski · 2025-02-18T11:34:02Z

src/main/groovy/org/pih/warehouse/core/dtos/BatchCommandUtils.groovy

+import org.springframework.validation.BeanPropertyBindingResult
+import org.springframework.validation.Errors
+
+class BatchCommandUtils {


thank you for that. I considered doing the same thing if I had to validate the batch in one more API.

kchelstowski · 2025-02-18T11:35:04Z

grails-app/i18n/messages.properties

 cycleCountRequestCommand.product.duplicateExists=Cycle count request already exists for this product ({3})
 cycleCountRequestBatchCommand.requests.duplicateExists=Cycle count request already exists for this/these products ({3})
-cycleCountStartCommand.countIndex.mustEqualZero=Count index must equal 0 for a new cycle count
+cycleCountStartRecountCommand.cycleCountRequest.noCycleCountFound=


the message is empty

…ge issues

awalkowiak

Beside one remaining !cycleCount.save() vs !cycleCount.validate() discussion, it looks good to me

Cycle count start recount API

2b4a923

ewaterman self-assigned this Feb 14, 2025

ewaterman commented Feb 14, 2025

View reviewed changes

remove unused BatchCommand class

961bfdf

ewaterman commented Feb 14, 2025

View reviewed changes

ewaterman requested review from alannadolny, awalkowiak and kchelstowski February 14, 2025 15:58

ewaterman marked this pull request as ready for review February 14, 2025 15:58

alannadolny reviewed Feb 17, 2025

View reviewed changes

kchelstowski reviewed Feb 18, 2025

View reviewed changes

ewaterman added 3 commits February 18, 2025 10:35

fix empty error message, remove redundant parenthesis

04cc258

Merge branch 'develop' into ft/OBPIH-6898-cycle-count-start-recount-api

5119a66

properly call start recount from frontend, fix message.properties mer…

b019730

…ge issues

awalkowiak approved these changes Feb 19, 2025

View reviewed changes

ewaterman merged commit fd73259 into develop Feb 19, 2025
9 checks passed

ewaterman deleted the ft/OBPIH-6898-cycle-count-start-recount-api branch February 19, 2025 15:16

Uh oh!

Conversation

ewaterman commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Description of Change

📷 Screenshots & Recordings (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchelstowski Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalkowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ewaterman commented Feb 14, 2025 •

edited

Loading

codecov bot commented Feb 14, 2025 •

edited

Loading

kchelstowski Feb 19, 2025 •

edited

Loading