OBPIH-6931 filter cycle count candidates by cycle count status by ewaterman · Pull Request #5030 · openboxes/openboxes

ewaterman · 2025-02-05T20:51:12Z

✨ Description of Change

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

Description:

We need to be able to filter the results in the TO COUNT and TO RESOLVE tabs by cycle count status (before it was being done on cycle count request status which doesn't have enough info for us to distinguish between the tabs). However it seems that the criteria doesn't support filtering by transient fields so the current code doesn't work. I'll try to think more about what to do here but maybe we can brainstorm together.

ewaterman · 2025-02-05T20:58:57Z

src/js/hooks/cycleCount/useToResolveTab.jsx

        </TableHeaderCell>
      ),
      cell: ({ getValue }) => (
-        // TODO: Use variant fetched from the API https://pihemr.atlassian.net/browse/OBPIH-6931


I removed the TODO here since I don't think this is something that the backend should be returning. Variant is only used for display purposes. The backend has no concept of what that field means (it's used as an html class), so it'd feel weird for the backend to decide on behalf of the frontend how it should be displaying rows of a certain status.

codecov · 2025-02-05T20:58:59Z

Codecov Report

Attention: Patch coverage is 3.70370% with 26 lines in your changes missing coverage. Please review.

Project coverage is 7.76%. Comparing base (6e6cce7) to head (db4230e).
Report is 202 commits behind head on develop.

Files with missing lines	Patch %	Lines
...main/org/pih/warehouse/inventory/CycleCount.groovy	8.33%	11 Missing ⚠️
...g/pih/warehouse/inventory/CycleCountService.groovy	0.00%	9 Missing ⚠️
...rehouse/inventory/CycleCountCandidateStatus.groovy	0.00%	2 Missing ⚠️
...y/org/pih/warehouse/inventory/CycleCountDto.groovy	0.00%	2 Missing ⚠️
...pih/warehouse/inventory/CycleCountCandidate.groovy	0.00%	1 Missing ⚠️
.../inventory/CycleCountCandidateFilterCommand.groovy	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             develop   #5030   +/-   ##
=========================================
  Coverage       7.76%   7.76%           
  Complexity       860     860           
=========================================
  Files            620     621    +1     
  Lines          42760   42758    -2     
  Branches       10373   10374    +1     
=========================================
+ Hits            3321    3322    +1     
+ Misses         38945   38942    -3     
  Partials         494     494

☔ 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-10T23:21:36Z

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

+             * maintain an accurate list of items (which it uses to compute its status). Still, the disadvantage is
+             * important to note, and we should strive to replace this hasMany with a better option in the future.
+             */
+            cycleCountItems: CycleCountItem,


@kchelstowski don't hate me for this 😅 I really tried to get it to work without the hasMany but no matter what I tried I couldn't get a simple solution for updating the cycle count status when an item's status is changed.

I tried doing it in the afterInsert/Update in CycleCountItem, but that didn't work due to transaction weirdness (it called save on the cycle count, but the status in the db didn't change, not even when I add flush: true).

I also tried adding a method like this in CycleCount:

