OBPIH-6472 Reintroduce shipment events tab in stock movement UI by alannadolny · Pull Request #4947 · openboxes/openboxes

alannadolny · 2024-11-19T15:08:27Z

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

codecov · 2024-11-19T15:14:21Z

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.

Project coverage is 7.62%. Comparing base (96e3580) to head (86ae66c).
Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
...-app/domain/org/pih/warehouse/core/Location.groovy	0.00%	8 Missing ⚠️
...warehouse/inventory/StockMovementController.groovy	0.00%	4 Missing ⚠️
...app/domain/org/pih/warehouse/core/EventType.groovy	0.00%	4 Missing ⚠️
.../domain/org/pih/warehouse/shipping/Shipment.groovy	0.00%	4 Missing ⚠️
...g/pih/warehouse/shipping/ShipmentController.groovy	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #4947      +/-   ##
============================================
+ Coverage       7.61%   7.62%   +0.01%     
- Complexity       810     818       +8     
============================================
  Files            600     600              
  Lines          42243   42270      +27     
  Branches       10268   10275       +7     
============================================
+ Hits            3216    3224       +8     
- Misses         38564   38581      +17     
- Partials         463     465       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

…ation

awalkowiak · 2024-11-20T10:32:35Z

grails-app/views/stockMovement/_events.gsp

+                    <g:select id="eventLocation.id"
+                              name="eventLocation.id"
+                              noSelection="['': warehouse.message(code: 'default.selectOne.label')]"
+                              from='${Location.list()}'


Ooof, that Location.list() might be an overkill. On obdev3 this gives us 51646 options with all locations in the system (even inactive). What should be a potential location to choose here? Depots, Wards, Internal Locations? Can we somehow narrow this or do an async select?

Additional question - should this be required? On the old event tab, it looks like it is preselected with origin and cannot be cleared.

AND the Location field selects the value “null” by default

awalkowiak · 2024-11-20T10:37:34Z

grails-app/views/stockMovement/_events.gsp

+                    </label>
+                </td>
+                <td valign="top" class="value ${hasErrors(bean: eventInstance, field: 'eventDate', 'errors')}">
+                    <g:datePicker name="eventDate" value="${eventInstance?.eventDate}" precision="minute"/>


Should this be required? Or is later set with the current datetime if empty?

This picker cannot be empty on form submission. It's preset with the current date.

kchelstowski · 2024-11-20T12:33:58Z

grails-app/controllers/org/pih/warehouse/inventory/StockMovementController.groovy

    }

