OBPIH-6215 Refactor returns statuses (both inbound and outbound) and … by kchelstowski · Pull Request #4586 · openboxes/openboxes

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


    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

                            <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

                            </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

            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

            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

            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

    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 alannadolny, awalkowiak, drodzewicz and ewaterman 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

Uh oh!

Conversation

kchelstowski commented Apr 17, 2024

Uh oh!

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

kchelstowski Apr 17, 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!

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kchelstowski Apr 17, 2024 •

edited

Loading