OBPIH-6901 Submit recount API by ewaterman · Pull Request #5057 · openboxes/openboxes

ewaterman · 2025-02-19T20:51:14Z

✨ Description of Change

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

Description: The API for submitting a recount. Now that the recount has been submitted, we need to be able to submit not only the "snapshot" product inventory transaction, but also an adjustment transaction containing the changes in quantity that resulted from the count. I chose to split this logic out into its own component because the cycle count service was getting really big already.

ewaterman · 2025-02-19T20:51:42Z

~~I haven't tested the code yet so I'm marking as do not merge, but I wanted to get some initial thoughts on the approach before I dug further into it.~~

Edit: tested now

ewaterman · 2025-02-19T20:52:22Z

src/main/groovy/org/pih/warehouse/core/Constants.groovy

    static final String PRODUCT_INVENTORY_TRANSACTION_TYPE_ID = "11"
    static final String CYCLE_COUNT_PRODUCT_INVENTORY_TRANSACTION_TYPE_ID = "12"
    static final String CYCLE_COUNT_ADJUSTMENT_TRANSACTION_TYPE_ID = "13"
-    static final String CYCLE_COUNT_ADJUSTMENT_DEBIT_TRANSACTION_TYPE_ID = "14"


https://openboxes.slack.com/archives/C06JCHWR807/p1739986744402599

if we want to delete it, we also have to create a migration to delete it from the db or force users to delete it on their own.

Oh yeah we don't necessarily want to delete the DEBIT adjustment, just standardize on the use case where we use the CREDIT adjustments and allow the sign of the quantity data to dictate whether it's a credit or debit.

Oh sorry, this was the cycle count adjustment debit, not the original one.

ewaterman · 2025-02-19T20:55:00Z

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

