OBPIH-5803 Add submit for approval functionality to create request workflow #4288

kchelstowski · 2023-09-20T09:12:58Z

No description provided.

…lizationException in a published event

…s for them

…a request

…ovement status, not shipment status

grails-app/services/org/pih/warehouse/inventory/RequisitionStatusTransitionEventService.groovy

…l activity and remove confirm submit modal

…usTransitionEvent

kchelstowski · 2023-09-21T14:08:10Z

grails-app/domain/org/pih/warehouse/requisition/Requisition.groovy

@@ -121,6 +121,7 @@ class Requisition implements Comparable<Requisition>, Serializable {

    // Request approval fields
    Person approvedBy
+    Person rejectedBy


As I mentioned on one of the tech huddles/standups - I forgot to add this field in the domain changes ticket, so I've added it now.

I know that @drodzewicz also added the migration for rejectedBy within the ticket that he is working on, so we have to remember to avoid duplicating them

kchelstowski · 2023-09-21T14:11:01Z

grails-app/domain/org/pih/warehouse/requisition/Requisition.groovy

@@ -150,7 +151,8 @@ class Requisition implements Comparable<Requisition>, Serializable {
        statusSortOrder formula: RequisitionStatus.getStatusSortOrderFormula()
        monthRequested formula: "date_format(date_requested, '%M %Y')"
        comments joinTable: [name: "requisition_comment", key: "requisition_id"]
-        events joinTable: [name: "requisition_event", key: "requisition_id"]
+        // Fetch join in the events should be removed in Grails 3 and replaced with proper join query using data services
+        events joinTable: [name: "requisition_event", key: "requisition_id"], fetch: 'join'


I was getting a LazyInitializationException when accessing the events in the RequisitionStatusTransitionEvent - it is because the event is done in a different session (after the session where we fetch the Requisition ends).

Current solution actually adds an eager fetching of the events, but using the left join clause, not calling a second query.
Normally I would not do it and would not recommend doing an eager fetch "globally", but I decided that it was not worth to spend time to create a custom query for that, because in Grails 1 we don't have data services.
This is why I left a comment to refactor this ASAP in Grails 3, to use a proper joining query using data services

Every time I see an eager join I can't decide if we should go with it or do something else.
For me it might be fine but I hesitate to give a final opinion on this.
Just like you said in other comments, we will probably use @Service methods for these scenarios in grails 3, so I am thinking maybe this should be a service method instead of a domain eager fetch

@drodzewicz this would mean we’d have to write a pure SQL in order to fetch that.
I just thought it’s waste of time for now, since we’ll migrate in a moment and shouldn’t experience any performance issues during that time

I am not sure about that one, @jmiranda what's your opinion here?

I would probably try a withNewSession in the event handler.

Requisition.withNewSession { requisition = Requisition.read() // or Requisition.get() if you need to set something }

Or if that doesn't work, just do a Requisition.read(id) or get(id) in the event handler.

Lastly, you could potentially use refresh the event handler but that one always scares me.

requisition.refresh()

Not sure if you need the withSession or withNewSession wrapper so I'll leave that as an exercise for the reader.

of course get/read will work, but I wanted to avoid additional query to refetch it, but if you are fine with it, I can just fetch it again in the event service.

kchelstowski · 2023-09-21T14:11:17Z

grails-app/i18n/messages.properties

@@ -3440,6 +3447,9 @@ react.stockMovement.status.picking.description.label=The inventory selection nee
 react.stockMovement.status.picked.description.label=This shipment is ready to be shipped. The packing information may need to be entered.
 react.stockMovement.status.issued.description.label=This shipment has been shipped out.
 react.stockMovement.status.checking.description.label=This shipment is ready to be shipped.
+react.stockMovement.status.pending_approval.description.label=Waiting for approval
+react.stockMovement.status.approved.description.label=Approved
+react.stockMovement.status.rejected.description.label=Rejected


those are for the tooltips on list pages

kchelstowski · 2023-09-21T14:12:34Z

grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy

@@ -230,6 +231,7 @@ class StockMovementService {
    void updateRequisitionStatus(String id, RequisitionStatus status) {

        log.info "Update status ${id} " + status
+        // TODO: In Grails the get below should be replaced by the data service get that joins the Events


here it sohuld be replaced by the data service query, something like getWithEvents that would @Join('events')
I know that @drodzewicz has run into the same issue but with approvers, so most probably we'll have to write the query using @Query and join those two tables (if I forget - remember to take care of left join there)

You could (but don't have to) add a getWithEvents() to the Requisition class now.

yeah, but since those data service annotations are not available in Grails 1, I would anyway have to write this HQL/SQL query by myself using .executeQuery, wouldn't I?

kchelstowski · 2023-09-21T14:13:34Z

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

+                requisition.save()
+                return
+            }
+            Event event = createEvent(EventCode.SUBMITTED, requisition.origin, new Date())


I am not sure if SUBMITTED is a correct code here, so ready for discussion/suggestions.
(It is if we submit a request "in an old way", so if we don't support the request approval workflow)

alannadolny · 2023-09-21T14:15:16Z

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

@@ -20,12 +21,18 @@ package org.pih.warehouse.core
 *{eventDate: 5/5/2010, eventLocation: Customs, eventType: ARRIVED}*/
 class Event implements Comparable, Serializable {

+    def beforeInsert = {


I remember that we were changing this from closures to methods. I am not sure if the methods work in Grails 1, but if they work, I think it would be better to change them to avoid inconsistency in Grails 3.

This is outdated - I had to remove this approach, since triggering an event creates its own session, hence the currentUser was null

alannadolny · 2023-09-21T14:18:26Z

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

+    Boolean isApprovalRequired() {
+        return supportedActivities?.contains(ActivityCode.APPROVE_REQUEST.id)
+    }
+


I can see that there is a method called supports maybe it would be better to use it?
I mean:

Boolean isApprovalRequired() { return supports(ActivityCode.APPROVE_REQUEST.id) }

Yeah, thanks for pointing that out, I forgot to ask about it as well - if we should use contains like I did, or supports, because supports also checks for locationType’s supportedActivities and I was not sure whether the request approval is supposed to work if we add this activity to a locationType

I think it should use supports

alannadolny · 2023-09-21T14:22:24Z

.../migrations/0.8.x/changelog-2023-09-21-1330-alter-table-requisition-add-rejectedby-field.xml

+                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog/1.9 http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-1.9.xsd">
+
+    <changeSet author="kchelstowski" id="210920231330-1">


Just a question, why are you numbering change sets from 1 instead of 0? 🤔

I don’t know 😄

Blasphemous behavior. 😈

alannadolny · 2023-09-21T14:25:09Z

grails-app/services/org/pih/warehouse/inventory/RequisitionStatusTransitionEventService.groovy


-class RequisitionStatusTransitionEventService {
+    static transactional = true


Is this transactional really necessary? (I know that you marked it to remember adding a transaction in Grails 3, but I am not sure if there is a need for the transaction)

I added it when searching for solutions for the Lazy issue mentioned above. I can remove it if you don’t like it

If this is not required anymore, then you should remove it

If we're just doing reads then we don't need a transaction. If you're writing to the database then we need a transaction.

In general I would not agree, because not having a transaction boundary also causes LazyInitializationExceptions while reading (assuming you don't use osiv). Anyway as I mentioned, it was actually added for debugging purposes and finding a solution for the Lazy that was thrown.
I will remove it.

alannadolny · 2023-09-21T14:30:19Z

grails-app/domain/org/pih/warehouse/requisition/Requisition.groovy

@@ -121,6 +121,7 @@ class Requisition implements Comparable<Requisition>, Serializable {

    // Request approval fields
    Person approvedBy
+    Person rejectedBy


I know that @drodzewicz also added the migration for rejectedBy within the ticket that he is working on, so we have to remember to avoid duplicating them

awalkowiak · 2023-09-21T15:15:49Z

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

+    Boolean isApprovalRequired() {
+        return supportedActivities?.contains(ActivityCode.APPROVE_REQUEST.id)
+    }
+


I think it should use supports

awalkowiak · 2023-09-21T15:16:22Z

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

@@ -9,6 +9,7 @@
 **/
 package org.pih.warehouse.core

+import org.pih.warehouse.auth.AuthService


Is this required?

Not anymore, forgot to delete it in the latest commit.

awalkowiak · 2023-09-21T15:18:53Z

grails-app/domain/org/pih/warehouse/requisition/Requisition.groovy

@@ -150,7 +151,8 @@ class Requisition implements Comparable<Requisition>, Serializable {
        statusSortOrder formula: RequisitionStatus.getStatusSortOrderFormula()
        monthRequested formula: "date_format(date_requested, '%M %Y')"
        comments joinTable: [name: "requisition_comment", key: "requisition_id"]
-        events joinTable: [name: "requisition_event", key: "requisition_id"]
+        // Fetch join in the events should be removed in Grails 3 and replaced with proper join query using data services
+        events joinTable: [name: "requisition_event", key: "requisition_id"], fetch: 'join'


I am not sure about that one, @jmiranda what's your opinion here?

awalkowiak · 2023-09-21T15:21:15Z

.../migrations/0.8.x/changelog-2023-09-21-1400-add-eventtypes-for-request-approval-workflow.xml

+        <insert tableName="event_type">
+            <column name="id" value="abb5b3d2-da3e-4a4c-9e3c-de139147f822"/>
+            <column name="version" valueNumeric="0"/>
+            <column name="date_created" valueDate="2023-09-21T00:00:00.0"/>


Perhaps it's better to use <property name="now" value="now()"/> for these dates.

Interesting. I hadn't really thought about the use of now() until you mentioned it here. I'm not sure I agree, but I'm fine with either approach. There's something to be said for having a static value for static data to ensure that everyone has the exact same data, down to the timestamps.

There's also an argument for knowing the exact time the data was created in case you are troubleshooting a database migration issue during deployment (i.e. a failed precondition).

And just from an aesthetic point of view, sometimes when I do a select * from event_type or some other static data table, there's a nice fuzzy feeling of seeing 2012 in the date_created. That is usually followed quickly with dread and despair of knowing I'm old af.

Ok then to prevent existential crises ... let's use now().

Definitely we should avoid existential crises 😆 so I'll change that

awalkowiak · 2023-09-21T15:24:09Z

.../migrations/0.8.x/changelog-2023-09-21-1400-add-eventtypes-for-request-approval-workflow.xml

+            <column name="event_code" value="PENDING_APPROVAL"/>
+            <column name="last_updated" valueDate="2023-09-21T00:00:00.0"/>
+            <column name="name" value="Waiting for approval"/>
+            <column name="sort_order" valueNumeric="1"/>


I am not sure if these sort orders make sense, but I am not sure if this is currently used for anything at all.

It's a good question. If we ever do

event1 > event2

then we probably need to deal with it.

But I assume we're using the ordinal value on the EventCode to do the sorting, so probably not an issue.

I'm ok leaving this for now OR changing sort order to 0.

it seems not to be used anywhere at the moment, but since the default event types also have the sortOrder, I decided to add it.
Not to cause confusions I can either remove it (since it is nullable) or change it to 0. Let me know.

awalkowiak · 2023-09-21T15:25:03Z

grails-app/services/org/pih/warehouse/inventory/RequisitionStatusTransitionEventService.groovy


-class RequisitionStatusTransitionEventService {
+    static transactional = true


If this is not required anymore, then you should remove it

jmiranda

I can't remember if I requested changes, so just leaving this as a comment. LGTM.

jmiranda · 2023-09-21T16:04:44Z

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

+    Boolean isApprovalRequired() {
+        return supportedActivities?.contains(ActivityCode.APPROVE_REQUEST.id)
+    }
+


jmiranda · 2023-09-21T16:11:55Z

grails-app/domain/org/pih/warehouse/requisition/Requisition.groovy

@@ -150,7 +151,8 @@ class Requisition implements Comparable<Requisition>, Serializable {
        statusSortOrder formula: RequisitionStatus.getStatusSortOrderFormula()
        monthRequested formula: "date_format(date_requested, '%M %Y')"
        comments joinTable: [name: "requisition_comment", key: "requisition_id"]
-        events joinTable: [name: "requisition_event", key: "requisition_id"]
+        // Fetch join in the events should be removed in Grails 3 and replaced with proper join query using data services
+        events joinTable: [name: "requisition_event", key: "requisition_id"], fetch: 'join'


I would probably try a withNewSession in the event handler.

Requisition.withNewSession { requisition = Requisition.read() // or Requisition.get() if you need to set something }

Or if that doesn't work, just do a Requisition.read(id) or get(id) in the event handler.

Lastly, you could potentially use refresh the event handler but that one always scares me.

requisition.refresh()

Not sure if you need the withSession or withNewSession wrapper so I'll leave that as an exercise for the reader.

jmiranda · 2023-09-21T16:13:20Z

.../migrations/0.8.x/changelog-2023-09-21-1330-alter-table-requisition-add-rejectedby-field.xml

+                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog/1.9 http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-1.9.xsd">
+
+    <changeSet author="kchelstowski" id="210920231330-1">


Blasphemous behavior. 😈

jmiranda · 2023-09-21T16:21:05Z

.../migrations/0.8.x/changelog-2023-09-21-1400-add-eventtypes-for-request-approval-workflow.xml

+        <insert tableName="event_type">
+            <column name="id" value="abb5b3d2-da3e-4a4c-9e3c-de139147f822"/>
+            <column name="version" valueNumeric="0"/>
+            <column name="date_created" valueDate="2023-09-21T00:00:00.0"/>


Interesting. I hadn't really thought about the use of now() until you mentioned it here. I'm not sure I agree, but I'm fine with either approach. There's something to be said for having a static value for static data to ensure that everyone has the exact same data, down to the timestamps.

There's also an argument for knowing the exact time the data was created in case you are troubleshooting a database migration issue during deployment (i.e. a failed precondition).

And just from an aesthetic point of view, sometimes when I do a select * from event_type or some other static data table, there's a nice fuzzy feeling of seeing 2012 in the date_created. That is usually followed quickly with dread and despair of knowing I'm old af.

Ok then to prevent existential crises ... let's use now().

jmiranda · 2023-09-21T16:23:13Z

.../migrations/0.8.x/changelog-2023-09-21-1400-add-eventtypes-for-request-approval-workflow.xml

+            <column name="event_code" value="PENDING_APPROVAL"/>
+            <column name="last_updated" valueDate="2023-09-21T00:00:00.0"/>
+            <column name="name" value="Waiting for approval"/>
+            <column name="sort_order" valueNumeric="1"/>


It's a good question. If we ever do

event1 > event2

then we probably need to deal with it.

But I assume we're using the ordinal value on the EventCode to do the sorting, so probably not an issue.

I'm ok leaving this for now OR changing sort order to 0.

jmiranda · 2023-09-21T16:24:06Z

grails-app/services/org/pih/warehouse/inventory/RequisitionStatusTransitionEventService.groovy


-class RequisitionStatusTransitionEventService {
+    static transactional = true


If we're just doing reads then we don't need a transaction. If you're writing to the database then we need a transaction.

jmiranda · 2023-09-21T16:25:50Z

grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy

@@ -230,6 +231,7 @@ class StockMovementService {
    void updateRequisitionStatus(String id, RequisitionStatus status) {

        log.info "Update status ${id} " + status
+        // TODO: In Grails the get below should be replaced by the data service get that joins the Events


You could (but don't have to) add a getWithEvents() to the Requisition class now.

jmiranda · 2023-09-21T16:26:47Z

grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy

@@ -244,6 +246,7 @@ class StockMovementService {
        } else {
            requisition.status = status
            requisition.save(flush: true)
+            publishEvent(new RequisitionStatusTransitionEvent(requisition, requisition.status))


You could pass the requisition.id here in order to force a read or get on the event service. We could think about doing that across all of our events.

Is there any need to pass both the from and to status here?

Could you elaborate on what you mean by "from and to" status? I pass only a new status (I got rid of the oldStatus, because I feel like it is not needed).

jmiranda · 2023-09-21T16:27:33Z

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

+    Event createEvent(EventCode eventCode, Location eventLocation, Date eventDate, User currentUser) {
+        EventType eventType = EventType.findByEventCode(eventCode)
+        if (!eventType) {
+            throw new NotFoundException("No event type with code ${eventCode} has been found")


Good call on the exception.

awalkowiak

I think it LGTM

…rkflow (#4288) * OBPIH-5803 Add approvalRequired transient to Location domain * OBPIH-5803 Change fetchMode of Requisition events to avoid LazyInitializationException in a published event * OBPIH-5803 Add request approval statuses to enums and add translations for them * OBPIH-5803 Include status transition in RequisitionStatusTransitionEvent * OBPIH-5803 Add publishing a status transition event while submitting a request * OBPIH-5803 Add transition status method in RequisitionService * OBPIH-5803 Change displayed status on SM view page to look for stockMovement status, not shipment status * OBPIH-5803 Add createdBy field for Event domain * OBPIH-5803 Add rejectedBy field to Requisition * OBPIH-5803 Display correct submit button depending on request approval activity and remove confirm submit modal * OBPIH-5803 Remove unnecessary oldStatus property from RequisitionStatusTransitionEvent * OBPIH-5803 Include new requisition statuses in the sortOrder formula * OBPIH-5803 Add default EventTypes for request approval workflow * OBPIH-5803 Fix assigning createdBy to an Event * OBPIH-5803 Fixes after review

kchelstowski added 7 commits September 20, 2023 11:07

OBPIH-5803 Add approvalRequired transient to Location domain

974735f

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

4c390c9

…lizationException in a published event

OBPIH-5803 Add request approval statuses to enums and add translation…

bcc0a96

…s for them

OBPIH-5803 Include status transition in RequisitionStatusTransitionEvent

1542ae6

OBPIH-5803 Add publishing a status transition event while submitting …

ad9dcb0

…a request

OBPIH-5803 Add transition status method in RequisitionService

fdb3a03

OBPIH-5803 Change displayed status on SM view page to look for stockM…

fdf50d9

…ovement status, not shipment status

kchelstowski self-assigned this Sep 20, 2023

kchelstowski marked this pull request as draft September 20, 2023 09:13

alannadolny reviewed Sep 20, 2023

View reviewed changes

grails-app/services/org/pih/warehouse/inventory/RequisitionStatusTransitionEventService.groovy Outdated Show resolved Hide resolved

This was referenced Sep 20, 2023

OBPIH-5821 Move send notification logic to notificationService and add individual links for approvers to request list #4289

Merged

OBPIH-5854 New approval statuses in SM list pages - Approvals lists pages #4290

Merged

kchelstowski added 6 commits September 21, 2023 15:59

OBPIH-5803 Add createdBy field for Event domain

f9aa91a

OBPIH-5803 Add rejectedBy field to Requisition

bbf8af4

OBPIH-5803 Display correct submit button depending on request approva…

1d43e68

…l activity and remove confirm submit modal

OBPIH-5803 Remove unnecessary oldStatus property from RequisitionStat…

db25c37

…usTransitionEvent

OBPIH-5803 Include new requisition statuses in the sortOrder formula

ae3ecb4

OBPIH-5803 Add default EventTypes for request approval workflow

5bf2ad4

kchelstowski marked this pull request as ready for review September 21, 2023 14:04

kchelstowski requested review from jmiranda, awalkowiak, drodzewicz and alannadolny September 21, 2023 14:05

kchelstowski commented Sep 21, 2023

View reviewed changes

OBPIH-5803 Fix assigning createdBy to an Event

459a2c5

alannadolny reviewed Sep 21, 2023

View reviewed changes

awalkowiak reviewed Sep 21, 2023

View reviewed changes

jmiranda reviewed Sep 21, 2023

View reviewed changes

OBPIH-5803 Fixes after review

5bd9cf0

awalkowiak approved these changes Sep 22, 2023

View reviewed changes

awalkowiak merged commit de3618b into develop Sep 22, 2023
3 checks passed

awalkowiak deleted the OBPIH-5803 branch September 22, 2023 13:30


		class RequisitionStatusTransitionEventService {
		static transactional = true

OBPIH-5803 Add submit for approval functionality to create request workflow #4288

OBPIH-5803 Add submit for approval functionality to create request workflow #4288

Conversation

kchelstowski commented Sep 20, 2023

Choose a reason for hiding this comment

alannadolny Sep 21, 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

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 21, 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

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

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 Sep 22, 2023 • edited

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

alannadolny Sep 21, 2023 •

edited

alannadolny Sep 21, 2023 •

edited

kchelstowski Sep 22, 2023 •

edited