OBPIH-7269 Add import count widget (dropzone on UI, upload endpoint, … by kchelstowski · Pull Request #5253 · openboxes/openboxes

kchelstowski · 2025-05-14T08:19:56Z

…importer class)

✨ Description of Change

Link to GitHub issue or Jira ticket:

Description:

📷 Screenshots & Recordings (optional)

…importer class)

alannadolny · 2025-05-14T10:00:46Z

src/js/api/services/CycleCountApi.js

  CYCLE_COUNT_START,
  CYCLE_COUNT_SUBMIT_COUNT,
-  CYCLE_COUNT_SUBMIT_RECOUNT,
+  CYCLE_COUNT_SUBMIT_RECOUNT, IMPORT_CYCLE_COUNT_ITEMS, IMPORT_PACKING_LIST,


Suggested change

CYCLE_COUNT_SUBMIT_RECOUNT, IMPORT_CYCLE_COUNT_ITEMS, IMPORT_PACKING_LIST,

CYCLE_COUNT_SUBMIT_RECOUNT,

IMPORT_CYCLE_COUNT_ITEMS,

IMPORT_PACKING_LIST,

alannadolny · 2025-05-14T10:17:27Z

src/js/hooks/cycleCount/useCountStep.jsx

+    try {
+      show();
+      const response = await cycleCountApi.importCycleCountItems(importFile, currentLocation?.id);
+      console.log(response);


Is it by accident?

No, I left it here so that I don't have an eslint error that response is not used. Anyway this will be adjusted in the next ticket, so I guess nothing wrong with leaving it for now.

alannadolny · 2025-05-14T10:17:49Z

src/js/hooks/cycleCount/useCountStep.jsx

+    } finally {
+      hide();
+    }
+    // TODO: Map items to the table


I assume it's for another ticket?

alannadolny · 2025-05-14T10:19:06Z

src/main/groovy/org/pih/warehouse/importer/CycleCountItemsExcelImporter.groovy

+    static Map PROPERTY_MAP = [
+            cycleCountId:             ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            productCode:                ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "product.name":         ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "lotNumber":            ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "expirationDate":       ([expectedType: ExpectedPropertyType.DateJavaType, defaultValue: null]),
+            "binLocation":          ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "quantityCounted":       ([expectedType: ExpectedPropertyType.IntType, defaultValue: null]),
+            "comment":            ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+    ]


I don't know whether it's just GitHub formatting, but the indentations are not aligned

codecov · 2025-05-14T14:08:42Z

Codecov Report

❌ Patch coverage is 15.00000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.28%. Comparing base (4127d11) to head (5809594).
⚠️ Report is 140 commits behind head on develop.

Files with missing lines	Patch %	Lines
...house/importer/CycleCountItemsExcelImporter.groovy	0.00%	11 Missing ⚠️
...g/pih/warehouse/api/CycleCountApiController.groovy	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             develop   #5253   +/-   ##
=========================================
  Coverage       8.27%   8.28%           
+ Complexity       980     965   -15     
=========================================
  Files            640     642    +2     
  Lines          43288   43308   +20     
  Branches       10521   10522    +1     
=========================================
+ Hits            3583    3586    +3     
- Misses         39154   39172   +18     
+ Partials         551     550    -1

☔ 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-05-14T15:37:39Z

Can you add a screenshot or recording of what the widget looks like? It would be good to be able to review the actual design of it.

