OBPIH-6215 Refactor returns statuses (both inbound and outbound) and … #4586

kchelstowski · 2024-04-17T10:55:26Z

…align them on list page<->view page

awalkowiak

I am not sure if this is change request or comment

awalkowiak · 2024-04-17T11:30:19Z

src/main/groovy/org/pih/warehouse/api/StockMovement.groovy

@@ -63,6 +63,7 @@ class StockMovement implements Validateable{

 StockMovementDirection stockMovementDirection
 StockMovementStatusCode stockMovementStatusCode
+ StockMovementType stockMovementType


Why this was added here if it is not populated (keep in mind this StockMovement is not backed by the stock-movement view)

It has been added in case we returned the StockMovement to the show.gsp, not the OutboundStockMovement.
Since this property was missing in the StockMovement, I would get an exception when trying to evaluate this part of the code:

<g:if test="${stockMovement?.stockMovementType == StockMovementType.RETURN_ORDER && isInbound}">

because either StockMovement or OutboundStockMovement might hide under the stockMovement.

I know it is not populated and it would return null for any case, we don't set it explicitly, but in this case we would want it to just evaluate to false, instead of throwing the exception about the missing method.

awalkowiak · 2024-04-17T11:32:10Z

grails-app/views/returns/show.gsp

@@ -130,7 +130,14 @@
 <g:message code="stockMovement.status.label"/>
 </td>
 <td class="value">
- <format:metadata obj="${stockMovement?.status ?: (stockMovement?.shipment?.status?.code ?: stockMovement?.statusCode)}"/>
+ <g:set var="isInbound" value="${stockMovement?.destination == currentLocation}" />
+ %{-- If it's an inbound return, we want to display a shipment status--}%


I feel like this should not be here because we wanted to move away from this type of logic in templates. Hopefully, this won't have to be drastically changed, after getting the results of the spike that @drodzewicz is working on.

I couldn't find an easier way to do it, as we have a generic view for both outbound and inbound returns and we have one single method (def show) for every SM we have in the system (regular inbound/outbound, PO shipments, returns, requests), so either I'd have to make mess there, or add this simple if statement here.
We'll see what @drodzewicz comes up with, but I suppose deeper refactors could be done when we refactor the SM API 🤔

awalkowiak · 2024-04-17T11:33:32Z

grails-app/views/returns/show.gsp

@@ -130,7 +130,14 @@
 <g:message code="stockMovement.status.label"/>
 </td>
 <td class="value">
- <format:metadata obj="${stockMovement?.status ?: (stockMovement?.shipment?.status?.code ?: stockMovement?.statusCode)}"/>
+ <g:set var="isInbound" value="${stockMovement?.destination == currentLocation}" />


You could (should) use a stockMovementDirection here (but first look at the comment from the line below)

I could for the StockMovement, but could not for the OutboundStockMovement, as this property/method is missing there.

awalkowiak · 2024-04-17T11:35:05Z

grails-app/migrations/views/stock-movement-list-item.sql

@@ -51,29 +51,29 @@ CREATE OR REPLACE VIEW stock_movement_list_item AS
 s.id AS shipment_id,
 s.current_status AS shipment_status,
 CASE


I wonder if we should not move away from this part and move it to the StockMovement/OutboundStockMovement/StockMovementListItem classes, but I think this is part of @drodzewicz spike

For now I just went with what was requested in the ticket, so to map the order statuses to different statuses than to what they were mapped until now.

awalkowiak · 2024-04-17T11:35:16Z

grails-app/migrations/views/stock-movement-list-item.sql

- WHEN o.status = 'COMPLETED' THEN 'DISPATCHED'
+ WHEN o.status = 'PARTIALLY_RECEIVED' THEN 'ISSUED'
+ WHEN o.status = 'RECEIVED' THEN 'ISSUED'
+ WHEN o.status = 'COMPLETED' THEN 'ISSUED'
 WHEN o.status = 'REJECTED' THEN 'CANCELED'
 ELSE 'PENDING'
 END AS status_code,
 NULL AS requisition_id,
 CASE


Same comment as above

Holy shit. This makes me very nervous and is the antithesis of what I wanted to do with these tickets. This is "possible long-term" refactor, not "just get it working" refactor.

As for this change I’m pretty confident, since this part of the code is responsible only for returns.

awalkowiak · 2024-04-17T11:35:25Z

grails-app/migrations/views/stock-movement.sql

@@ -77,27 +77,27 @@ CREATE OR REPLACE VIEW stock_movement AS
 s.current_status AS shipment_status,
 rn.identifier AS tracking_number,
 CASE


Same comment as above

awalkowiak · 2024-04-17T11:35:31Z

grails-app/migrations/views/stock-movement.sql

- WHEN o.status = 'COMPLETED' THEN 'DISPATCHED'
+ WHEN o.status = 'PARTIALLY_RECEIVED' THEN 'ISSUED'
+ WHEN o.status = 'RECEIVED' THEN 'ISSUED'
+ WHEN o.status = 'COMPLETED' THEN 'ISSUED'
 WHEN o.status = 'REJECTED' THEN 'CANCELED'
 ELSE 'PENDING'
 END AS status_code,
 NULL AS requisition_id,
 CASE


Same comment as above

jmiranda

Aside: The Tech Hurdle: State Machine document that I'm drafting is going to propose a change that will allow us to (hopefully) represent all of the state machines in the system using a library like Spring Statemachine https://docs.spring.io/spring-statemachine/docs/4.0.0/reference/index.html#crashcourse. We might keep the enums, but this will allow us to define all of the states and transitions of each state machine. That means we'll be calling the state machine API to check whether we can transition to the next state rather than implementing that using a switch statement.

jmiranda · 2024-04-17T21:25:54Z

grails-app/migrations/views/stock-movement-list-item.sql

- WHEN o.status = 'COMPLETED' THEN 'DISPATCHED'
+ WHEN o.status = 'PARTIALLY_RECEIVED' THEN 'ISSUED'
+ WHEN o.status = 'RECEIVED' THEN 'ISSUED'
+ WHEN o.status = 'COMPLETED' THEN 'ISSUED'
 WHEN o.status = 'REJECTED' THEN 'CANCELED'
 ELSE 'PENDING'
 END AS status_code,
 NULL AS requisition_id,
 CASE


Holy shit. This makes me very nervous and is the antithesis of what I wanted to do with these tickets. This is "possible long-term" refactor, not "just get it working" refactor.

jmiranda · 2024-04-17T21:33:28Z

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

@@ -29,8 +29,9 @@ enum StockMovementStatusCode {
 PACKED(7, PENDING),
 REVIEWING(8, PENDING),
 DISPATCHED(9),
+ ISSUED(9),


I don't really understand why we're replacing DISPATCHED, but if you're going to replace DISPATCHED with ISSUED then you need to go 💯. DISPATCHED was the name of the status I gave for the stock movement state machine to differentiate it from the requisition. A requisition is ISSUED. In the case of an outbound shipment when the requisition is ISSUED it's also DISPATCHED as part of a shipment. We could have also used SHIPPED but I wanted to be generic in case we were dispatching to a ward or pharmacy within the hospital, in which case there isn't a need for shipment metadata.

What is the reasoning behind adding an ISSUED enum here? Would this be rectified by changing the localized message rendered for DISPATCHED state? If not, can you elaborate on what we're trying to accomplish.

Reminder: We want to change as little as possible.

It was a request in the ticket, that they want a different mapping for particular order statuses. Manon has provided me a table with exact statuses we want to display for a particular order status. If you read the comments in the ticket and also the description, everything should be self-explanatory.
Why I added the ISSUED to the enum is because of the fact, that I was getting an exception on the list page when trying to sort, and where eventually one of the 10 items has the ISSUED status. The exception was „No enum value [ISSUED] for StockMovementStatusCode”

OBPIH-6215 Refactor returns statuses (both inbound and outbound) and …

6d4d277

…align them on list page<->view page

kchelstowski requested review from EWaterman, awalkowiak, drodzewicz and alannadolny April 17, 2024 10:55

kchelstowski self-assigned this Apr 17, 2024

awalkowiak reviewed Apr 17, 2024

View reviewed changes

jmiranda reviewed Apr 17, 2024

View reviewed changes

kchelstowski closed this Apr 19, 2024

drodzewicz mentioned this pull request May 16, 2024

OBPIH-6368 Fix statuses mapping on outbound list page while filtering after state machine refactor + remove unused filters #4624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-6215 Refactor returns statuses (both inbound and outbound) and … #4586

OBPIH-6215 Refactor returns statuses (both inbound and outbound) and … #4586

kchelstowski commented Apr 17, 2024

awalkowiak left a comment

awalkowiak Apr 17, 2024

kchelstowski Apr 17, 2024

awalkowiak Apr 17, 2024

kchelstowski Apr 17, 2024 •

edited

Loading

awalkowiak Apr 17, 2024

kchelstowski Apr 17, 2024

awalkowiak Apr 17, 2024

kchelstowski Apr 17, 2024

awalkowiak Apr 17, 2024

jmiranda Apr 17, 2024

kchelstowski Apr 18, 2024

awalkowiak Apr 17, 2024

awalkowiak Apr 17, 2024

jmiranda left a comment

jmiranda Apr 17, 2024

jmiranda Apr 17, 2024

jmiranda Apr 17, 2024

kchelstowski Apr 18, 2024

OBPIH-6215 Refactor returns statuses (both inbound and outbound) and … #4586

OBPIH-6215 Refactor returns statuses (both inbound and outbound) and … #4586

Conversation

kchelstowski commented Apr 17, 2024

awalkowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Apr 17, 2024 •

edited

Loading