OBPIH-5768 Add request approval supported activity to the Location do… #4277

kchelstowski · 2023-09-13T07:31:37Z

…main

Either can be merged and for now use ROLE_APPOVER for user to check if it works fine, or do not merge and wait for the OBPIH-5799 to replace the role with REQUEST_APPROVER or whatever it will be.

…main

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

grails-app/i18n/messages.properties

awalkowiak · 2023-09-13T07:43:39Z

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

+            // Don't allow to set REQUEST_APPROVAL activity if there are not any users that
+            // have Request Approver location role for this location
+            // TODO: Replace ROLE_APPROVER with the ROLE_REQUEST_APPROVER when OBPIH-5799 is done
+            if (activities?.contains(ActivityCode.REQUEST_APPROVAL.id)


Was it meant to be a validator? I thought it was a message to inform the user that he has to do this after adding this supported activity to the location

I apologize, I was confused because of many different versions that we were speaking about, but I've consulted with Katarzyna and she confirmed this could be just a message, not a validation.
I fixed that part.

awalkowiak · 2023-09-13T07:44:01Z

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

@@ -64,7 +64,9 @@ enum ActivityCode {

    AUTOSAVE('AUTOSAVE'),

-    NONE('NONE')
+    NONE('NONE'),


Can you put it at the end?

… and instead return an information message

drodzewicz · 2023-09-13T08:38:09Z

grails-app/controllers/org/pih/warehouse/core/LocationController.groovy

@@ -114,6 +114,10 @@ class LocationController {
                    }
                    flash.message = "${warehouse.message(code: 'default.updated.message', args: [warehouse.message(code: 'location.label', default: 'Location'), locationInstance.id])}"

+                    if (locationInstance.supportedActivities.contains(ActivityCode.REQUEST_APPROVAL.id)) {
+                        flash.message += " ${g.message(code: 'location.supportedActivities.noRequestApprovers', default: 'You must add one or several Approvers to the location to support the requests approval workflow. Go to users list to add an Approver.')}"


can you confirm that the component where flash.message is rendered does not render HTML?
Because my first thought was to have a messages.properties label

location.supportedActivities.noRequestApprovers=You must add one or several Approvers to the location to support the requests approval workflow. Go to <a href="{0}">users list</a> to add an Approver.

and inject the link with an argument

flash.message += " ${g.message(code: 'location.supportedActivities.noRequestApprovers', default: 'You must add one or several Approvers to the location to support the requests approval workflow. Go to users list to add an Approver.', args: ["https://somelink...."])}"

alannadolny · 2023-09-13T13:09:35Z

grails-app/controllers/org/pih/warehouse/core/LocationController.groovy

+                    flash.message = []
+                    flash.message.add([code: 'default.updated.message', args: [warehouse.message(code: 'location.label', default: 'Location'), locationInstance.id]])


Can it be done without initializing empty array? I mean merging those two lines into one:

flash.message = [[code: 'default.updated.message', args: [warehouse.message(code: 'location.label', default: 'Location'), locationInstance.id]]]

alannadolny · 2023-09-13T13:12:51Z

grails-app/views/location/edit.gsp

-    <g:if test="${flash.message}">
-        <div class="message">${flash.message}</div>
-    </g:if>
+    <g:renderMessage />


I like this solution since rendering messages with HTML tags doesn't work well in Grails 1. The first situation that came to my mind was the message after logging in with incorrect credentials. (This message works on Grails 3 - the same code, without any changes) 👍👍

…ages passed by flash.message

awalkowiak · 2023-09-14T10:22:28Z

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

+ * by default takes the value stored in flash.message.
+ * accepts data like: String, List<String>, Map<code: string, args: obj>, List<Map<code: string, args: obj>>
+ */
+class RenderMessageTagLib {


@drodzewicz I am not sure if this is a good place for this tag lib (if there is need for a new tag lib). I wonder if this rather should be in the MessageTagLib.

jmiranda · 2023-09-15T03:52:02Z

grails-app/i18n/messages.properties

@@ -1456,6 +1457,7 @@ location.bgColor.label=Background color
 location.fgColor.label=Foreground color
 location.bgColor.invalid.matchingcolor=Colors cannot be the same
 location.supportedActivities.invalid.supportedActivities=You cannot set supported activity NONE in combination with other supported activities
+location.supportedActivities.noRequestApprovers=You must add one or several Approvers to the location to support the requests approval workflow. Go to <a href="{0}">users list</a> to add an Approver.


"one or more approvers to the location to support the requisition approval workflow."

jmiranda · 2023-09-15T03:53:22Z

grails-app/views/location/edit.gsp

-    <g:if test="${flash.message}">
-        <div class="message">${flash.message}</div>
-    </g:if>
+    <warehouse:renderMessage />


Can you explain the reason for this?

Using a join(".") in the controller might allow us to avoid this.

Can you explain the reason for this?

The reason for that was to be able to include a link inside the message. This has nothing to do with the dot.

The reason behind creating this taglib util function for rendering messages is because when translating labels in the controller and passing them using flash.messages it would either escape the HTML or render the HTML as a string.

So as a solution we want to pass to the flash.message all of the arguments needed for rendering the translated label and let the g.message handle the rendering.

I implemented this <warehouse:renderMessage /> taglib function in a way so it would be possible to render either a single message or a list of messages.
We can also both pass a normal string or, as I mentioned before, all of the arguments for rendering message.properties labels like code, args, default

jmiranda · 2023-09-15T03:54:33Z

grails-app/i18n/messages.properties

@@ -589,7 +589,7 @@ default.qtyPerUom.label=Qty per UoM
 default.pricePerUom.label=Price per UoM
 default.upload.label=Upload {0}
 default.uploaded.message={0} uploaded
-default.updated.message={0} updated
+default.updated.message={0} updated.


Periods. We don't need no stinkin' periods.

jmiranda · 2023-09-15T03:59:57Z

grails-app/controllers/org/pih/warehouse/core/LocationController.groovy

+                    ]
+                    if (locationInstance.supportedActivities.contains(ActivityCode.REQUEST_APPROVAL.id)) {
+                        flash.message.add([code: 'location.supportedActivities.noRequestApprovers', args: [g.createLink(controller: "user", action: "list")]])
+                    }


Instead of adding a period to the message, just add a join(".") here.

The comma isn't necessary anymore since the message is rendered in the new line like

Previously @kchelstowski must have added the comma to separate both messages since they were rendered in the same line.

jmiranda · 2023-09-15T04:02:22Z

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

@@ -64,6 +64,8 @@ enum ActivityCode {

    AUTOSAVE('AUTOSAVE'),

+    REQUEST_APPROVAL('REQUEST_APPROVAL'),


Why can't we use APPROVE_REQUEST above?

Honestly I have not seen this before. I checked it and it seems to be existing, but not having any effect 🤔 so we could potentially use it.
It's also displayed as Approve requisition - I saw your comments in @alannadolny 's ticket and I am a bit confused. In tickets it was supposed to be named with "Request" and you suggest that it should be "Requisition" instead of "Request".
So would it mean this activity code should be also renamed from APPROVE_REQUEST to APPROVE_REQUISITION?

I think we should not mix "Requisition" and "Request", so the question is, what is the path we want to follow?

awalkowiak

@drodzewicz There are still some pending requests (If everything is fixed, @kchelstowski you have green light to merge this one)

#4277) * OBPIH-5768 Add request approval supported activity to the Location domain * OBPIH-5798 Remove validation for request approval supported activity, and instead return an information message * OBPIH-5798 Render list of messages using flash.messages * OBPIH-5798 Create RenderMessageTagLib which handles rendering of messages passed by flash.message * OBPIH-5798 Move renderMessage to MessageTagLib * OBPIH-5798 Use APPROVE_REQUEST Activity code instead of REQUEST_APPROVAL --------- Co-authored-by: Darek Rodzewicz <DRodzewicz@gmail.com> (cherry picked from commit 19bf7ec)

…APPROVE_REQUEST activity (#4364) * OBPIH-5768 Add request approval supported activity to the Location do… (#4277) * OBPIH-5768 Add request approval supported activity to the Location domain * OBPIH-5798 Remove validation for request approval supported activity, and instead return an information message * OBPIH-5798 Render list of messages using flash.messages * OBPIH-5798 Create RenderMessageTagLib which handles rendering of messages passed by flash.message * OBPIH-5798 Move renderMessage to MessageTagLib * OBPIH-5798 Use APPROVE_REQUEST Activity code instead of REQUEST_APPROVAL --------- Co-authored-by: Darek Rodzewicz <DRodzewicz@gmail.com> (cherry picked from commit 19bf7ec) * OBGM-714 Fixed i18n code used to alert user to add approvers (cherry-picked re: OBPIH-5768) * OBGM-714 Removed unnecessary newline --------- Co-authored-by: jmiranda <jcm62@columbia.edu>

OBPIH-5768 Add request approval supported activity to the Location do…

3f1721a

…main

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

kchelstowski self-assigned this Sep 13, 2023

kchelstowski commented Sep 13, 2023

View reviewed changes

grails-app/domain/org/pih/warehouse/core/Location.groovy Outdated Show resolved Hide resolved

kchelstowski commented Sep 13, 2023

View reviewed changes

grails-app/i18n/messages.properties Outdated Show resolved Hide resolved

awalkowiak reviewed Sep 13, 2023

View reviewed changes

OBPIH-5798 Remove validation for request approval supported activity,…

91837b6

… and instead return an information message

kchelstowski force-pushed the OBPIH-5798 branch from 2e184ac to 91837b6 Compare September 13, 2023 08:20

kchelstowski requested a review from awalkowiak September 13, 2023 08:25

drodzewicz reviewed Sep 13, 2023

View reviewed changes

OBPIH-5798 Render list of messages using flash.messages

e11b14e

alannadolny approved these changes Sep 13, 2023

View reviewed changes

OBPIH-5798 Create RenderMessageTagLib which handles rendering of mess…

d442c4d

…ages passed by flash.message

drodzewicz force-pushed the OBPIH-5798 branch from d9256ca to d442c4d Compare September 14, 2023 08:21

awalkowiak reviewed Sep 14, 2023

View reviewed changes

OBPIH-5798 Move renderMessage to MessageTagLib

992abe3

jmiranda requested changes Sep 15, 2023

View reviewed changes

awalkowiak approved these changes Sep 15, 2023

View reviewed changes

OBPIH-5798 Use APPROVE_REQUEST Activity code instead of REQUEST_APPROVAL

4ef6883

awalkowiak requested changes Sep 15, 2023

View reviewed changes

drodzewicz force-pushed the OBPIH-5798 branch from 750f91a to 4ef6883 Compare September 15, 2023 11:33

awalkowiak merged commit 19bf7ec into develop Sep 15, 2023
3 checks passed

awalkowiak deleted the OBPIH-5798 branch September 15, 2023 12:05

kchelstowski mentioned this pull request Oct 31, 2023

OBGM-714 Approve request label broken + no flash message when adding APPROVE_REQUEST activity #4364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5768 Add request approval supported activity to the Location do… #4277

OBPIH-5768 Add request approval supported activity to the Location do… #4277

kchelstowski commented Sep 13, 2023

awalkowiak Sep 13, 2023

kchelstowski Sep 13, 2023

awalkowiak Sep 13, 2023

kchelstowski Sep 13, 2023

drodzewicz Sep 13, 2023

alannadolny Sep 13, 2023

alannadolny Sep 13, 2023

awalkowiak Sep 14, 2023

jmiranda Sep 15, 2023

jmiranda Sep 15, 2023

jmiranda Sep 15, 2023

kchelstowski Sep 15, 2023

drodzewicz Sep 15, 2023 •

edited

jmiranda Sep 15, 2023

jmiranda Sep 15, 2023

drodzewicz Sep 15, 2023

jmiranda Sep 15, 2023

kchelstowski Sep 15, 2023 •

edited

awalkowiak left a comment •

edited

		flash.message = []
		flash.message.add([code: 'default.updated.message', args: [warehouse.message(code: 'location.label', default: 'Location'), locationInstance.id]])

		@@ -64,6 +64,8 @@ enum ActivityCode {

		AUTOSAVE('AUTOSAVE'),

		REQUEST_APPROVAL('REQUEST_APPROVAL'),

OBPIH-5768 Add request approval supported activity to the Location do… #4277

OBPIH-5768 Add request approval supported activity to the Location do… #4277

Conversation

kchelstowski commented Sep 13, 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

drodzewicz 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

kchelstowski Sep 15, 2023 • edited

Choose a reason for hiding this comment

awalkowiak left a comment • edited

Choose a reason for hiding this comment

drodzewicz Sep 15, 2023 •

edited

kchelstowski Sep 15, 2023 •

edited

awalkowiak left a comment •

edited