OBPIH-5838 Option to disable approval notifications #4324

awalkowiak · 2023-10-10T13:24:33Z

No description provided.

awalkowiak · 2023-10-10T14:40:03Z

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

+ // For now this is meant for requests between two depots (checked only in that case).
+ // Perhaps we could also distinguish it for different type of downstream consumers and/or "upstream suppliers"
+ // (depot vs non managed locally ward)?
+ DISABLE_APPROVAL_NOTIFICATIONS('DISABLE_APPROVAL_NOTIFICATIONS'),


@jmiranda Actually now if I think about it, enabling by default should be as easy as disabling by default if we want to rely on users to enable it. This would not need any migration.

The only problematic case would be distinguishing when it is "Depot" requestor vs "Ward" requestor and then deciding who should be getting the notification (if it is enabled) based on the status (?)

Actually now if I think about it, enabling by default should be as easy as disabling by default if we want to rely on users to enable it. This would not need any migration.

In general, I think the default should be "enable approval notifications" at all location types that fulfill stock or request stock. But we can leave the migration for later if it becomes necessary.

The only problematic case would be distinguishing when it is "Depot" requestor vs "Ward" requestor and then deciding who should be getting the notification (if it is enabled) based on the status (?)

Yeah that's what I was trying to unravel in my head during the tech huddle. There's a matrix here that I just can't wrap my head around yet, both on the configuration side as well as on the evaluation side. I think we need to work out the evaluation side first and then we can make our way back to the configuration.

We just need to figure out the dimensions

Requisition Status

Requesting Location Type

Fulfilling Location Type

Is that enough? Do we need to take user roles into account as well?

User Roles

For example, one scenario in the matrix might look like this

Prevent the system from sending notifications to Approvers and Requestors when a request from a depot has been submitted, approved, or rejected. When: Requisition Status: Approved Inbound Location Type: Depot Inbound User Role: Requestor Then: Enable Approval Notifications: False

I just can't figure out how to set this as a supported activity on locations. Maybe this?

ENABLE_APPROVAL_NOTIFICATIONS: Sends all approval notifications ENABLE_CONSUMER_APPROVAL_NOTIFICATIONS: Send notifications when requesting location = CONSUMER ENABLE_DEPOT_APPROVAL_NOTIFICATIONS: Send notifications when requesting location has MANAGE_INVENTORY?

Does that configuration even cover all of the different scenarios we need to handle? The naming here is also going to be atrocious.

@jmiranda ok here how it looks like I think:

Assumptions:
All notifications are enabled by default,
Option to disable on location types (all notifications, only requestor notifications or only approver notifications),
Expected notification matrix (by PIH):

Requestor Fulfilling Request Submitted notification (status= PENDING_APROVAL, receives approver) Request Approved/rejected notification (status=APPROVED or REJECTED, receives requestor)

Depot Depot No No

Ward Depot Yes Yes

which would translate into:
If request from “Depot”, then if status PENDING_APPROVAL, then send a notification to approvers if the origin does not support DISABLE_DEPOT_APPROVAL_NOTIFICATIONS and DISABLE_APPROVAL_NOTIFICATIONS (to disable set the “fulfiller” notification activity code or “global” approval notification code),
If request from "Depot", then if status APPROVED or REJECTED, then send a notification to requestors if the destination does not support DISABLE_CONSUMER_APPROVAL_NOTIFICATIONS and DISABLE_APPROVAL_NOTIFICATIONS(to disable set the consumer notification activity code or “global” approval notification code),
If request from "Ward", then if status PENDING_APPROVAL, then send a notification to approvers if the origin does not support DISABLE_APPROVAL_NOTIFICATIONS (to disable set the “global” approval notification activity code),
If request from "Ward", then if status APPROVED or REJECTED, then send a notification to requestors if the destination does not support DISABLE_APPROVAL_NOTIFICATIONS (to disable set the “global” approval notification activity code),

Please take a look at the shouldSendApprovalNotification code. I was wondering how DISABLE_DEPOT_APPROVAL_NOTIFICATIONS should be named (I went with your option for now) and I wondered if DISABLE_SUPPLIER_APPROVAL_NOTIFICATIONS would be more suitable, as an opposite to the DISABLE_CONSUMER_APPROVAL_NOTIFICATIONS.

Tech huddle:

..._FULFILLER_APPROVAL_NOTIFICATIONS and ..._REQUESTOR_APPROVAL_NOTIFICATIONS instead of ..._DEPOT_APPROVAL_NOTIFICATIONS and ..._CONSUMER_APPROVAL_NOTIFICATIONS

Let's make it enabled by default (with migration) and disable it by unassigning the activity code (on a location type level)

Let's stick to two activity codes (if possible)

