OBPIH-5868 mandatory comment after reject action gsp view page approvals end to end workflow #4328

drodzewicz · 2023-10-12T15:15:15Z

No description provided.

alannadolny · 2023-10-13T07:57:56Z

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

@@ -245,7 +250,7 @@ class StockMovementService {
        return true
    }

-    void updateRequisitionStatus(String id, RequisitionStatus status) {
+    void updateRequisitionStatus(String id, RequisitionStatus status, Comment comment) {


maybe it would be better to add default value to the comment. I mean adding here:

void updateRequisitionStatus(String id, RequisitionStatus status, Comment comment = null)

instead of passing empty comment above:

Comment comment = null

alannadolny · 2023-10-13T08:06:17Z

grails-app/views/stockMovement/addComment.gsp

+<script type="text/javascript">
+	$(document).ready(function() {
+		const commentInput = $("form [name='comment']")
+		if(${isRequestRejected}) {


Suggested change

if(${isRequestRejected}) {

if (${isRequestRejected}) {

😅

Is there a better way to do this without using GSP inside javascript? For example, using a hidden input field? Both are hacky but I love mixing languages less than hidden input fields.

I completely removed this now and set required attribute on the comment field since I believe that it should be required either way, if it is a regular comment or a mandatory comment when rejecting.

kchelstowski · 2023-10-13T16:45:36Z

grails-app/migrations/0.8.x/changelog-2023-10-11-1000-alter-table-event-add-comment-field.xml

+    <changeSet author="drodzewicz" id="111020231000-1">
+        <preConditions onFail="MARK_RAN">
+            <not>
+                <foreignKeyConstraintExists foreignKeyName="fk_comment"/>


It should be named fk_event_comment to follow the convention as we have for example for requisitoin's rejected by, we have: fk_requisition_rejected_by.

thanks for pointing that out

kchelstowski · 2023-10-13T16:46:53Z

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

@@ -114,10 +114,11 @@ class StockMovementService {
        Boolean statusOnly =
                jsonObject.containsKey("statusOnly") ? jsonObject.getBoolean("statusOnly") : false

+        Comment comment = null


I don't mind this, but I think it would be maybe better to make it as a default value of the method?
I leave the decision for you

I could add default value to a comment but it wouldn't change much.
It might be difficult to see on github view why I am defining a null comment at the begging but in short code looks something like this

Comment comment = null switch(status){ case CREATED: break case REQUESTED: break .... case REJECTED: comment = new Comment(parameters) break ... } updateRequisitionStatus(stockMovement.id, requisitionStatus, comment)

updateRequisitionStatus is called at the end for all status transitions, and I only need to pass the comment on REJECT.
So even if I instantiate the comment on REJECT I still need to access it in the outer scope so that's why I defined it at the top.

As I said, I don't mind it, it was only a suggestion to let you decide. I'm fine with it as it is currently.

kchelstowski · 2023-10-13T16:54:25Z

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

+                event.comment = comment
+                requisition.addToEvents(event)
+
+                requisition.addToComments(comment)


Instead of getting rid of calling the transitionRequisitionStatus here, I'd suggest refactoring the transitionRequisitionStatus method to include this part, so to add there something like:

if (comment) { event.comment = comment requisition.addToEvents(event) requisition.addToComments(comment) }

By the way it feels a bit dangerous to me, that we use the same comment instance adding it to a Requisition and to an Event - do we really need in both places?
Assuming we'd want to delete a comment, we'd have to remember to check if a Comment is not also associated with an Event, because otherwise it would not allow us to delete it (some of the comments would, some not, that's why it makes me confused about this approach).
I will come back to it on Monday, because I feel like I miss some context.

The reason I also ask about it, is that I don't see you "extracting" a Comment from an Event anywhere (in the view, or anywhere on the backend side) - I see you only adding it, so I don't know what's the purpose of making a relationship between an Event and a Comment.

I agree this could be in the transitionRequisitionStatus. It could have a null by default comment parameter.

good point, I'll do that

kchelstowski · 2023-10-13T16:55:12Z

grails-app/views/stockMovement/addComment.gsp

+				<g:if test="${comment?.id}">
+					<g:set var="formAction"  value="updateComment" />
+				</g:if>
+				<g:elseif test="${isRequestRejected}">


since you don't have more statements below, g:else should be enough here.

What if Dariusz wants to catch if request is rejected here, but not approved without comment?

btw I feel like isRejected should be enough

Since I am using the same template for 3 different actions saveComment, updateComment and updateStatus
I created something like a switch that assigns the formAction.
a normal code would looks something like

String formAction = "saveComment" if (comment.id) { formAction = "updateComment" } elseif(isRequestRejected) { formAction = "updateStatus" }

In my opinion all of the logical statements a re correct and elseif(isRequestRejected) is required since if it turn out false we will have formAction = "saveComment"

yes, you both are right, I have not realized that you have some statement inside the elseif.

kchelstowski · 2023-10-13T16:55:40Z

grails-app/views/stockMovement/addComment.gsp

+					</g:if>
+					<g:if test="${comment?.id}">
+						<g:hiddenField name="id" value="${comment?.id}" />
+					</g:if>


I kinda don't understand what is going on here, could you be that kind and explain those cases?

Because I am using this same template for both updateComment and updateStatus which both expect a different id parameter, for updateComment id = commented and for updateStatus id = stockMovementId I needed to wrap them inside the if statements to avoid stacking of values since calling this code

<g:hiddenField name="id" value="${stockMovement?.id}" /> <g:hiddenField name="id" value="${comment?.id}" />

will result in prams.id to have a value of an Array, it doesn't redefine the value but appends to the existing one of both are called.

awalkowiak · 2023-10-15T17:40:03Z

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

+                event.comment = comment
+                requisition.addToEvents(event)
+
+                requisition.addToComments(comment)


I agree this could be in the transitionRequisitionStatus. It could have a null by default comment parameter.

awalkowiak · 2023-10-15T17:40:14Z

grails-app/migrations/0.8.x/changelog-2023-10-11-1000-alter-table-event-add-comment-field.xml

+    <changeSet author="drodzewicz" id="111020231000-1">
+        <preConditions onFail="MARK_RAN">
+            <not>
+                <foreignKeyConstraintExists foreignKeyName="fk_comment"/>


awalkowiak · 2023-10-15T17:41:51Z

grails-app/views/stockMovement/addComment.gsp

+				<g:if test="${comment?.id}">
+					<g:set var="formAction"  value="updateComment" />
+				</g:if>
+				<g:elseif test="${isRequestRejected}">


What if Dariusz wants to catch if request is rejected here, but not approved without comment?

awalkowiak · 2023-10-15T17:42:25Z

grails-app/views/stockMovement/addComment.gsp

+				<g:if test="${comment?.id}">
+					<g:set var="formAction"  value="updateComment" />
+				</g:if>
+				<g:elseif test="${isRequestRejected}">


btw I feel like isRejected should be enough

awalkowiak · 2023-10-15T17:45:34Z

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

@@ -313,6 +318,14 @@ class StockMovementController {
        [stockMovement: stockMovement, comment: new Comment()]
    }

+    def rejectRequestWithComment = {


Do you have to be that explicit here? I think reject or rejectRequest would be fine if the comment is mandatory.

Yes please rename as reject.

jmiranda · 2023-10-16T01:55:18Z

grails-app/i18n/messages.properties

@@ -2306,6 +2306,8 @@ request.statusUpdate.success.message.label=You have successfully {0} the request
 request.rollbackApproval.insufficientPermissions.message=Unable to rollback approval due to insufficient permissions
 request.rollbackApproval.error.message=Unable to rollback approval: {0}
 request.rollbackApproval.success.message=Successfully rolled back approval
+request.confirmReject.label=Confirm Reject
+request.rejectReason.message.label=Please provide a reason for rejecting request


It's .message or .label, not both. I've seen this a few times. Label is used for field labels and message is used for a message.

jmiranda · 2023-10-16T02:01:58Z

grails-app/views/stockMovement/addComment.gsp

+<script type="text/javascript">
+	$(document).ready(function() {
+		const commentInput = $("form [name='comment']")
+		if(${isRequestRejected}) {


Is there a better way to do this without using GSP inside javascript? For example, using a hidden input field? Both are hacky but I love mixing languages less than hidden input fields.

…rejection

… mandatory comment on request reject by passing a prarameter approvalStatus=REJECTED addComment template renders appropriate fields and labels for reject comment

…a dof a list page

- change constraint name to fk_event_comment - use comment and status from recent event when sending mail about request status change - move comment logic to transitionRequisitionStatus as an optional parameter - rename variables

kchelstowski · 2023-10-16T15:33:38Z

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

@@ -891,6 +895,10 @@ class RequisitionService {

    void deleteComment(Comment comment, Requisition requisition) {
        requisition.removeFromComments(comment)
+        Event event = Event.findByComment(comment)
+        if (event) {
+            event.comment = null


Will the comment be deleted at this point? I assume it won't, because you don't have cascade/orphan delete on comment on the Event side, do you?

but I do on Requisition so requisition.removeFromComments(comment) should handle the delete.

kchelstowski · 2023-10-16T15:35:28Z

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

-        requisition.save()
+
+        if (comment) {
+            event.comment = comment


I still kinda don't get why the same comment has to be in both event and requisition - I don't see it (event.comment being used anywhere), but I won't delay it any longer, but I hope to get some explanation anyway to understand the context.

the comment alongside the status is used in email template
https://github.com/openboxes/openboxes/pull/4328/files#diff-9daf2cbe7ed9db21cdafd23a18dcb50073830df9194f9311c16303e1c76654f0R8-R10
we wanted to have a stronger association between Event <=> Comment.
At the moment it's done for sending a proper notification but there maybe some other plans for it in the future I think.

…als end to end workflow (#4328) * OBPIH-5868 Add optional comment association to Event * OBPIH-5868 Add mandatory comment to event and requisition on request rejection * OBPIH-5868 Refactored addComment template to configure this page as a mandatory comment on request reject by passing a prarameter approvalStatus=REJECTED addComment template renders appropriate fields and labels for reject comment * OBPIH-5868 On request reject reidrect user to request view page instea dof a list page * OBPIH-5868 Added translations for mandatory comment on reject * OBPIH-5868 Fixes after review - change constraint name to fk_event_comment - use comment and status from recent event when sending mail about request status change - move comment logic to transitionRequisitionStatus as an optional parameter - rename variables * OBPIH-5868 Make text area field on comment required

drodzewicz self-assigned this Oct 12, 2023

drodzewicz requested review from awalkowiak, kchelstowski, jmiranda and alannadolny October 13, 2023 07:32

alannadolny approved these changes Oct 13, 2023

View reviewed changes

kchelstowski reviewed Oct 13, 2023

View reviewed changes

awalkowiak requested changes Oct 15, 2023

View reviewed changes

jmiranda requested changes Oct 16, 2023

View reviewed changes

drodzewicz force-pushed the OBPIH-5868-mandatory-comment-after-reject-action-gsp-view-page-approvals-end-to-end-workflow branch from bc67509 to ae6ff18 Compare October 16, 2023 11:05

drodzewicz added 7 commits October 16, 2023 13:08

OBPIH-5868 Add optional comment association to Event

9126e3f

OBPIH-5868 Add mandatory comment to event and requisition on request …

f3ef22b

…rejection

OBPIH-5868 Refactored addComment template to configure this page as a…

3455a05

… mandatory comment on request reject by passing a prarameter approvalStatus=REJECTED addComment template renders appropriate fields and labels for reject comment

OBPIH-5868 On request reject reidrect user to request view page inste…

54774c9

…a dof a list page

OBPIH-5868 Added translations for mandatory comment on reject

7ee4084

OBPIH-5868 Fixes after review

76e3f4f

- change constraint name to fk_event_comment - use comment and status from recent event when sending mail about request status change - move comment logic to transitionRequisitionStatus as an optional parameter - rename variables

OBPIH-5868 Make text area field on comment required

246babf

drodzewicz force-pushed the OBPIH-5868-mandatory-comment-after-reject-action-gsp-view-page-approvals-end-to-end-workflow branch from ae6ff18 to 246babf Compare October 16, 2023 11:17

drodzewicz requested review from jmiranda, awalkowiak and kchelstowski October 16, 2023 11:19

kchelstowski reviewed Oct 16, 2023

View reviewed changes

awalkowiak approved these changes Oct 17, 2023

View reviewed changes

awalkowiak merged commit 2182896 into develop Oct 17, 2023
3 checks passed

awalkowiak deleted the OBPIH-5868-mandatory-comment-after-reject-action-gsp-view-page-approvals-end-to-end-workflow branch October 17, 2023 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5868 mandatory comment after reject action gsp view page approvals end to end workflow #4328

OBPIH-5868 mandatory comment after reject action gsp view page approvals end to end workflow #4328

drodzewicz commented Oct 12, 2023

alannadolny Oct 13, 2023

alannadolny Oct 13, 2023

jmiranda Oct 16, 2023

drodzewicz Oct 16, 2023

kchelstowski Oct 13, 2023

awalkowiak Oct 15, 2023

drodzewicz Oct 16, 2023

kchelstowski Oct 13, 2023

drodzewicz Oct 16, 2023

kchelstowski Oct 16, 2023

kchelstowski Oct 13, 2023

kchelstowski Oct 13, 2023

awalkowiak Oct 15, 2023

drodzewicz Oct 16, 2023

kchelstowski Oct 13, 2023

awalkowiak Oct 15, 2023

awalkowiak Oct 15, 2023

drodzewicz Oct 16, 2023

kchelstowski Oct 16, 2023

kchelstowski Oct 13, 2023

drodzewicz Oct 16, 2023

awalkowiak Oct 15, 2023

awalkowiak Oct 15, 2023

awalkowiak Oct 15, 2023

awalkowiak Oct 15, 2023

awalkowiak Oct 15, 2023

jmiranda Oct 16, 2023

jmiranda Oct 16, 2023

jmiranda Oct 16, 2023

kchelstowski Oct 16, 2023

drodzewicz Oct 16, 2023

kchelstowski Oct 16, 2023

drodzewicz Oct 16, 2023

OBPIH-5868 mandatory comment after reject action gsp view page approvals end to end workflow #4328

OBPIH-5868 mandatory comment after reject action gsp view page approvals end to end workflow #4328

Conversation

drodzewicz commented Oct 12, 2023

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

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