OBPIH-5981 Improve redirect for e-request edit action for a requestor… by kchelstowski · Pull Request #4423 · openboxes/openboxes

kchelstowski · 2023-12-15T16:40:03Z

… in a ward location (request approval feature) + refactor redirect logic for SM edit action

Since it's targeting the 0.9.1, and I had to touch this code, I decided to refactor this part to improve the readability of the redirect logic which was like this:

if(stockMovement.isReturn) {
    redirect(controller: "stockTransfer", action: "edit", params: params)
} else if (stockMovement?.getStockMovementDirection(currentLocation) == StockMovementDirection.OUTBOUND && stockMovement?.requisition?.sourceType == RequisitionSourceType.ELECTRONIC) {
    redirect(action: "verifyRequest", params: params)
}
else if (stockMovement?.getStockMovementDirection(currentLocation) == StockMovementDirection.INBOUND) {

    if (stockMovement.isFromOrder) {
        redirect(action: "createCombinedShipments", params: params)
    } else if (stockMovement.requisition?.sourceType == RequisitionSourceType.ELECTRONIC) {
        if (stockMovement.requisition?.status == RequisitionStatus.CREATED) {
            redirect(action: "createRequest", params: params)
        } else {
            redirect(action: "verifyRequest", params: params)
        }
    } else {
        redirect(action: "createInbound", params: params)
    }
}
else {
    if (stockMovement.isFromOrder) {
        redirect(action: "createCombinedShipments", params: params)
    }
    redirect(action: "createOutbound", params: params)
}

and after my changes (I paste it since the changes in the github might be difficult to read):

if (stockMovement.isReturn) {
    redirect(controller: "stockTransfer", action: "edit", params: params)
    return
}

StockMovementDirection stockMovementDirection = stockMovement?.getStockMovementDirection(currentLocation)

// Redirect logic for the e-request
if (stockMovement?.electronicType) {
    switch (stockMovementDirection) {
        case StockMovementDirection.OUTBOUND:
            redirect(action: "verifyRequest", params: params)
            break
        case StockMovementDirection.INBOUND:
            User currentUser = AuthService.currentUser
            RequisitionStatus requisitionStatus = stockMovement.requisition?.status
            // If a request's status is created or a user is a requestor logged in, in a location
            // with submit request and without manage inventory, and the request has waiting for approval
            // status, we want to redirect to add items page (OBPIH-5981)
            if (requisitionStatus == RequisitionStatus.CREATED
                || (userService.isUserRequestor(currentUser)
                    && currentLocation.downstreamConsumer
                    && requisitionStatus == RequisitionStatus.PENDING_APPROVAL)) {
                        redirect(action: "createRequest", params: params)
                        return
            }
            redirect(action: "verifyRequest", params: params)
            break
        default:
            break
    }
    return
}

// Redirect logic for the combined shipments feature
if (stockMovement.isFromOrder) {
    redirect(action: "createCombinedShipments", params: params)
    return
}

// Redirect logic for the regular stock movement feature
switch (stockMovementDirection) {
    case StockMovementDirection.OUTBOUND:
        redirect(action: "createOutbound", params: params)
        break
    case StockMovementDirection.INBOUND:
        redirect(action: "createInbound", params: params)
        break
    default:
        break
}

the part that is the scope of the ticket is this:

if (requisitionStatus == RequisitionStatus.CREATED
      || (userService.isUserRequestor(currentUser)
          && currentLocation.downstreamConsumer
          && requisitionStatus == RequisitionStatus.PENDING_APPROVAL)) {
              redirect(action: "createRequest", params: params)
              return
  }

… in a ward location (request approval feature) + refactor redirect logic for SM edit action

drodzewicz

Thanks for doing the refactor, I had to dos some mental gymnastics to compare the old and the new versions of this redirect logic and it wasn't easy.

In general I think it looks good and does cover all of the old cases, but please take a look at the empty default cases, I think there should be something in case we land in that default case, as unlikely as it may seem.

drodzewicz · 2023-12-18T12:43:13Z

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

+                default:
+                    break
            }
+            return
        }


something feels off here, I feel like this should have some default redirect

drodzewicz · 2023-12-18T12:45:35Z

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

+            default:
+                break


