OBPIH-5806 approval and reject action approvals end to end workflow #4293

drodzewicz · 2023-09-22T13:05:54Z

No description provided.

kchelstowski

overall good job, those are only "beautify" suggestions

kchelstowski · 2023-09-25T07:08:22Z

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

+    def updateStatus = {
+        StockMovement stockMovement = stockMovementService.getStockMovement(params.id)
+
+        try {


I really don't like throwing that try/catch everywhere - lately we've been doing that very much. Just wanted to mention that a potential rollback might not take place in Grails 1 when we catch exceptions.
Is it in requirements that after an exception we want to show the same view page?

It was not mentioned if it should be the same page so it was my own interpretation.

It was mentioned that reject and approve buttons should be disabled if a request is already approved/rejected.
This try catch was more like a failsafe I added so I think I can remove it and let the generic handler do it's job and render generic error page.

kchelstowski · 2023-09-25T07:12:43Z

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

+            return
+        }
+
+        redirect(action: "list", params: ["flash": flash as JSON, sourceType: "ELECTRONIC", direction: "OUTBOUND"])


Suggested change

redirect(action: "list", params: ["flash": flash as JSON, sourceType: "ELECTRONIC", direction: "OUTBOUND"])

redirect(action: "list", params: ["flash": flash as JSON, sourceType: RequisitionSourceType.ELECTRONIC, direction: StockMovementDirection.OUTBOUND.toUpperCase()])

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

kchelstowski · 2023-09-25T07:16:47Z

grails-app/services/org/pih/warehouse/core/UserService.groovy

@@ -236,6 +236,13 @@ class UserService {
        return isSuperuser(currentUser) || (currentUser.getHighestRole(location) >= otherUser.getHighestRole(location))
    }

+    Boolean userHasRoles(String userId, Collection roleTypes, String locationId) {


I see an inconsistency here - you return the method you've created in the domain, but the return types are different - here it is Boolean and in the domain it is boolean - I think it should be boolean here as well.

yes, it should be boolean

grails-app/services/org/pih/warehouse/core/UserService.groovy

kchelstowski · 2023-09-25T07:26:44Z

grails-app/services/org/pih/warehouse/requisition/RequisitionService.groovy

+                Event event = createEvent(EventCode.REJECTED, requisition.origin, new Date(), currentUser)
+                requisition.addToEvents(event)
+                requisition.save()
+                return


I think this could be refactored. We have almost the same content in all of the statements, hence we could create a helper method to handle that, something like:

Requisition transitionRequisitionStatus(Requisition requisition, RequisitionStatus requisitionStatus, EventCode eventCode, User currentUser) { requisition.dateApproved = new Date() requisition.approvedBy = currentUser requisition.status = requisitionStatus Event event = createEvent(eventCode, requisition.origin, new Date(), currentUser) requisition.addToEvents(event) requisition.save() }

and here:

if (status == RequisitionStatus.PENDING_APPROVAL) { if (requisition.origin.approvalRequired) { Event event = createEvent(EventCode.PENDING_APPROVAL, requisition.origin, new Date(), currentUser) requisition.addToEvents(event) requisition.status = RequisitionStatus.PENDING_APPROVAL requisition.approvalRequired = true requisition.save() return } Event event = createEvent(EventCode.SUBMITTED, requisition.origin, new Date(), currentUser) requisition.addToEvents(event) requisition.status = RequisitionStatus.VERIFYING requisition.approvalRequired = false requisition.save() return } if (status == RequisitionStatus.APPROVED) { transitionRequisitionStatus(requisition, RequisitionStatus.APPROVED, EventCode.APPROVED, currentUser) return } if (status == RequisitionStatus.REJECTED) { transitionRequisitionStatus(requisition, RequisitionStatus.REJECTED, EventCode.REJECTED, currentUser) return }

we could go even further and you could refactor also the statements I've made with one minor change:

if (status == RequisitionStatus.PENDING_APPROVAL) { if (requisition.origin.approvalRequired) { Requisition updatedRequisition = transitionRequisitionStatus(requisition, RequisitionStatus.PENDING_APPROVAL, EventCode.PENDING_APPROVAL, currentUser) updatedRequisition.approvalRequired = true // requisition.save() <--- notice I've commented that, it shouldn't be necessary anymore, because the event which has not been yet persisted has been already saved in the transition method, hence another save should not be necessary and the approvalRequired flag should be persisted at the end of the session, but please, double check that. return } Requisition updatedRequisition = transitionRequisitionStatus(requisition, RequisitionStatus.VERIFYING, EventCode.SUBMITTED, currentUser) requisition.approvalRequired = false return } if (status == RequisitionStatus.APPROVED) { transitionRequisitionStatus(requisition, RequisitionStatus.APPROVED, EventCode.APPROVED, currentUser) return } if (status == RequisitionStatus.REJECTED) { transitionRequisitionStatus(requisition, RequisitionStatus.REJECTED, EventCode.REJECTED, currentUser) return }

It would be great to move some common logic into some other method but the reason why I didn't do it at first was that there a slight differences in each of the status changes like in example of a APPROVED we assign approvedBy but in case of the REJECETD we must assign rejectedBy.
These are small differences so a common method should work but I'll think about if there is a better implementation for all of this

then you can just follow the path I've shown with my statements with the approvalRequired - so just from this method return the common stuff, and then just operate on it like:

Requisition updatedRequisition = transitionRequisitionStatus(...) requisition.rejectedBy = currentUser

I would also be happy with a switch statement for the top level status check

switch (status) { case PENDING_APPROVAL: doPendingApprovalStuff() return case APPROVED: transitionRequisitionStatus(requisition, RequisitionStatus.APPROVED, EventCode.APPROVED, currentUser) return ... }

requisition.save() <--- notice I've commented that, it shouldn't be necessary anymore, because the event which has not been yet persisted has been already saved in the transition method, hence another save should not be necessary and the approvalRequired flag should be persisted at the end of the session, but please, double check that.

Not sure if that'll be true on Grails 3.

I like the switch case a lot more

@jmiranda

Not sure if that'll be true on Grails 3.

yes, it will, because it's Hibernate's mechanism, not Grails'

grails-app/views/stockMovement/show.gsp

kchelstowski · 2023-09-25T07:31:59Z

grails-app/views/stockMovement/show.gsp

-                        <img src="${resource(dir: 'images/icons/silk', file: 'arrow_rotate_anticlockwise.png')}" />&nbsp;
-                        <warehouse:message code="stockMovement.rollbackLastReceipt.label" />
+            <g:set var="userHasRequestApproverRole" value="${false}"/>
+            <g:userHasRoles location="${stockMovement?.origin?.id}" roles="${[RoleType.ROLE_REQUISITION_APPROVER]}">


I know @jmiranda is not a big fan of that, but I don't know if I see any other possible solution at the moment
But I like the fact, that you've actually checked it once, and assigned it to a variable, so we wouldn't have to call this <g:userHasRoles> in many places, like we do for <g:isSuperuser> in many places

Yeah, I hate this paradigm, but I think I'm the one who made it popular so I don't blame @drodzewicz.

What is interesting is that the <g:set /> actually gets invoked with that. I guess that makes sense, but I assumed g:if did something special. I guess it's just that GSP is rendered/invoked recursively so whether the next tag is rendering or setting doesn't really matter ... it just gets invoked. But it would be great if someone investigated the Grails tab library where "g:if" is defined to see what they do in this case.

And as I said above, I would prefer ifUserHasRoles over userHasRoles.

It probably needs to be done as a tech debt refactoring but we probably should have thought about converging our permission/supports taglibs with something closer to Spring Security (like using authorize with logic expressions).

kchelstowski · 2023-09-25T07:34:08Z

grails-app/views/stockMovement/show.gsp

+                    <img src="${resource(dir: 'images/icons/', file: 'handtruck.png')}" />&nbsp;
+                    <warehouse:message code="default.button.receive.label" />
+                </g:link>
+                <g:isUserAdmin>


I built you up in the comment above for assigning it to a variable, but here I can see that you call the <g:isUserAdmin> a few times - maybe it'd be good from the performance POV to do it also in this place?

btw... I don't know if it'd be a good direction in the future, but why can't we just return user roles in the model while rendering the view?... it's a "loud thinking", but I'll leave this comment here in case anyone wanted to comment on that.

this is old code whcih github highlighted as a new change since I wrapped a <g:if> and <g:userHasRoles> around it

Can someone add to the tech debt backlog an investigation into the right way to do these types of tag libs? It should be under Security and Performance. We invoke things like "isInRole" all the time throughout the code and I know it degrades performance. But what I don't understand is how Spring Security gets away with it. Do all of the security invocations go through the thread local?

alannadolny · 2023-09-25T10:10:21Z

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

+            switch(stockMovement.requisition?.status) {
+                case RequisitionStatus.APPROVED:
+                    flash.message = g.message(
+                            code: "request.approved.message.label",
+                            default: "You have successfully approved the request {0}",
+                            args: [stockMovement.id]
+                    )
+                    break
+                case RequisitionStatus.REJECTED:
+                    flash.message = g.message(
+                            code: "request.rejected.message.label",
+                            default: "You have successfully rejected the request {0}",
+                            args: [stockMovement.id]
+                    )
+                    break
+                default:
+                    break
+            }


Maybe we can shorten it to:

if (stockMovement.requisition?.status in [RequisitionStatus.APPROVED, RequisitionStatus.REJECTED]) { String status = stockMovement.requisition?.status?.toLowerCase() flash.message = g.message( code: "request.${status}.message.label", default: "You have successfully ${status} the request {0}", args: [stockMovement.id] ) }

but if you don't like this solution, let's leave it as is

kchelstowski · 2023-09-26T10:26:53Z

grails-app/services/org/pih/warehouse/requisition/RequisitionService.groovy

+                requisition.dateApproved = new Date()
+                requisition.approvedBy = currentUser
+                return
+            } else if (requisition.status == RequisitionStatus.REJECTED) {


nitpicky: else is not required if you call return above (take a look at my suggestion below)

I'd rather remove the return than the else.

jmiranda

LGTM. I can't remember if I requested any changes, so I'll leave this as a comment for now.

jmiranda · 2023-09-26T19:44:35Z

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

+    def updateStatus = {
+        StockMovement stockMovement = stockMovementService.getStockMovement(params.id)
+
+        stockMovementService.transitionStockMovement(stockMovement, params as JSONObject)


Hmm, this sort of highlights a bad decision I made when we first created the stock movement API. I'm a dum dum. JSONObject is a web layer class so it's not meant to be passed to the service layer. I'm guessing we've noted this as a tech debt somewhere in the past, but if not we should think about it adding it to our tech debt backlog now.

jmiranda · 2023-09-26T19:54:49Z

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

+        stockMovementService.transitionStockMovement(stockMovement, params as JSONObject)
+
+        if (stockMovement.requisition?.status in [RequisitionStatus.APPROVED, RequisitionStatus.REJECTED]) {
+            String status = stockMovement.requisition?.status


I'm not sure what you're trying to do here with the dynamic coercion, but I would assume you'd need to use toLowerCase() here to get the proper message from messages.properties. It's possible that the message is resolved properly only because you set a default message.

jmiranda · 2023-09-26T19:56:17Z

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

+        // check if status is either approved or rejecetd
+            // if no then throw error and return a message that it can't be rolledback
+
+        // TODO: to be implemented


It would be good to throw an UnsupportedOperationException (or NotImplementedException) here until it's implemented.

jmiranda · 2023-09-26T19:58:35Z

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

+        }
+
+        redirect(action: "list", params: [
+                "flash"     : flash as JSON,


Can you explain why this is necessary? I assume you're not seeing the flash.message on the list page due to the redirect, but this doesn't seem like the right approach to that problem.

Once you explain what you're encountering I'll try to come up with a more elegant solution.

the redirect is directing us to react page for request list page.
There on the frontend we have a handler that extracts flash object from the parameters and renders a message or error depending on what parameters are set on the flash.

I was following an example from the method above for def remove()

@jmiranda This is our workaround to get messages from "grails" pages to "react" pages (when it is done through a redirect). This won't be needed if, in some bright future, we separate frontend from backend (read: we finally get rid of gsp templates and server-side rendering).

jmiranda · 2023-09-26T20:03:31Z

grails-app/services/org/pih/warehouse/core/UserService.groovy

@@ -236,6 +236,13 @@ class UserService {
        return isSuperuser(currentUser) || (currentUser.getHighestRole(location) >= otherUser.getHighestRole(location))
    }

+    boolean userHasRoles(String userId, Collection roleTypes, String locationId) {


Use hasRoles as the name here too. Although, I'd also be fine with

doesUserHaveRoles

isUserInRoles

isUserInAllRoles

isUserInAnyRole

jmiranda · 2023-09-26T20:43:48Z

grails-app/views/stockMovement/show.gsp

-                        <img src="${resource(dir: 'images/icons/silk', file: 'arrow_rotate_anticlockwise.png')}" />&nbsp;
-                        <warehouse:message code="stockMovement.rollbackLastReceipt.label" />
+            <g:set var="userHasRequestApproverRole" value="${false}"/>
+            <g:userHasRoles location="${stockMovement?.origin?.id}" roles="${[RoleType.ROLE_REQUISITION_APPROVER]}">


It probably needs to be done as a tech debt refactoring but we probably should have thought about converging our permission/supports taglibs with something closer to Spring Security (like using authorize with logic expressions).

jmiranda · 2023-09-26T20:46:08Z

grails-app/views/stockMovement/show.gsp

+                    <img src="${resource(dir: 'images/icons/', file: 'handtruck.png')}" />&nbsp;
+                    <warehouse:message code="default.button.receive.label" />
+                </g:link>
+                <g:isUserAdmin>


Can someone add to the tech debt backlog an investigation into the right way to do these types of tag libs? It should be under Security and Performance. We invoke things like "isInRole" all the time throughout the code and I know it degrades performance. But what I don't understand is how Spring Security gets away with it. Do all of the security invocations go through the thread local?

jmiranda · 2023-09-26T20:49:58Z

grails-app/views/stockMovement/show.gsp

+                    </g:if>
+                    <g:elseif test="${stockMovement?.hasBeenIssued() || ((stockMovement?.hasBeenShipped() ||
+                            stockMovement?.hasBeenPartiallyReceived()) && stockMovement?.isFromOrder)}">
+                        <g:link controller="stockMovement" action="rollback" id="${stockMovement.id}" class="button">


@drodzewicz Are you saying that all of the green code is old? If not we definitely need to add another tech debt item for cleaning up our logic because this is getting out of hand.

At the very least we need to push more of this logic into the stock movement.

by pushing more of this logic to stock Movement do you mean pushing authorization logic there, like eg. stockMovement.approverAuhtorized(user) stockMovement.canEddit(user)?

jmiranda · 2023-09-26T20:55:59Z

grails-app/views/stockMovement/show.gsp

                    >
-                        <img src="${resource(dir: 'images/icons/silk', file: 'delete.png')}" />&nbsp;
-                        <warehouse:message code="default.button.delete.label" />
+                        <img src="${resource(dir: 'images/icons/silk', file: 'accept.png')}" />&nbsp;


Is this the only place we use this icon on this page? If so accept.png (approved) and decline.png (rejected) should work perfectly. Otherwise, we might want to consider using a different pair (like tick.png, cross.png).

jmiranda · 2023-09-26T20:56:36Z

grails-app/views/stockMovement/show.gsp

-                        <img src="${resource(dir: 'images/icons/silk', file: 'delete.png')}" />&nbsp;
-                        <warehouse:message code="default.button.delete.label" />
+                        <img src="${resource(dir: 'images/icons/silk', file: 'accept.png')}" />&nbsp;
+                        <g:message code="request.approval.approve.label"  default="approve" />


Why lowecase?

…lizationException in a published event

…gin which supports approvals and for users that have roletype request approver

remove catching excetion on updateStatus and let generic error handler catch it

- refactor method names - refactor triggerRequisitionStatusTransition to use switch case instead of if

awalkowiak

@drodzewicz two small requests from my side, other than that I think it LGTM

awalkowiak · 2023-09-27T11:32:54Z

grails-app/services/org/pih/warehouse/requisition/RequisitionService.groovy

-                requisition.status = RequisitionStatus.VERIFYING
-                requisition.approvalRequired = false
-                requisition.save()
+            switch(requisition.status) {


@drodzewicz Perhaps a comment here would be handy? Please add a note here about why we have various options for status transition here, with a link to request approvals epic ticket.

awalkowiak · 2023-09-27T11:37:44Z

grails-app/taglib/org/pih/warehouse/AuthTagLib.groovy

@@ -88,6 +88,12 @@ class AuthTagLib {
            out << body()
    }

+    def ifUserHasRoles = {attrs, body ->


Please for now follow here the convention of isUserInRole and make this one isUserInAllRoles instead ifUserHasRoles

…4293) * OBPIH-5803 Change fetchMode of Requisition events to avoid LazyInitializationException in a published event * OBPIH-5803 Include status transition in RequisitionStatusTransitionEvent * OBPIH-5803 Add createdBy field for Event domain * OBPIH-5803 Fix assigning createdBy to an Event * OBPIH-5803 Fixes after review * OBPIH-5806 Render approve and reject buttons on request show page * OBPIH-5806 Implement approve and reject status changes logic * OBPIH-5806 Fix messages after updaing request status * OBPIH-5806 Render approve and reject buttons only on request with origin which supports approvals and for users that have roletype request approver * OBPIH-5806 Fix after rebase * OBPIH-5806 Render only reject, approve and rollback buttons to approver * OBPIH-5806 Rafactor requets state transition function remove catching excetion on updateStatus and let generic error handler catch it * OBPIH-5806 use requisition.status instead of status * OBPIH-5806 Fixes after review - refactor method names - refactor triggerRequisitionStatusTransition to use switch case instead of if * OBPIH-5806 Rename AuthTagLib method and add comment on status transition --------- Co-authored-by: kchelstowski <kchelstowski@soldevelo.com>

Base automatically changed from OBPIH-5803 to develop September 22, 2023 13:30

drodzewicz force-pushed the OBPIH-5806-approval-and-reject-action-approvals-end-to-end-workflow branch 2 times, most recently from e6c9763 to 450cac5 Compare September 22, 2023 15:21

drodzewicz self-assigned this Sep 25, 2023

drodzewicz requested review from kchelstowski, awalkowiak and alannadolny September 25, 2023 07:05

drodzewicz marked this pull request as ready for review September 25, 2023 07:05

kchelstowski requested changes Sep 25, 2023

View reviewed changes

alannadolny reviewed Sep 25, 2023

View reviewed changes

drodzewicz force-pushed the OBPIH-5806-approval-and-reject-action-approvals-end-to-end-workflow branch from 57a57db to b0dd4e1 Compare September 25, 2023 13:09

drodzewicz requested a review from kchelstowski September 25, 2023 13:09

alannadolny approved these changes Sep 25, 2023

View reviewed changes

drodzewicz requested a review from jmiranda September 26, 2023 09:29

drodzewicz force-pushed the OBPIH-5806-approval-and-reject-action-approvals-end-to-end-workflow branch from b0dd4e1 to 177d3ab Compare September 26, 2023 10:16

kchelstowski reviewed Sep 26, 2023

View reviewed changes

jmiranda reviewed Sep 26, 2023

View reviewed changes

kchelstowski and others added 13 commits September 27, 2023 10:56

OBPIH-5803 Change fetchMode of Requisition events to avoid LazyInitia…

da03735

…lizationException in a published event

OBPIH-5803 Include status transition in RequisitionStatusTransitionEvent

2ce4ed3

OBPIH-5803 Add createdBy field for Event domain

31a38d3

OBPIH-5803 Fix assigning createdBy to an Event

6c5ae62

OBPIH-5803 Fixes after review

3167577

OBPIH-5806 Render approve and reject buttons on request show page

863da4d

OBPIH-5806 Implement approve and reject status changes logic

6341a8f

OBPIH-5806 Fix messages after updaing request status

2e338fa

OBPIH-5806 Render approve and reject buttons only on request with ori…

54f2cc7

…gin which supports approvals and for users that have roletype request approver

OBPIH-5806 Fix after rebase

bade116

OBPIH-5806 Render only reject, approve and rollback buttons to approver

9255fa5

OBPIH-5806 Rafactor requets state transition function

5e1c861

remove catching excetion on updateStatus and let generic error handler catch it

OBPIH-5806 use requisition.status instead of status

43f04d8

drodzewicz force-pushed the OBPIH-5806-approval-and-reject-action-approvals-end-to-end-workflow branch from 177d3ab to bd7b909 Compare September 27, 2023 10:56

OBPIH-5806 Fixes after review

cd42095

- refactor method names - refactor triggerRequisitionStatusTransition to use switch case instead of if

drodzewicz force-pushed the OBPIH-5806-approval-and-reject-action-approvals-end-to-end-workflow branch from bd7b909 to cd42095 Compare September 27, 2023 11:01

awalkowiak requested changes Sep 27, 2023

View reviewed changes

OBPIH-5806 Rename AuthTagLib method and add comment on status transition

eac0405

awalkowiak approved these changes Sep 27, 2023

View reviewed changes

awalkowiak merged commit 565da4f into develop Sep 27, 2023
3 checks passed

awalkowiak deleted the OBPIH-5806-approval-and-reject-action-approvals-end-to-end-workflow branch September 27, 2023 13:01

	redirect(action: "list", params: ["flash": flash as JSON, sourceType: "ELECTRONIC", direction: "OUTBOUND"])
	redirect(action: "list", params: ["flash": flash as JSON, sourceType: RequisitionSourceType.ELECTRONIC, direction: StockMovementDirection.OUTBOUND.toUpperCase()])

OBPIH-5806 approval and reject action approvals end to end workflow #4293

OBPIH-5806 approval and reject action approvals end to end workflow #4293

Conversation

drodzewicz commented Sep 22, 2023

kchelstowski 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

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

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

alannadolny Sep 25, 2023 • edited

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

drodzewicz Sep 27, 2023 • edited

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

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

alannadolny Sep 25, 2023 •

edited

drodzewicz Sep 27, 2023 •

edited