ewaterman · 2025-05-14T15:25:02Z

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

        cycleCounts.each { CycleCountDto cycleCount ->
            cycleCount.cycleCountItems.each { CycleCountItemDto item ->
                data << [
+                        "Cycle count id": cycleCount.id,


I'm not a huge fan of needing to put cycle count id in the xls file since I'd rather not expose our internal ids to users. Can the frontend not look up the count by product? If this is the only/best solution, we should discuss with Manon to confirm that it's okay to include it.

We will see in the next ticket if there is an easy way to map product codes to cycle count ids. For now it was the easiest solution to just add cycle count id to the export/import sheet (as we also do in the inbound/outbound add items import/export).
I've already discussed it with Manon before adding it, and got an approval for that.

ewaterman · 2025-05-14T15:26:34Z

src/main/groovy/org/pih/warehouse/importer/CycleCountItemsExcelImporter.groovy

+            productCode:              ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "product.name":           ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "lotNumber":              ([expectedType: ExpectedPropertyType.StringType, defaultValue: null]),
+            "expirationDate":         ([expectedType: ExpectedPropertyType.DateJavaType, defaultValue: null]),


What happens if the date is poorly formatted at this point? Does the import fail? If so we should probably think of a different solution since we eventually want users to be able to resolve bad data in the UI, In a previous tech huddle we discussed potentially making all these fields StringType and letting the frontend handle all the validation. But if the import doesn't fail this is fine.

I checked that, and if you e.g. provide a random string instead of a date, it is parsed to null, so I believe it's expected.

This is fine for now but we might want to refactor this later to return it to the client as a string so that we can notify the user that they provided an invalid date. Otherwise if we just return null when we have an invalid format we wont be able to distinguish between the case where the lot just doesnt't have an expiry date (because it also returns null)

kchelstowski · 2025-05-15T09:28:51Z

@ewaterman attaching a screenshot:

I have not changed anything in terms of the design, it's the same as in the outbound import.

alannadolny · 2025-05-15T10:06:37Z

src/js/api/urls.js

 export const CYCLE_COUNT_SUBMIT_COUNT = (locationId, cycleCountId) => `${CYCLE_COUNT(locationId)}/${cycleCountId}/count`;
 export const CYCLE_COUNT_SUBMIT_RECOUNT = (locationId, cycleCountId) => `${CYCLE_COUNT(locationId)}/${cycleCountId}/recount`;
 export const CYCLE_COUNT_REFRESH_ITEMS = (locationId, cycleCountId, removeOutOfStockItemsImplicitly) => `${CYCLE_COUNT(locationId)}/${cycleCountId}/refresh${removeOutOfStockItemsImplicitly ? '?removeOutOfStockItemsImplicitly=true' : ''}`;
+export const IMPORT_CYCLE_COUNT_ITEMS = (locationId) => `${CYCLE_COUNT(locationId)}/items/upload/count`;


To match this constant with our previous naming for cycle count actions, I would call it CYCLE_COUNT_IMPORT_ITEMS

ewaterman

I'm approving but I think we should be ready discuss the design with Manon. I was originally expecting the importer to be a popup window (or an expandable section if that's easier).

Having it always be there is a bit awkward since some users will never use it and it takes up so much of the screen so they have to scroll just to see the table fully.

sentry · 2025-05-27T13:01:03Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ NullPointerException: Cannot get property 'originalFilename' on null object POST /openboxes/api/facilities/8a8a9e9665c4f59d... View Issue

_{Did you find this useful? React with a 👍 or 👎}

OBPIH-7269 Add import count widget (dropzone on UI, upload endpoint, …

0998105

…importer class)

kchelstowski requested review from SebastianLib, alannadolny, awalkowiak and ewaterman May 14, 2025 08:19

kchelstowski self-assigned this May 14, 2025

github-actions bot added domain: frontend Changes or discussions relating to the frontend UI domain: backend Changes or discussions relating to the backend server domain: l10n Changes or discussions relating to localization & Internationalization labels May 14, 2025

alannadolny reviewed May 14, 2025

View reviewed changes

OBPIH-7269 Adjust indentations

3d2126d

kchelstowski requested a review from alannadolny May 14, 2025 11:52

ewaterman reviewed May 14, 2025

View reviewed changes

alannadolny reviewed May 15, 2025

View reviewed changes

OBPIH-7269 Rename url

5809594

kchelstowski requested review from alannadolny and ewaterman May 15, 2025 15:50

ewaterman approved these changes May 15, 2025

View reviewed changes

awalkowiak merged commit 05c516a into develop May 16, 2025
9 checks passed

awalkowiak deleted the OBPIH-7269 branch May 16, 2025 15:18

Uh oh!

Conversation

kchelstowski commented May 14, 2025

✨ 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!

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

Uh oh!

Codecov Report

Uh oh!

ewaterman commented May 14, 2025

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 commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewaterman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sentry bot commented May 27, 2025

Suspect Issues

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 14, 2025 •

edited

Loading