CycleCount saveCountAndItems(List<CycleCountItem> items) { // Save the count first so that the item save doesn't fail validation this.save() for (item in items) { item.save() } // Now that the items are saved, fetch them from the db (inside the recomputeStatus method) and recompute status status = recomputeStatus() this.save() return this } CycleCountStatus recomputeStatus() { // This doesn't work! The "items" from the saveCountAndItems call aren't in the db yet and so aren't fetched here... List<CycleCountItems> items = CycleCountItem.findAllByCycleCount(this) ... }

We can discuss options for a better solution if you want but as much as I dislike hasMany, I think this is fine for now (for the reasons I mention in the comment).

@ewaterman that's weird, I might take a look if I find time, in my opinion if it's happening in the same session and transaction (I mean you update a few items, or add new), those should be immediately accessible via CycleCountItem.findAllByCycleCount(this).

Anyway, I'm fine with having the bidirectional association, but I would lie if I didn't have in mind "why not a few tickets before??" 😆

if you have time to look at it, that'd be awesome. I was also assuming that they'd be accessible because it should be the same transaction, but I couldn't get it to work. You can see some of my older commits for references to what I was testing.

ewaterman · 2025-02-10T23:23:08Z

grails-app/migrations/0.9.x/changelog-2025-02-06-0000-add-column-status-to-cycle-count.xml

+
+        <addColumn tableName="cycle_count">
+            <column name="status" type="VARCHAR(100)">
+                <constraints nullable="false" />


TODO: set a default value for existing data! Or just truncate obdev3 and 5 because this isn't live yet??

We've run once recently into a similar issue. We've just cleared the data (via the script I've sent on Slack for you some time ago). I'm fine with doing the same again.

awalkowiak · 2025-02-11T12:10:57Z

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


    def beforeUpdate() {
        updatedBy = AuthService.currentUser
+        status = recomputeStatus()


Is it updated after related items change as well? Have you considered using a formula for it (see point 8.2.11 here https://grails.github.io/legacy-gorm-doc/6.0.x/hibernate/manual/index.html#ormdsl) instead of recomputing here? Perhaps it would be a good/suiting approach as well?

The problem with having it be derived was that it was inaccessible when building the session view in cycle-count-session.sql. I didn't try it the way you suggest but I feel like that would have the same issue. I can test though

Ok, I think this makes sense then.

kchelstowski

I would like to either do another round of review tomorrow

kchelstowski · 2025-02-11T16:02:27Z

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

+             * maintain an accurate list of items (which it uses to compute its status). Still, the disadvantage is
+             * important to note, and we should strive to replace this hasMany with a better option in the future.
+             */
+            cycleCountItems: CycleCountItem,


@ewaterman that's weird, I might take a look if I find time, in my opinion if it's happening in the same session and transaction (I mean you update a few items, or add new), those should be immediately accessible via CycleCountItem.findAllByCycleCount(this).

Anyway, I'm fine with having the bidirectional association, but I would lie if I didn't have in mind "why not a few tickets before??" 😆

kchelstowski · 2025-02-11T16:05:20Z

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

+    ]
+
+    static mapping = {
+        cycleCountItems cascade: 'all-delete-orphan'


I would like to have a convention around the cascading. There are pros and cons of having/not having cascade behavior. Due to the cascading we have sometimes run into unexpected issues difficult to spot. It's better to have some of the operations being done explicitly (in this case deleting).

yeah I agree. We don't ever delete cycle counts so I can change this to only cascade updates if we want, but I figured if we ever did need to delete a cycle count, we'd also want to delete the items (otherwise we'll get constraint violations)

kchelstowski · 2025-02-11T16:05:34Z

grails-app/i18n/messages.properties

-locale.es.label=Español (España)
-locale.es_MX.label=Español (México)
+locale.es.label=EspanÌol (EspanÌa)
+locale.es_MX.label=EspaÃ±ol (MÃ©xico)


what happened here?

hm weird. I think it happened when I merged develop into the PR. I'll revert

kchelstowski · 2025-02-11T16:06:13Z

grails-app/migrations/0.9.x/changelog-2025-02-06-0000-add-column-status-to-cycle-count.xml

+
+        <addColumn tableName="cycle_count">
+            <column name="status" type="VARCHAR(100)">
+                <constraints nullable="false" />


We've run once recently into a similar issue. We've just cleared the data (via the script I've sent on Slack for you some time ago). I'm fine with doing the same again.

kchelstowski · 2025-02-11T16:06:37Z

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

            }
        }
-        return CycleCountDto.createFromCycleCountItems(cycleCountItems)
+        return CycleCountDto.asDto(cycleCount)


wouldn't toDto sound better?

kchelstowski · 2025-02-11T16:06:50Z

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

        }
-        return CycleCountDto.createFromCycleCountItems(cycleCountItems)
+
+        if(!newCycleCount.save()) {


nitpicky: missing space between if and (

kchelstowski · 2025-02-11T16:07:58Z

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

        )
+
        List<AvailableItem> itemsToSave = determineCycleCountItemsToSave(facility, request.cycleCountRequest.product)
-        newCycleCount.save()


have you made sure it can be removed? I was running into error that "Not-null property references a transient value - transient instance must be saved before current operation", but I assume the cascading now makes it work, since you introduced the bidirectional association

yup with the bidirectional + cascade save it's not needed anymore

ewaterman · 2025-02-12T20:20:04Z

I'm merging this as-is after our tech huddle discussion but we can (and should) revisit the "hasMany" and cascade behaviour conversation as we get more of the feature implemented

WIP: filter cycle count candidates by cycle count status

3f03fd2

ewaterman self-assigned this Feb 5, 2025

github-actions bot added type: feature A new piece of functionality for the app domain: frontend Changes or discussions relating to the frontend UI domain: backend Changes or discussions relating to the backend server labels Feb 5, 2025

ewaterman marked this pull request as ready for review February 5, 2025 20:51

ewaterman requested review from alannadolny, awalkowiak and kchelstowski February 5, 2025 20:51

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

ewaterman commented Feb 5, 2025

View reviewed changes

ewaterman added 5 commits February 7, 2025 10:11

WIP: make cycle count status a real field

1af0ab3

cycle count candidate status resolves properly

29ea9a5

clean up method recomputing cycle count status

2c993b7

switch back to using hasMany for cycle count items

e96c210

Merge branch 'develop' into ft/OBPIH-6931-filter-cycle-counts-by-status

ba9f82f

github-actions bot added flag: schema change Hilights a pull request that contains a change to the database schema domain: l10n Changes or discussions relating to localization & Internationalization labels Feb 10, 2025

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

ewaterman commented Feb 10, 2025

View reviewed changes

awalkowiak reviewed Feb 11, 2025

View reviewed changes

kchelstowski reviewed Feb 11, 2025

View reviewed changes

ewaterman added 2 commits February 12, 2025 08:25

fix message.properties merge commit

f4a0187

addressing minor review comments

db4230e

ewaterman merged commit 865435d into develop Feb 12, 2025
9 checks passed

ewaterman deleted the ft/OBPIH-6931-filter-cycle-counts-by-status branch February 12, 2025 20:20

Uh oh!

Conversation

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

Uh oh!

✨ Description of Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ewaterman Feb 10, 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!

awalkowiak Feb 11, 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!

kchelstowski left a comment

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!

ewaterman Feb 11, 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!

ewaterman commented Feb 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ewaterman commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

ewaterman Feb 10, 2025 •

edited

Loading

awalkowiak Feb 11, 2025 •

edited

Loading

ewaterman Feb 11, 2025 •

edited

Loading