Same here, I think a default redirect should be present here.
getStockMovementDirection can return a null value and there is no redirect handler at the end, so I think the app would crash because no redirects are specified at the end.

kchelstowski · 2023-12-18T13:15:37Z

@drodzewicz I've had the same feeling about it and expected someone of you to point that out, because I have not had idea what the default redirect should be, because as far as I'm concerned, the old code did not have a special handling for that case, did it?
I'm open for suggestions about the default redirect.

drodzewicz · 2023-12-18T15:41:00Z

Was playing around with rearranging these if statements to avoid any more indentation to make the code more readable at a glance and came to this version.
What do you think @kchelstowski ?

if (stockMovement?.electronicType) {
    switch (stockMovementDirection) {
        case StockMovementDirection.INBOUND:
            User currentUser = AuthService.currentUser
            RequisitionStatus requisitionStatus = stockMovement.requisition?.status
            // If a request's status is created or a user is a requestor logged in, in a location
            // with submit request and without manage inventory, and the request has waiting for approval
            // status, we want to redirect to add items page (OBPIH-5981)
            if (requisitionStatus == RequisitionStatus.CREATED) {
                redirect(action: "createRequest", params: params)
                return
            }
            if (requisitionStatus == RequisitionStatus.PENDING_APPROVAL 
                && userService.isUserRequestor(currentUser)
                && currentLocation.downstreamConsumer
             ) {
                 redirect(action: "createRequest", params: params)
                 return
            }
            break
        case StockMovementDirection.OUTBOUND:
            break
        default:
            break
    }
    redirect(action: "verifyRequest", params: params)
    return
}

kchelstowski · 2023-12-18T16:08:10Z

@drodzewicz thanks for the proactivity. I agree separating the CREATED/PENDING_APPROVAL statements is more readable even though we repeat the createRequest redirect. I didn't want to repeat the code, since they redirect to the same thing, but we should probably balance readability vs repetition of the code.
I'm fine with the version proposed by you for the CREATED/PENDING_APPROVAL case although I'd say this part feels weird to me:

case StockMovementDirection.OUTBOUND:
            break

at first sight it would mean that we don't care about the OUTBOUND at all even though we do the redirect a few lines below.
Then maybe we could take one more step and get rid of this case at all?

if (stockMovement?.electronicType) {
    switch (stockMovementDirection) {
        case StockMovementDirection.INBOUND:
            User currentUser = AuthService.currentUser
            RequisitionStatus requisitionStatus = stockMovement.requisition?.status
            // If a request's status is created or a user is a requestor logged in, in a location
            // with submit request and without manage inventory, and the request has waiting for approval
            // status, we want to redirect to add items page (OBPIH-5981)
            if (requisitionStatus == RequisitionStatus.CREATED) {
                redirect(action: "createRequest", params: params)
                return
            }
            if (requisitionStatus == RequisitionStatus.PENDING_APPROVAL 
                && userService.isUserRequestor(currentUser)
                && currentLocation.downstreamConsumer
             ) {
                 redirect(action: "createRequest", params: params)
                 return
            }
            redirect(action: "verifyRequest", params: params)
            break
        default:
            redirect(action: "verifyRequest", params: params)
    }
    return
}

Now I've even started to realize if the switch/case makes sense here. What do you think?

it'd look like this:

if (stockMovement?.electronicType) {
    if (stockMovementDirection == StockMovementDirection.INBOUND) {
        User currentUser = AuthService.currentUser
        RequisitionStatus requisitionStatus = stockMovement.requisition?.status
        // If a request's status is created or a user is a requestor logged in, in a location
        // with submit request and without manage inventory, and the request has waiting for approval
        // status, we want to redirect to add items page (OBPIH-5981)
        if (requisitionStatus == RequisitionStatus.CREATED) {
            redirect(action: "createRequest", params: params)
            return
        }
        if (requisitionStatus == RequisitionStatus.PENDING_APPROVAL 
            && userService.isUserRequestor(currentUser)
            && currentLocation.downstreamConsumer
         ) {
             redirect(action: "createRequest", params: params)
             return
        }
     }
     redirect(action: "verifyRequest", params: params)
     return
}

drodzewicz · 2023-12-19T07:21:49Z

@kchelstowski looks good, let's proceed with that

kchelstowski · 2023-12-19T16:52:24Z

