OBPIH-6870 Add endpoint to fetch cycle count candidates by kchelstowski · Pull Request #4981 · openboxes/openboxes

kchelstowski · 2024-12-16T17:58:55Z

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket:

Description:
To be added tomorrow

📷 Screenshots & Recordings (optional)

If this PR contains a UI change, consider adding one or more screenshots here or link to a screen recording to help reviewers visualize the change. Otherwise, you can remove this section.

alannadolny · 2024-12-17T13:28:48Z

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

+                tags: product.tags?.collect { Tag tag ->
+                    [
+                        id: tag.id,
+                        tag: tag.tag,
+                    ]
+                },


I don't particularly appreciate placing any logic in the toJson function, I would rather move this part to a simple function and call it here, but it's only my opinion

alannadolny · 2024-12-17T15:45:10Z

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

+        }
+    }
+
+    private static void getCandidatesSortOrder(String sort, String orderDirection, Criteria criteria, Set<String> usedAliases) {


I would name sort -> sortBy. Sort sounds like it should be desc or asc

I'm fine with renaming it, but in my mind sort is what we sort by, and desc/asc is the direction.

ewaterman · 2024-12-17T23:07:16Z

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

+import org.pih.warehouse.product.Category
+import org.pih.warehouse.product.ProductCatalog
+
+class CycleCountCandidateFilterCommand extends PaginationCommand implements Validateable {


what about status? We want to be able to filter out candidates that already have a count in progress.

It's not achievable yet, because the cycle count table doesn't exist. Moreover I think we still need to determine how we would want to join the status data to this query. In my mind we would have to modify the view to join the cycle count table to return status. I would not recommend doing an additional query to cycle count table and then filtering it out statically, because we might lose the pagination.

We discussed this in the meeting, but I agree that we should modify the view to join Cycle Counts, and also Cycle Count Requests. We likely need both because Request is the table that holds productId and so that's how we'll join with Candidate. And then we'll fetch the Cycle Count associated with that request to get the Cycle Count status (for the purpose of filtering out rows)

But yes that's for a future ticket

alannadolny · 2024-12-18T14:13:29Z

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

+@Transactional
+class CycleCountService {
+
+    List<CycleCountCandidate> getCandidates(CycleCountCandidateFilterCommand command, String facilityId) {


don't we need a filter for this field? Or has a ticket already been created for that?

I skipped that part for now, because those fields were meant to be "included later". For now negative item count is hardcoded as null, and I am not sure what this actually will be later.

codecov · 2024-12-18T17:37:27Z

Codecov Report

Attention: Patch coverage is 6.25000% with 90 lines in your changes missing coverage. Please review.

Project coverage is 7.67%. Comparing base (8e9991c) to head (7d3e83b).
Report is 155 commits behind head on develop.

Files with missing lines	Patch %	Lines
...g/pih/warehouse/inventory/CycleCountService.groovy	0.00%	67 Missing ⚠️
.../inventory/CycleCountCandidateFilterCommand.groovy	0.00%	10 Missing ⚠️
...pih/warehouse/inventory/CycleCountCandidate.groovy	20.00%	8 Missing ⚠️
...g/pih/warehouse/api/CycleCountApiController.groovy	0.00%	4 Missing ⚠️
grails-app/init/org/pih/warehouse/BootStrap.groovy	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #4981      +/-   ##
============================================
- Coverage       7.68%   7.67%   -0.01%     
- Complexity       828     834       +6     
============================================
  Files            601     605       +4     
  Lines          42340   42451     +111     
  Branches       10283   10308      +25     
============================================
+ Hits            3252    3260       +8     
- Misses         38614   38711      +97     
- Partials         474     480       +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.

ewaterman · 2024-12-18T17:47:06Z

grails-app/controllers/org/pih/warehouse/api/CycleCountApiController.groovy

+    def getCandidates(CycleCountCandidateFilterCommand filterParams) {
+        List<CycleCountCandidate> candidates = cycleCountService.getCandidates(filterParams, params.facilityId)
+
+        render([data: candidates, totalCount: candidates.totalCount] as JSON)


is the "totalCount" what will be used to display the number next to the ALL PRODUCTS tab?

If so, I'm not sure this will work if we apply any filters since that will shrink the result set of the query (and presumably the totalCount) but we want the number to continue to show for the total number of un-filtered elements.

totalCount is meant for the pagination, so as you mentioned, whenever any filters are applied, the totalCount changes as well. I haven't thought about the number next to the "ALL PRODUCTS", but if you say, this should not change regardless the filters, we could just set it on frontend once (during the initial fetch, when none filters would be applied), and not update it whenever we refetch the data.

ewaterman · 2024-12-18T17:50:32Z

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

+    }
+
+
+    Map toJson() {


I'm surprised this is performant since we're fetching products, then their categories, catalogs, and tags. It seems like it would result in a lot of queries.

But if it's quick enough like you said, I'm fine with it for now.

@ewaterman yeah, since the results are paginated, it's not that slow. The slowest part of any of those queries is the part when the view is re-built.
As soon as the PR is approved I will prepare somewhat a description of the endpoint with avg times. Anyway, it was not more than 3s in any of the scenarios.

ewaterman · 2024-12-18T17:52:52Z

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

+class CycleCountService {
+
+    List<CycleCountCandidate> getCandidates(CycleCountCandidateFilterCommand command, String facilityId) {
+        if (command.hasErrors()) {


👍 for the way you've set up the command to be validated at the start

commands are validated while being bound, so here I'm only checking if it contains errors (as you can see, I did not run the command.validate() - this is run while binding the command).

ewaterman · 2024-12-18T17:55:41Z

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

+        if (command.hasErrors()) {
+            throw new ValidationException("Invalid params", command.errors)
+        }
+        Location facility = Location.read(facilityId)


I assume "read" is like find but for read-only. Seems like we should be doing this more often 👍

What kind of exception is thrown if the location does not exist? Is it a 500? Maybe we should wrap it in a try/catch so that we can return a 400 or 404. We probably don't want to have a gross stack trace here.

For now none exception is thrown (.read returns null if an object is not found). Since we need the location id in the URL, it's a very rare case that the location would not exist. Even if it doesn't, the output would just be:

{ data: [], totalCount: 0, }

I'm fine with addressing this edge case later, if we are not comfortable with the empty result for non-existing location, which is barely to happen.

OBPIH-6870 Add endpoint to fetch cycle count candidates

3724698

kchelstowski requested review from alannadolny, awalkowiak, drodzewicz, ewaterman and jmiranda December 16, 2024 17:58

kchelstowski self-assigned this Dec 16, 2024

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 Dec 16, 2024

alannadolny reviewed Dec 17, 2024

View reviewed changes

ewaterman reviewed Dec 17, 2024

View reviewed changes

OBPIH-6870 Fixes after review

d4cab60

alannadolny approved these changes Dec 18, 2024

View reviewed changes

alannadolny reviewed Dec 18, 2024

View reviewed changes

OBPIH-6870 Get rid of RANK() and sort view by default

7d3e83b

ewaterman reviewed Dec 18, 2024

View reviewed changes

awalkowiak approved these changes Dec 19, 2024

View reviewed changes

awalkowiak merged commit 08b3608 into develop Dec 19, 2024

awalkowiak deleted the OBPIH-6870 branch December 19, 2024 09:43

Uh oh!

Conversation

kchelstowski commented Dec 16, 2024 • 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!

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 Dec 18, 2024 • 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!

codecov bot commented Dec 18, 2024 • 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!

ewaterman Dec 18, 2024 • 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!

kchelstowski Dec 18, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kchelstowski commented Dec 16, 2024 •

edited

Loading

kchelstowski Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

ewaterman Dec 18, 2024 •

edited

Loading

kchelstowski Dec 18, 2024 •

edited

Loading