@jmiranda I did it with only two activity codes. To disable this for any type of location it needs to be disabled (removed) on the requesting location (ENABLE_FULFILLER_APPROVAL_NOTIFICATIONS to not send notification to approvers and ENABLE_REQUESTOR_APPROVAL_NOTIFICATIONS to not get notification about approval or rejection) because ultimately everything here is depending on the requestor.

jmiranda · 2023-10-19T02:02:47Z

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

@@ -401,6 +402,38 @@ class Requisition implements Comparable<Requisition>, Serializable {
 return null
 }

+ boolean shouldSendApprovalNotification() {
+ if (destination.isDownstreamConsumer()) {


You can just use isConsumer().

I am assuming this is part of the older review, because this if is not there anymore

jmiranda · 2023-10-20T18:15:59Z

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

+ }
+
+ // by default always send approval workflow notifications (if not handled above)
+ return true


Given that we're handling all of the states above it doesn't seem possible. Can you identify a scenario where this could happen?

Also I feel like I'm missing something here. It seems like the "shouldSendApprovalNotification" method should be location-dependent? In other words, we should enable fulfiller notifications on the origin and requestor approval notifications on the requestor? So asking whether to send the approval notifications should be again the stock movement or the

Pending Approval: send Approval notification to approvers if origin has approval notifications enabled
Approved: send Approved notification to requestors if destination has approval notifications enabled
Rejected: send Rejected notification to requestors if destination has approval notifications enabled

That way we can turn each notification type on / off for each side of the stock movement.

Can you explain why it's better for the downstream endpoint to control the notification? I'm thinking of a scenario where, as an implementer in a distribution center, I want to disable notifications for Pending Approval because I've set up my system to allow users to manage their workflow through the dashboard. I don't want one of my requestors to then re-enable notifications because they have a different workflow.

Actually, this is an obsolete version. You are right that it should be handled above. I am gonna change it to "false", just in case someone triggers this for the wrong status that is not handled above.

jmiranda · 2023-10-20T18:17:38Z

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

@@ -268,7 +268,9 @@ class StockMovementService {
 log.warn("Transition from ${requisition.status.name()} to ${status.name()} is not allowed - use rollback instead")
 } else {
 requisitionService.triggerRequisitionStatusTransition(requisition, AuthService.currentUser.get(), status, comment)
- publishEvent(new RequisitionStatusTransitionEvent(requisition))
+ if (requisition.shouldSendApprovalNotification()) {
+ publishEvent(new RequisitionStatusTransitionEvent(requisition))


This should happen no matter what.

Whether the notification is sent should be up to the event listener.

* OBPIH-5838 Disabling notifications WIP * OBPIH-5838 Option to disable approval notifications * OBPIH-5838 Add activity code translations * OBPIH-5838 Option to disable approval notifications * Improvements after review

awalkowiak requested a review from jmiranda October 10, 2023 13:24

awalkowiak force-pushed the OBPIH-5838 branch from 6587379 to df70e03 Compare October 10, 2023 13:35

awalkowiak commented Oct 10, 2023

View reviewed changes

awalkowiak force-pushed the OBPIH-5838 branch from df70e03 to 4e55fd9 Compare October 17, 2023 14:05

awalkowiak marked this pull request as ready for review October 17, 2023 14:40

awalkowiak changed the title ~~OBPIH-5838 Disabling notifications WIP~~ OBPIH-5838 Option to disable approval notifications Oct 17, 2023

jmiranda requested changes Oct 20, 2023

View reviewed changes

awalkowiak added 5 commits October 23, 2023 12:02

OBPIH-5838 Disabling notifications WIP

1cc0927

OBPIH-5838 Option to disable approval notifications

cacd3a7

OBPIH-5838 Add activity code translations

b48cea2

OBPIH-5838 Option to disable approval notifications

6fbeba7

Improvements after review

d38347e

awalkowiak force-pushed the OBPIH-5838 branch from 0fd02c0 to d38347e Compare October 23, 2023 10:04

awalkowiak merged commit 5de6066 into develop Oct 23, 2023
3 checks passed

awalkowiak deleted the OBPIH-5838 branch October 23, 2023 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5838 Option to disable approval notifications #4324

OBPIH-5838 Option to disable approval notifications #4324

awalkowiak commented Oct 10, 2023

awalkowiak Oct 10, 2023

jmiranda Oct 10, 2023

awalkowiak Oct 17, 2023

awalkowiak Oct 18, 2023

awalkowiak Oct 19, 2023 •

edited

Loading

jmiranda Oct 19, 2023

awalkowiak Oct 23, 2023

jmiranda Oct 20, 2023

awalkowiak Oct 23, 2023

jmiranda Oct 20, 2023

OBPIH-5838 Option to disable approval notifications #4324

OBPIH-5838 Option to disable approval notifications #4324

Conversation

awalkowiak commented Oct 10, 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

awalkowiak Oct 19, 2023 • edited Loading

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 Oct 19, 2023 •

edited

Loading