@awalkowiak @jmiranda do you also agree with our latest proposition? I want to make sure everyone agrees, before I make the change.

jmiranda

My first thought when seeing this was "rendering a view should not be this complicated".

Can we reduce the complexity by creating separate URLs for the different types of requests / transfers and not using the stock movement controller as a central hub? If not, is there another way to reduce the complexity.

kchelstowski · 2023-12-22T15:05:45Z

@jmiranda since for "Edit stock movement" our url is /stockMovement/edit and it's currently handled in the method I've refactored, to do it like you suggested, we would actually have to add a similar in terms of complexity logic in the show.gsp view of the stock movement, to determine what url the "Edit stock movement" should have.

Since the current else-if logic drove us nuts and I had to add one more case there, I decided to refactor it a bit, so it becomes more readable.
Let me know if it's fine for now or if we'd want to have a different approach there.

awalkowiak

I am very "pro" these changes. I think this part was getting worse and worse.

@kchelstowski lets finalize the refactoring that you started, if there are any improvements left from Dariusz's review. And please make double check locally on every branch we had previously, that it is still working correctly.

@jmiranda I think that the issue here is that we rather wanted to keep various workflows under one stock movement. And it kept growing and expanding. I think ideally we could think about the separation of the pages or other type of the refactor, but there are to many of "what ifs". For now I would go with this cleanup for 0.9.1, since it is already done, and I think looks good. We can come back to this chunk of code, as tech debt and discuss on the tech huddle. I feel we might run into few use cases from past, where we get some answers why its like that. We need to dig it out a bit from tickets and perhaps test/use cases.

OBPIH-5981 Improve redirect for e-request edit action for a requestor…

a3a62c5

… in a ward location (request approval feature) + refactor redirect logic for SM edit action

kchelstowski requested review from alannadolny, awalkowiak, drodzewicz and jmiranda December 15, 2023 16:40

kchelstowski self-assigned this Dec 15, 2023

kchelstowski added this to the 0.9.1 milestone Dec 15, 2023

drodzewicz requested changes Dec 18, 2023

View reviewed changes

jmiranda reviewed Dec 21, 2023

View reviewed changes

awalkowiak reviewed Dec 22, 2023

View reviewed changes

OBPIH-5981 Refactor electronicType statement

b550018

drodzewicz approved these changes Jan 9, 2024

View reviewed changes

awalkowiak approved these changes Jan 9, 2024

View reviewed changes

awalkowiak merged commit b8e11d8 into feature/upgrade-to-grails-3.3.10 Jan 9, 2024

awalkowiak deleted the OBPIH-5981 branch January 9, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OBPIH-5981 Improve redirect for e-request edit action for a requestor…#4423

OBPIH-5981 Improve redirect for e-request edit action for a requestor…#4423
awalkowiak merged 2 commits intofeature/upgrade-to-grails-3.3.10from
OBPIH-5981

kchelstowski commented Dec 15, 2023

Uh oh!

drodzewicz left a comment

Uh oh!

drodzewicz Dec 18, 2023

Uh oh!

drodzewicz Dec 18, 2023

Uh oh!

kchelstowski commented Dec 18, 2023 •

edited

Loading

Uh oh!

drodzewicz commented Dec 18, 2023

Uh oh!

kchelstowski commented Dec 18, 2023 •

edited

Loading

Uh oh!

drodzewicz commented Dec 19, 2023

Uh oh!

kchelstowski commented Dec 19, 2023

Uh oh!

jmiranda left a comment •

edited

Loading

Uh oh!

kchelstowski commented Dec 22, 2023

Uh oh!

awalkowiak left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

kchelstowski commented Dec 15, 2023

Uh oh!

drodzewicz left a comment

Choose a reason for hiding this comment

Uh oh!

drodzewicz Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

drodzewicz Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

kchelstowski commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodzewicz commented Dec 18, 2023

Uh oh!

kchelstowski commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodzewicz commented Dec 19, 2023

Uh oh!

kchelstowski commented Dec 19, 2023

Uh oh!

jmiranda left a comment • 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 commented Dec 22, 2023

Uh oh!

awalkowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kchelstowski commented Dec 18, 2023 •

edited

Loading

kchelstowski commented Dec 18, 2023 •

edited

Loading

jmiranda left a comment •

edited

Loading