+ * Importantly, transactions created via this class should NOT result in any quantity changes/adjustments.
+ */
+@Component
+class ProductInventorySnapshotTaker {


@jmiranda added you as a reviewer primarily to check out this file. I figure that as we move towards the "product inventory as snapshot" approach, we'll want this code to be in a class like this so it's easier to integrate into other places while keeping the logic the same.

I think I would just put it in the TransactionService because you are basically only creating a Transaction. Or TransactionRepository if you want to avoid Service We also have an existing InventorySnapshot domain, so imho this is adding another class to confuse us (InventorySnapshot, InventoryItemSnapshot, ProductAvailability). But I will leave a final call for this one to @jmiranda.

codecov · 2025-02-19T20:59:19Z

Codecov Report

Attention: Patch coverage is 67.04545% with 29 lines in your changes missing coverage. Please review.

Project coverage is 8.04%. Comparing base (ea98c46) to head (9eec4e8).
Report is 198 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ouse/inventory/CycleCountTransactionService.groovy	81.25%	6 Missing and 3 partials ⚠️
...se/inventory/CycleCountSubmitRecountCommand.groovy	0.00%	6 Missing ⚠️
...g/pih/warehouse/api/CycleCountApiController.groovy	0.00%	5 Missing ⚠️
...nventory/ProductInventoryTransactionService.groovy	73.68%	3 Missing and 2 partials ⚠️
...main/org/pih/warehouse/inventory/CycleCount.groovy	60.00%	2 Missing ⚠️
...g/pih/warehouse/inventory/CycleCountService.groovy	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #5057      +/-   ##
============================================
+ Coverage       7.79%   8.04%   +0.24%     
- Complexity       876     918      +42     
============================================
  Files            629     632       +3     
  Lines          42911   42983      +72     
  Branches       10392   10406      +14     
============================================
+ Hits            3345    3457     +112     
+ Misses         39067   38999      -68     
- Partials         499     527      +28

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

kchelstowski · 2025-02-21T12:53:13Z

src/main/groovy/org/pih/warehouse/core/Constants.groovy

    static final String PRODUCT_INVENTORY_TRANSACTION_TYPE_ID = "11"
    static final String CYCLE_COUNT_PRODUCT_INVENTORY_TRANSACTION_TYPE_ID = "12"
    static final String CYCLE_COUNT_ADJUSTMENT_TRANSACTION_TYPE_ID = "13"
-    static final String CYCLE_COUNT_ADJUSTMENT_DEBIT_TRANSACTION_TYPE_ID = "14"


if we want to delete it, we also have to create a migration to delete it from the db or force users to delete it on their own.

kchelstowski · 2025-02-21T12:54:47Z

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

+ * Component responsible for creating and persisting the Transactions that result from a cycle count.
+ */
+@Component
+class CycleCountTransactionCreator {


what's the reason it is not just a regular service? I can see it calling GORM operations, so imho it should be a regular service and should be marked as @Transactional

This is what convinced me: https://tedvinke.wordpress.com/2017/04/04/grails-anti-pattern-everything-is-a-service/

The flow I was imagining is controller -> service -> helper classes like this one, and since the service is already Transactional, not adding Transactional here would be an indicator that these classes shouldn't necessarily be called directly, it should always be through a service.

This is a weird case imho. To get rid of keeping everything in the service, we are creating helper classes that we have to remember which should be called from service, and which don't have to. This imho adds more complication. Overall this is an interesting point, but I'd first discuss it on the tech huddle and we could decide and define a rule of thumb, when it makes sense to move to helper class and when not.

sure we can discuss. And we can add Transactional here if we want. No need to make it a hard requirement that non-services always be called from services, I just felt it made the intended flow more explicit

kchelstowski · 2025-02-21T12:57:10Z

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

+
+        switch (sourceType) {
+            case ProductInventorySnapshotSource.CYCLE_COUNT:
+                transaction.cycleCount = source as CycleCount


if you cast it to CycleCount here, why can't it become a type of the argument, instead of having Object source?

becase this class isn't cycle count specific. The goal is to eventually have all sources that create product inventory snapshots come through this method, and each can provide their own source. We're going to be refactoring old product inventory flows in 0.9.5 so I was imagining this would be helpful for that.

grails-app/domain/org/pih/warehouse/inventory/CycleCount.groovy

awalkowiak · 2025-02-21T14:33:17Z

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

+ * Component responsible for creating and persisting the Transactions that result from a cycle count.
+ */
+@Component
+class CycleCountTransactionCreator {


This is a weird case imho. To get rid of keeping everything in the service, we are creating helper classes that we have to remember which should be called from service, and which don't have to. This imho adds more complication. Overall this is an interesting point, but I'd first discuss it on the tech huddle and we could decide and define a rule of thumb, when it makes sense to move to helper class and when not.

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

awalkowiak · 2025-02-21T15:25:42Z

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

+ * Importantly, transactions created via this class should NOT result in any quantity changes/adjustments.
+ */
+@Component
+class ProductInventorySnapshotTaker {


I think I would just put it in the TransactionService because you are basically only creating a Transaction. Or TransactionRepository if you want to avoid Service We also have an existing InventorySnapshot domain, so imho this is adding another class to confuse us (InventorySnapshot, InventoryItemSnapshot, ProductAvailability). But I will leave a final call for this one to @jmiranda.

awalkowiak · 2025-02-21T15:30:28Z

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

+     *
+     * @param itemQuantityOnHandIsUpToDate
+     */
+    List<Transaction> createTransactions(CycleCount cycleCount, boolean itemQuantityOnHandIsUpToDate=false) {


When will itemQuantityOnHandIsUpToDate be passed as a true? Will it be a part of a request?

yeah it's the refreshQuantityOnHand flag in the request. For us it will always be true when submitting a recount. I forgot to pass it in actually so I'll update that.

awalkowiak · 2025-02-21T15:35:21Z

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

+                continue
+            }
+
+            // We need to compare the quantity counted against the most up to date QoH in product availability.


We need to compare the quantity counted against the most up to date QoH in product availability.

A side question popped up in my head, and I do not remember if or when this was discussed. If something is already allocated, then should this be counted as well? So we are always counting QoH? Is there a chance that we miss in the count something that is allocated, but not yet sent?

a good question. Lets discuss with Manon

Answer: yes we want to include already allocated stock. We always count QoH

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

ewaterman · 2025-02-24T18:06:36Z

@jmiranda this is ready for review. Let me know if you have thoughts otherwise I'll merge it at the end of the day

jmiranda · 2025-02-24T21:52:22Z

grails-app/controllers/org/pih/warehouse/UrlMappings.groovy

            action = [POST: "submitCount"]
        }

+        "/api/facilities/$facility/cycle-counts/$cycleCountId/recount" {


Let's pluralize the subresource to abide by conventions and make it clear that it's a resource and not an action.

you mean /recounts? Why is recount not an action? I guess you could say "we're publishing a new recount" but we're not actually creating a "recount" resources, we're updating the status of existing items and creating transactions for them. So you could also say "we're performing a recount" which IMO more accurately reflects what we're doing here

jmiranda · 2025-02-24T21:57:42Z

grails-app/domain/org/pih/warehouse/inventory/CycleCount.groovy

+     */
+    Set<CycleCountItem> getItemsOfMostRecentCount() {
+        Integer countIndex = maxCountIndex
+        return cycleCountItems.findAll{ it.countIndex == countIndex}


I'd be okay with the following (slightly more succinct and sleek).

return cycleCountItems.findAll{ it.countIndex == maxCountIndex }

I tested it and doing that would re-invoke maxCountIndex for every item in cycleCountItems

jmiranda · 2025-02-24T21:59:50Z

grails-app/domain/org/pih/warehouse/inventory/CycleCount.groovy

+     * @return The largest count index of all the cycle count items. Helps determine what count we're on.
+     */
+    Integer getMaxCountIndex() {
+        return cycleCountItems.max{ it.countIndex }?.countIndex


I don't think this is possible (maybe if cycleCountItems is an empty list?), but if this could return a null, we might want to consider Elvis operator'ing a zero at the end.

that's why I have the ? at the end there. If there are no elements, the max countIndex is null. I wanted to distinguish between the case where there are no elements and there are elements with index 0. Functionally it works the same either way

jmiranda · 2025-02-24T22:13:32Z

grails-app/i18n/messages.properties

 cycleCountStartRecountCommand.cycleCountRequest.invalidCycleCountStatus=Recount cannot be started when cycle count status is ({3})
 cycleCountStartRecountCommand.countIndex.invalid=Count index ({3}) is invalid for recount
 cycleCountSubmitCountCommand.cycleCount.invalidStatus=Invalid cycle count status to be able to submit the count
+cycleCountSubmitRecountCommand.cycleCount.invalidStatus=Invalid cycle count status to be able to submit the recount


We've never adopted a standard naming convention for command classes before (and we can discuss this convention later). Throughout the application, we go back and forth between resource vs action so it doesn't really matter at this point. But this seems more like a combination of the action convention (SubmitRecount) with a namespace (CycleCount).

I like the action-oriented command class naming convention.

So I think one of the following would

SubmitCycleCountRecountCommand

SubmitRecountCommand

But would be fine with CycleCountRecountCommand as well.

I generally like the action-oriented naming convention as well but what we did here was also prefix with feature name to make it super clear what commands are grouped together. So:

feature + action + thing being actioned on

cycleCount + Submit + Recount

jmiranda · 2025-02-24T22:15:11Z

...app/migrations/0.9.x/changelog-2025-02-21-0000-remove-cycle-count-debit-transaction-type.xml

+                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog/1.9 http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-1.9.xsd">
+
+    <changeSet author="ewaterman" id="210220250000-0">


Let's not do this here. Even if PIH has never created a debit adjustment it's possible there are existing transactions in other system so this is going to lead to some support headaches.

Nevermind, I just realized this was added erroneously in a previous PR. In other words, we're not trying to delete a long-standing transaction type ID.

If it's possible, I would rather erase the earlier database migration (and manually remove the data from all "dev" databases). Since this never made it to production, that would be better than adding and removing.

yeah sure I can do that

jmiranda · 2025-02-24T22:18:28Z

src/main/groovy/org/pih/warehouse/core/Constants.groovy

@@ -91,7 +91,6 @@ class Constants {
    static final String PRODUCT_INVENTORY_TRANSACTION_TYPE_ID = "11"
    static final String CYCLE_COUNT_PRODUCT_INVENTORY_TRANSACTION_TYPE_ID = "12"
    static final String CYCLE_COUNT_ADJUSTMENT_TRANSACTION_TYPE_ID = "13"


Do we need the distinction between a product inventory transaction and cycle count product inventory transaction? Same with the adjustment?

The original idea was to be able to distinguish cycle count transactions and other transactions. I suppose this can be achieved by checking if transaction.cycleCount != null but I thought it might be useful to have a more explicit labeling of that.

Maybe we don't need it though and could re-use the existing ones

ewaterman · 2025-02-25T22:14:18Z

I'm going to submit this with the current changes but I'll start a doc for us where we can work on standardizing our naming scheme

WIP: initial implementation of submit recount API

4ba0c01

ewaterman self-assigned this Feb 19, 2025

github-actions bot added type: feature A new piece of functionality for the app domain: backend Changes or discussions relating to the backend server domain: l10n Changes or discussions relating to localization & Internationalization labels Feb 19, 2025

ewaterman added the warn: do not merge Marks a pull request that is not yet ready to be merged label Feb 19, 2025

ewaterman commented Feb 19, 2025

View reviewed changes

ewaterman requested review from awalkowiak, jmiranda and kchelstowski February 19, 2025 20:53

ewaterman commented Feb 19, 2025

View reviewed changes

ewaterman marked this pull request as ready for review February 19, 2025 20:55

awalkowiak marked this pull request as draft February 20, 2025 09:43

awalkowiak changed the title ~~OBPIH-6841 submit recount API~~ OBPIH-6901 Submit recount API Feb 20, 2025

kchelstowski reviewed Feb 21, 2025

View reviewed changes

awalkowiak reviewed Feb 21, 2025

View reviewed changes

add tests and address review comments

cb41e0a

github-actions bot added the flag: schema change Hilights a pull request that contains a change to the database schema label Feb 21, 2025

ewaterman removed the warn: do not merge Marks a pull request that is not yet ready to be merged label Feb 21, 2025

ewaterman marked this pull request as ready for review February 21, 2025 19:45

change helper classes to services

9f37c43

jmiranda reviewed Feb 24, 2025

View reviewed changes

ewaterman added 2 commits February 25, 2025 16:02

remove feature specific transaction types and other review fixes

c1df6a5

Merge branch 'develop' into ft/OBPIH-6841-perform-recount-api

9eec4e8

ewaterman merged commit f3d5de6 into develop Feb 25, 2025
9 checks passed

ewaterman deleted the ft/OBPIH-6841-perform-recount-api branch February 25, 2025 22:14

Uh oh!

Conversation

ewaterman commented Feb 19, 2025

✨ Description of Change

Uh oh!

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

Uh oh!

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!

codecov bot commented Feb 19, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

ewaterman commented Feb 24, 2025

Uh oh!

jmiranda Feb 24, 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!

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!

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

Uh oh!

Choose a reason for hiding this comment

ewaterman commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

jmiranda Feb 24, 2025 •

edited

Loading

jmiranda Feb 25, 2025 •

edited

Loading