+    def events() {
+        StockMovement stockMovement = getStockMovement(params.id)


I would use the stockMovementService's method to be consistent with the show/view page.

actually see how the logic is handled for the def show() , I would use the same snippet here:

// Pull Outbound Stock movement (Requisition based) or Outbound or Inbound Return (Order based) def stockMovement = outboundStockMovementService.getStockMovement(params.id) // For inbound stockMovement only if (!stockMovement) { stockMovement = stockMovementService.getStockMovement(params.id) }

I would leave the code as is here, the getStockMovement method was introduced not that long ago, and I can see there is an additional logic inside

// Used by SM show page 'tabs' actions - packing list, documents and receipts def getStockMovement(String stockMovementId) { def stockMovement // Pull stock movement in "old fashion" way to bump up performance a bit (instead of getting OutboundStockMovement) for Non-Returns def order = Order.get(stockMovementId) if (order) { stockMovement = outboundStockMovementService.getStockMovement(stockMovementId) } else { stockMovement = stockMovementService.getStockMovement(stockMovementId) } return stockMovement }

ok, if this method is used for other tabs as well, I'm fine with that

kchelstowski · 2024-11-20T12:34:59Z

grails-app/controllers/org/pih/warehouse/inventory/StockMovementController.groovy


+    def events() {
+        StockMovement stockMovement = getStockMovement(params.id)
+        List<HistoryItem> historyItems = stockMovement.shipment.getHistory()


don't we need nullsafes here?

kchelstowski · 2024-11-20T12:37:28Z

grails-app/domain/org/pih/warehouse/shipping/Shipment.groovy

-            events?.collect({
-                it.eventType?.eventCode
-            })?.unique({ a, b -> a <=> b })?.size() == events?.size()
+            List<Event> systemEvents = events.toList().findAll {


a titbit, that you probably didn't have to parse it to list, since you have the .findAll method available also for the Set.

When I am removing the .toList() I am getting:

Cannot cast object '[Shipped|fr:Exp Rwinkwavu Pharmacy Warehouse on 2024-08-23 16:02:22.0]' with class 'java.util.TreeSet' to class 'java.util.List' due to: groovy.lang.GroovyRuntimeException: Could not find matching constructor for: java.util.List(org.pih.warehouse.core.Event)

Isn’t it due to the fact that you declared the returned type as List?

kchelstowski · 2024-11-20T12:38:34Z

grails-app/domain/org/pih/warehouse/shipping/Shipment.groovy


-        // a shipment can't have two events with the same event code (this should be the case for the current event codes: CREATED, SHIPPED, RECEIVED)
-        // we may want to change this in the future?
+        // a shipment can't have two system events with the same event code


actually what about partially_received events?

For now I guess it doesn't matter, because we build history items for partially_received events looking at the receipts, not the `events.
Just loud thinking

kchelstowski · 2024-11-20T12:42:19Z

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

+
+    Comment comment
+
+    User createdBy


(my bad) I can see redundant identifier in this domain (it was moved to the ReferenceDocument).
Could you please remove the identifier from the HistoryItem and the assignments of it (Shipment domain, Receipt domain, Event domain).

kchelstowski · 2024-11-21T13:36:18Z

grails-app/domain/org/pih/warehouse/core/Location.groovy

+            and {
+                eq('active', true)
+                not {
+                    'in'('locationType.locationTypeCode', [LocationTypeCode.INTERNAL, LocationTypeCode.BIN_LOCATION])


you could use LocationTypeCode.listInternalTypeCodes() here.

alannadolny requested review from awalkowiak, ewaterman and kchelstowski November 19, 2024 15:08

alannadolny self-assigned this Nov 19, 2024

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 Nov 19, 2024

alannadolny added 8 commits November 20, 2024 11:08

OBPIH-6472 Add events list and create section

2402eca

OBPIH-6472 Add new translations

f5a6595

OBPIH-6472 Add comment to history item

da8bcdf

OBPIH-6472 Add listing custom types for selecting in custom event cre…

7fbb224

…ation

OBPIH-6472 Fix validation for custom events in shipment domain

54e5cf6

OBPIH-6472 Fix redirecting after events creation

91c38a1

OBPIH-6472 Add action for events tab

04f0603

OBPIH-6472 Add createdBy to the events list

936e2c9

alannadolny force-pushed the OBPIH-6472-2 branch from 5e2687a to 936e2c9 Compare November 20, 2024 10:09

awalkowiak requested changes Nov 20, 2024

View reviewed changes

kchelstowski reviewed Nov 20, 2024

View reviewed changes

OBPIH-6472 Fixes after review

8175267

alannadolny force-pushed the OBPIH-6472-2 branch from 5f6c8f9 to 8175267 Compare November 21, 2024 11:37

alannadolny requested review from awalkowiak and kchelstowski November 21, 2024 11:38

kchelstowski reviewed Nov 21, 2024

View reviewed changes

OBPIH-6472 Fix listing internal type codes

86ae66c

kchelstowski approved these changes Nov 21, 2024

View reviewed changes

awalkowiak approved these changes Nov 22, 2024

View reviewed changes

awalkowiak merged commit c9f0bf3 into develop Nov 25, 2024

awalkowiak deleted the OBPIH-6472-2 branch November 25, 2024 09:53

Uh oh!

Conversation

alannadolny commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📷 Screenshots & Recordings (optional)

Uh oh!

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

kchelstowski Nov 20, 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!

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 Nov 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alannadolny commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

kchelstowski Nov 20, 2024 •

edited

Loading

kchelstowski Nov 20, 2024 •

edited

Loading