OBPIH-5801 Add approval/rejection fields to the Requisition domain #4280

kchelstowski · 2023-09-13T13:52:45Z

No description provided.

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

kchelstowski · 2023-09-14T10:44:07Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

+    CONFIRMING(0, null, StatusType.PRIMARY),
+    WAITING_FOR_APPROVAL(0, null, StatusType.WARNING),
+    APPROVED(0, null, StatusType.SUCCESS),
+    REJECTED(0, null, StatusType.DANGER)


let me know what sortOrder those should have in your opinion - I've just added it at the end for now, to have a discussion about that.

These new fields should be mixed in the first "section" of statuses. As you can see you added these in // Removed section.

awalkowiak

I have not yet reviewed the DB change log, but I already found a few issues (see comments) and you forgot about adding a list of designated approvers to that request.

awalkowiak · 2023-09-14T10:51:08Z

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

@@ -91,6 +93,8 @@ class Requisition implements Comparable<Requisition>, Serializable {
    // Intended recipient
    Person recipient

+    Person approvedBy


Can you move it down to other related fields and add a comment that these are approval related fields

awalkowiak · 2023-09-14T10:52:51Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

+    CONFIRMING(0, null, StatusType.PRIMARY),
+    WAITING_FOR_APPROVAL(0, null, StatusType.WARNING),
+    APPROVED(0, null, StatusType.SUCCESS),
+    REJECTED(0, null, StatusType.DANGER)


These new fields should be mixed in the first "section" of statuses. As you can see you added these in // Removed section.

awalkowiak

See my previous comment

drodzewicz

Everything looks good to me, good job

jmiranda · 2023-09-14T18:18:47Z

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

@@ -117,16 +119,42 @@ class Requisition implements Comparable<Requisition>, Serializable {

    Integer statusSortOrder

+    // Request approval fields
+    Person approvedBy
+


Minor: Let's remove the white space since all of the properties are related to the approvals feature

jmiranda · 2023-09-14T21:27:55Z

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

    static mapping = {
        id generator: 'uuid'
        requisitionItems cascade: "all-delete-orphan", sort: "orderIndex", order: 'asc', batchSize: 100

        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"]
+        designatedApprovers joinTable: [name: "requisition_designated_approvers", key: "requisition_id"]


We're going to need to refactor this a bit.

My first thought was that designated is an unnecessary qualifier so we should just name this property approvers and the table should be called requisition_approvers.

That would suffice.

But my spidey senses quickly indicated that maybe we need a more generic relationship here. One where we might have different users (and even people or organizations) that are associated with the requisition in different ways. Therefore we could have a generic table requisition_role that would map users (eventually parties) to requisitions through roles (creator, modifier, approver, etc).

We've never really needed a table like this in the past because the creator/modifier roles were always 1:1 and just needed a simple association (createdBy, updatedBy). But with approvers, it becomes a little different since we need to support multiple approvers.

By using a request_role table we would easily handle the approver use case, but could also future-proof the system to support more complex use cases like approval voting (require at least two approvals), notification schemes (users/roles watching certain requisitions), task assignments (picker, packer).

After consulting it with @awalkowiak , we will leave it as it is for now, not to block the whole feature, as we need to discuss it and we'll eventually include this refactor in this ticket but as "post-factum".

jmiranda · 2023-09-14T21:30:33Z

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

+
+    Date dateRejected
+
+    Boolean requiresApproval


I know this was what I came up with, but at the time I was thinking "that doesn't sound exactly right". Is there a naming convention that makes this sound more like a property that overrides some behavior or that the requisition is marked to be approved?

honestly I don't have a better idea. The only one that comes to my mind would be approvalRequired, but this is the same, but in reversed order, so I don't know.

went for approvalRequired (approved by @awalkowiak )

jmiranda · 2023-09-14T21:32:47Z

...0.8.x/changelog-2023-09-13-1300-alter-table-requisition-add-request-approval-improvement.xml

+        </preConditions>
+        <addForeignKeyConstraint baseColumnNames="comment_id"
+                                 baseTableName="requisition_comment"
+                                 constraintName="fk_requisition_comment_comment"


I prefer the way that Hibernate handles the foreign key naming so we don't have these long meandering names. I'll review old tickets to try to remember how that gets done.

since you probably forgot to reply on that one, I left the "manual" foreign keys names for now not to waste time.

jmiranda · 2023-09-14T21:37:35Z

...0.8.x/changelog-2023-09-13-1300-alter-table-requisition-add-request-approval-improvement.xml

+        </preConditions>
+        <addForeignKeyConstraint baseColumnNames="approved_by_id"
+                                 baseTableName="requisition"
+                                 constraintName="fk_approved_by_requisition"


You followed the correct naming convention for the previous foreign keys (which should be fk__) but not for this one.

fk_requisition_approved_by

jmiranda · 2023-09-14T21:40:53Z

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

+
+    Date dateApproved
+
+    Date dateRejected


I wondering if we should add one more timestamp to represent the date the approval was requested in case we need to send notifications after some period of time. This would probably be unnecessary if we set a timestamp when the requisition is submitted (i.e. dateSubmitted).

I don't mind doing that, so I leave the decision for you.
On the other hand, I think this could be misleading, unless we'd override the dateSubmitted over and over again.
I imagine a situation, when the request is REJECTED, then I believe someone will have a chance to correct it (?), and then again will send it for an approval (submit).

So the question would be - if we reject the request - should the dateSubmitted be cleared out or should it stay, and then, after another submission we'd just override it?

Discussed with the guys - we have dateRequested already, hence we didn't find an advantage of adding it for now.

jmiranda · 2023-09-14T21:41:55Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

-    ERROR(11, null, StatusType.DANGER),
+    WAITING_FOR_APPROVAL(3, null, StatusType.WARNING),
+    VERIFYING(4, PENDING, StatusType.WARNING),
+    APPROVED(5, null, StatusType.SUCCESS),


Verifying should come after Approved/Rejected.

I've consulted the order with Katarzyna and yeah, we've struggled with positioning those two - we agreed they are "equal", so I couldn't decide whether I should place them above or below the VERIFYING.
I'll change that.

changed the order

jmiranda · 2023-09-14T22:15:54Z

src/groovy/org/pih/warehouse/core/EventCode.groovy

@@ -24,11 +24,28 @@ enum EventCode {
    DELIVERED,
    RECEIVED,
    PARTIALLY_RECEIVED,
-    CANCELLED
+    CANCELLED,
+    WAITING_FOR_APPROVAL,


I'm wondering if we could come up with a more generic term for this like SUBMITTED. Or maybe we need both SUBMITTED and something like PENDING_APPROVAL.

If it were up to me, I'd distinguish them. Having said PENDING_APPROVAL, do you mean that I should change the naming from WAITING_FOR_APPROVAL to PENDING_APPROVAL, or it was just an open suggestion?

Changed to PENDING_APPROVAL, but left the translation as "Waiting for approval". Additionally - added translations for all of the EventCodes, since there were not any.

…able/domain

…those fields also to StockMovement Dto

…flow

…esting purposes

…domain for request approval workflow

…atus enum

…nslations for them

…equired

…4280) * OBPIH-5801 Add fields for request approval workflow for Requisition table/domain * OBPIH-5801 Include new fields in data binding of requisition and add those fields also to StockMovement Dto * OBPIH-5801 Add new requisition statuses for the request approval workflow * OBPIH-5801 Add events, requiresApproval fields to Requisition domain * OBPIH-5801 Add event codes for request approval workflow * OBPIH-5801 Revert changes in data binding of requisition, added for testing purposes * OBPIH-5801 Add a few unit tests to test fields added for Requisition domain for request approval workflow * OBPIH-5801 Revert unnecessary changes provided in the StockMovement dto * OBPIH-5801 Add designatedApprovers field to Requisition domain * OBPIH-5801 Change order of request approval statuses in RequisitionStatus enum * OBPIH-5801 Fixes after review to Requisition domain * OBPIH-5801 Add approvers list to StockMovement dto * OBPIH-5801 Rename event codes related to request approval and add translations for them * OBPIH-5801 Fix unit test related to renaming the boolean to approvalRequired * OBPIH-5801 Remove unused Event import in stock movement service

kchelstowski requested review from jmiranda, awalkowiak, drodzewicz and alannadolny September 13, 2023 13:52

kchelstowski self-assigned this Sep 13, 2023

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

kchelstowski commented Sep 13, 2023

View reviewed changes

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

kchelstowski commented Sep 14, 2023

View reviewed changes

kchelstowski marked this pull request as ready for review September 14, 2023 10:44

awalkowiak reviewed Sep 14, 2023

View reviewed changes

awalkowiak requested changes Sep 14, 2023

View reviewed changes

kchelstowski requested a review from awalkowiak September 14, 2023 11:37

alannadolny approved these changes Sep 14, 2023

View reviewed changes

drodzewicz approved these changes Sep 14, 2023

View reviewed changes

jmiranda requested changes Sep 14, 2023

View reviewed changes

kchelstowski added 15 commits September 18, 2023 10:16

OBPIH-5801 Add fields for request approval workflow for Requisition t…

39ec231

…able/domain

OBPIH-5801 Include new fields in data binding of requisition and add …

e23b73a

…those fields also to StockMovement Dto

OBPIH-5801 Add new requisition statuses for the request approval work…

c7f7ee6

…flow

OBPIH-5801 Add events, requiresApproval fields to Requisition domain

63bd4e8

OBPIH-5801 Add event codes for request approval workflow

0113aac

OBPIH-5801 Revert changes in data binding of requisition, added for t…

ffc49b8

…esting purposes

OBPIH-5801 Add a few unit tests to test fields added for Requisition …

bbbacbe

…domain for request approval workflow

OBPIH-5801 Revert unnecessary changes provided in the StockMovement dto

ab3e1b9

OBPIH-5801 Add designatedApprovers field to Requisition domain

ea4166c

OBPIH-5801 Change order of request approval statuses in RequisitionSt…

467702a

…atus enum

OBPIH-5801 Fixes after review to Requisition domain

10d0abc

OBPIH-5801 Add approvers list to StockMovement dto

3567489

OBPIH-5801 Rename event codes related to request approval and add tra…

7a152d7

…nslations for them

OBPIH-5801 Fix unit test related to renaming the boolean to approvalR…

6e1d4d0

…equired

OBPIH-5801 Remove unused Event import in stock movement service

b3f38ab

kchelstowski force-pushed the OBPIH-5801 branch from c449041 to b3f38ab Compare September 18, 2023 08:17

awalkowiak approved these changes Sep 18, 2023

View reviewed changes

awalkowiak merged commit 5783b9b into develop Sep 18, 2023
3 checks passed

awalkowiak deleted the OBPIH-5801 branch September 18, 2023 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5801 Add approval/rejection fields to the Requisition domain #4280

OBPIH-5801 Add approval/rejection fields to the Requisition domain #4280

kchelstowski commented Sep 13, 2023

kchelstowski Sep 14, 2023

awalkowiak Sep 14, 2023

awalkowiak left a comment

awalkowiak Sep 14, 2023

awalkowiak Sep 14, 2023

awalkowiak left a comment

drodzewicz left a comment

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023 •

edited

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

kchelstowski Sep 15, 2023

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

kchelstowski Sep 15, 2023

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

kchelstowski Sep 15, 2023

jmiranda Sep 14, 2023

kchelstowski Sep 15, 2023

kchelstowski Sep 15, 2023

OBPIH-5801 Add approval/rejection fields to the Requisition domain #4280

OBPIH-5801 Add approval/rejection fields to the Requisition domain #4280

Conversation

kchelstowski commented Sep 13, 2023

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

awalkowiak left a comment

Choose a reason for hiding this comment

drodzewicz 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 Sep 15, 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

kchelstowski Sep 15, 2023 •

edited