Open
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
nilasini
changed the title
nilasini
force-pushed
the
notification
branch
from
0c4d4db to
319f038
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds notification support to the workflow approval system, enabling notifications to be sent to both approvers (when a new approval task is created) and requesters (when a decision is made on their request). The implementation supports multiple notification channels including email and SMS.
Changes:
- Added notification triggering functionality for approvers when approval tasks are created and for requesters when workflows are completed
- Implemented multi-channel notification support with channel-specific property building
- Extended the
completeWorkflowRequestmethod signature to includeworkflowIdparameter for retrieving notification preferences
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntegrated IdentityEventService and multi-channel notifications into the workflow engine: added governance dependency and OSGi imports, wired IdentityEventService in the component/data holder, and implemented channel parsing, contact-claim resolution, role→user expansion, per-channel property builders, and notification triggering in approval and completion flows. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant ApprovalSvc as ApprovalTaskServiceImpl
participant WorkflowStore as Workflow DB
participant EventSvc as IdentityEventService
participant Channel as Notification Channel (Email/SMS)
Client->>ApprovalSvc: Create workflow request (with notification params)
ApprovalSvc->>WorkflowStore: Persist request & tasks
ApprovalSvc->>ApprovalSvc: Expand roles -> getAssignedUserIds()
ApprovalSvc->>ApprovalSvc: Resolve contact claims for recipients
ApprovalSvc->>ApprovalSvc: Build per-channel notification properties
ApprovalSvc->>EventSvc: Trigger channel-specific notification events
EventSvc->>Channel: Deliver notification
Client->>ApprovalSvc: Approve/Reject task
ApprovalSvc->>WorkflowStore: Update task & workflow status
ApprovalSvc->>ApprovalSvc: Build requester notification properties
ApprovalSvc->>EventSvc: Trigger completion notification events
EventSvc->>Channel: Deliver decision notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
components/org.wso2.carbon.identity.workflow.engine/pom.xml (1)
131-134: Wildcard Import-Package patterns differ from existing style.Existing
Import-Packageentries specify exact packages (e.g.,org.wso2.carbon.identity.workflow.mgt;), whereas the new entries use wildcards (org.wso2.carbon.identity.event.*;). This works with BND/maven-bundle-plugin but is stylistically inconsistent with the rest of this configuration. Consider whether explicit package names would be preferable for consistency and to avoid importing unintended sub-packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/pom.xml` around lines 131 - 134, The Import-Package entries use wildcard patterns (e.g., "org.wso2.carbon.identity.event.*;", "org.wso2.carbon.identity.application.authentication.framework.util.*;", "org.wso2.carbon.identity.governance.*;") which is stylistically inconsistent with existing explicit imports like "org.wso2.carbon.identity.workflow.mgt;"; change these wildcard imports to explicit package names to match the project style and avoid pulling unintended sub-packages: list each concrete package under org.wso2.carbon.identity.event, org.wso2.carbon.identity.application.authentication.framework.util, and org.wso2.carbon.identity.governance that are actually used and replace the wildcard tokens with those exact package entries (keeping the same version range properties).components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java (4)
491-496: Dead code:resolveEventNamenever returns blank.
resolveEventNamealways returns either the SMS event name orIdentityEventConstants.Event.TRIGGER_NOTIFICATION, so theStringUtils.isBlank(eventName)check on line 493 will never be true. This is harmless defensive code, but thelog.debug("Unsupported notification channel: {}", ch)message could be misleading if someone reads it expecting it to fire.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 491 - 496, The StringUtils.isBlank(eventName) branch in ApprovalTaskServiceImpl's loop over channels is dead because resolveEventName(...) never returns blank; remove the conditional and its misleading log.debug("Unsupported notification channel: {}", ch) to avoid confusion. Specifically, delete the check that tests eventName for blank and the continue/log inside that branch (the loop using variable ch and calling resolveEventName), leaving only the resolved eventName usage; if you want explicit validation instead, perform a channel-level check before calling resolveEventName and log a clear message about unrecognized channels rather than relying on a never-empty return value.
484-486: Misleading parameter namerecipientUserId— it's actually the approver for requester notifications.In
triggerNotification, therecipientUserIdparameter is documented as "The user ID of the notification recipient." However, whenisInitialNotification=false, the value passed (line 1028) is the approver's userId, and the actual recipient (requester) is resolved fromworkflowRequest.getCreatedBy()insidebuildRequesterNotificationProperties.This dual-purpose parameter is confusing and error-prone. Consider renaming to
actorUserIdor splitting into separate methods for approver vs. requester notifications.Also applies to: 530-546, 739-744
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 484 - 486, The parameter recipientUserId in triggerNotification is misleading because it sometimes holds the approver's userId while the real recipient for requester notifications is resolved via workflowRequest.getCreatedBy() inside buildRequesterNotificationProperties; rename recipientUserId to actorUserId (or similar) across triggerNotification, its callers, and Javadoc to reflect that it represents the acting user (approver) rather than the final recipient, and update usages in methods referenced (triggerNotification, buildRequesterNotificationProperties, and all callers that pass approver IDs) to avoid confusion; alternatively, split triggerNotification into two explicit methods (triggerApproverNotification and triggerRequesterNotification) so one accepts approverUserId and the other resolves requester from workflowRequest.getCreatedBy() and update call sites accordingly.
293-301: Reduce visibility ofresolveEventNametoprivate static.The method is called only internally at line 492 within the same class. Changing from
public statictoprivate staticimproves encapsulation and reduces unnecessary API surface exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 293 - 301, The method resolveEventName is declared public static but is only used internally within this class; change its declaration to private static to reduce API exposure and improve encapsulation by updating the method signature for resolveEventName accordingly (ensure all internal calls within ApprovalTaskServiceImpl still compile against the new private static method).
118-118: Inconsistent claim URI sourcing — email uses framework constant, mobile is hardcoded.
FrameworkConstants.EMAIL_ADDRESS_CLAIMis used for the email channel at line 461, but the mobile claim is sourced from a local constantCLAIM_MOBILEdefined at line 118 with a hardcoded string value"http://wso2.org/claims/mobile". For consistency, consider defining the mobile claim using a unified approach — either both as local constants or both from FrameworkConstants if a mobile claim constant is available there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` at line 118, The mobile claim is hardcoded via the local constant CLAIM_MOBILE in ApprovalTaskServiceImpl while the email uses FrameworkConstants.EMAIL_ADDRESS_CLAIM; update the code to be consistent by removing/avoiding CLAIM_MOBILE and using the FrameworkConstants mobile claim constant (e.g., FrameworkConstants.MOBILE_NUMBER_CLAIM or the actual mobile claim identifier defined in FrameworkConstants) wherever CLAIM_MOBILE is referenced in ApprovalTaskServiceImpl; if FrameworkConstants does not provide a mobile constant, instead define both email and mobile as local constants in ApprovalTaskServiceImpl with consistent naming (e.g., LOCAL_CLAIM_EMAIL and LOCAL_CLAIM_MOBILE) and replace usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 559-582: The buildApproverNotificationProperties method currently
puts values from getContact(...) and getUserClaimValue(...) directly into
properties; first validate that approverContact and approverUsername are
non-null/non-empty after calling getContact and getUserClaimValue, and if
approverContact is missing, log a warning (using the class logger) and
abort/skip building/sending the notification (e.g., return early or throw a
WorkflowEngineException) to avoid inserting a null "send-to" property; likewise
handle a missing approverUsername by using a sensible fallback (e.g., the
approverUserId or "Unknown") or logging and skipping, and ensure no null values
are inserted into properties keys "send-to" or "approverName" before returning
from buildApproverNotificationProperties.
- Line 783: In ApprovalTaskServiceImpl, fix the checkstyle violation by adding a
space after the if keyword in the conditional that checks log.isDebugEnabled();
locate the occurrence of "if(log.isDebugEnabled()) {" and change it to use the
correct spacing ("if (log.isDebugEnabled()) {") so the if keyword is followed by
whitespace and the style linter passes.
- Around line 506-508: The call to
WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService().handleEvent(notificationEvent)
in ApprovalTaskServiceImpl can NPE if getIdentityEventService() returns null;
add a null guard around the service lookup: retrieve IdentityEventService into a
local variable (e.g., identityEventService), check for null before calling
handleEvent(notificationEvent), and if null either log a warning/error via the
existing logger and skip calling handleEvent or throw/capture an
IdentityEventException as appropriate to preserve the existing exception
handling flow.
- Around line 346-350: In ApprovalTaskServiceImpl, avoid NPE by guarding the
qName call: inside the block that checks
parameter.getParamName().equalsIgnoreCase(PARAM_NOTIFICATION), ensure
parameter.getqName() is not null before calling
startsWith(Q_NAME_APPROVER_CHANNELS_PREFIX) (e.g., add an explicit null check or
use a null-safe startsWith helper); then set approverNotificationChannels =
parameter.getParamValue() only when qName is non-null and startsWith the prefix.
Ensure you reference PARAM_NOTIFICATION, Q_NAME_APPROVER_CHANNELS_PREFIX,
approverNotificationChannels and the local variable parameter when making this
change.
- Around line 116-124: Remove the duplicate local constant APPROVER_TYPE_ROLES
from ApprovalTaskServiceImpl and replace all usages of that local constant with
WorkflowEngineConstants.APPROVER_TYPE_ROLES (e.g., the usage currently
referencing APPROVER_TYPE_ROLES around the approval-type check). Delete the
private static final String APPROVER_TYPE_ROLES declaration and update any
references (including the occurrence noted near the approval-type handling) to
use WorkflowEngineConstants.APPROVER_TYPE_ROLES for consistency.
- Around line 1004-1032: completeWorkflowRequest currently calls
wsWorkflowCallBackService.onCallback(...) and then runs notification logic
(getApprovalWorkflowParameters, Utils.resolveUserID, triggerNotification) but
re-throws WorkflowEngineException which can make callers think completion failed
even after callback succeeded; wrap the notification block (the call to
getApprovalWorkflowParameters/Utils.resolveUserID/triggerNotification) in a
try-catch that catches WorkflowEngineException (and any RuntimeException from
notification) and logs a warning (e.g., log.warn or the class logger) with the
exception and context (workflowRequestId, workflowId) instead of re-throwing, so
the callback/commit remains successful and notification failures are best-effort
only.
---
Nitpick comments:
In `@components/org.wso2.carbon.identity.workflow.engine/pom.xml`:
- Around line 131-134: The Import-Package entries use wildcard patterns (e.g.,
"org.wso2.carbon.identity.event.*;",
"org.wso2.carbon.identity.application.authentication.framework.util.*;",
"org.wso2.carbon.identity.governance.*;") which is stylistically inconsistent
with existing explicit imports like "org.wso2.carbon.identity.workflow.mgt;";
change these wildcard imports to explicit package names to match the project
style and avoid pulling unintended sub-packages: list each concrete package
under org.wso2.carbon.identity.event,
org.wso2.carbon.identity.application.authentication.framework.util, and
org.wso2.carbon.identity.governance that are actually used and replace the
wildcard tokens with those exact package entries (keeping the same version range
properties).
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 491-496: The StringUtils.isBlank(eventName) branch in
ApprovalTaskServiceImpl's loop over channels is dead because
resolveEventName(...) never returns blank; remove the conditional and its
misleading log.debug("Unsupported notification channel: {}", ch) to avoid
confusion. Specifically, delete the check that tests eventName for blank and the
continue/log inside that branch (the loop using variable ch and calling
resolveEventName), leaving only the resolved eventName usage; if you want
explicit validation instead, perform a channel-level check before calling
resolveEventName and log a clear message about unrecognized channels rather than
relying on a never-empty return value.
- Around line 484-486: The parameter recipientUserId in triggerNotification is
misleading because it sometimes holds the approver's userId while the real
recipient for requester notifications is resolved via
workflowRequest.getCreatedBy() inside buildRequesterNotificationProperties;
rename recipientUserId to actorUserId (or similar) across triggerNotification,
its callers, and Javadoc to reflect that it represents the acting user
(approver) rather than the final recipient, and update usages in methods
referenced (triggerNotification, buildRequesterNotificationProperties, and all
callers that pass approver IDs) to avoid confusion; alternatively, split
triggerNotification into two explicit methods (triggerApproverNotification and
triggerRequesterNotification) so one accepts approverUserId and the other
resolves requester from workflowRequest.getCreatedBy() and update call sites
accordingly.
- Around line 293-301: The method resolveEventName is declared public static but
is only used internally within this class; change its declaration to private
static to reduce API exposure and improve encapsulation by updating the method
signature for resolveEventName accordingly (ensure all internal calls within
ApprovalTaskServiceImpl still compile against the new private static method).
- Line 118: The mobile claim is hardcoded via the local constant CLAIM_MOBILE in
ApprovalTaskServiceImpl while the email uses
FrameworkConstants.EMAIL_ADDRESS_CLAIM; update the code to be consistent by
removing/avoiding CLAIM_MOBILE and using the FrameworkConstants mobile claim
constant (e.g., FrameworkConstants.MOBILE_NUMBER_CLAIM or the actual mobile
claim identifier defined in FrameworkConstants) wherever CLAIM_MOBILE is
referenced in ApprovalTaskServiceImpl; if FrameworkConstants does not provide a
mobile constant, instead define both email and mobile as local constants in
ApprovalTaskServiceImpl with consistent naming (e.g., LOCAL_CLAIM_EMAIL and
LOCAL_CLAIM_MOBILE) and replace usages accordingly.
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
nilasini
force-pushed
the
notification
branch
from
8ed3d06 to
b13da31
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java (2)
572-580: Consider extracting hardcoded notification property keys into constants.Both
buildApproverNotificationPropertiesandbuildRequesterNotificationPropertiesuse raw string literals for property keys ("TEMPLATE_TYPE","send-to","approverName","tenant-domain","workflowId","requesterName", etc.). Extracting these intoprivate static final Stringconstants would reduce typo risk and improve maintainability.Also applies to: 753-761
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 572 - 580, The notification property keys in ApprovalTaskServiceImpl (used in buildApproverNotificationProperties and buildRequesterNotificationProperties) are hardcoded string literals; extract each unique key (e.g., "TEMPLATE_TYPE", "approverName", "send-to", "tenant-domain", "approvalActionUrl", "workflowId", "requesterName", "submittedDate", "workflowType", etc.) into private static final String constants at the top of the class and replace the raw literals in both methods with those constants to avoid typos and improve maintainability (apply the same change for the other occurrence referenced around the 753-761 region).
116-123: ChangeQ_NAME_INITIATOR_CHANNELS_PREFIXvisibility frompublictoprivate.This constant is only used internally at line 1017 within the same class. All sibling notification constants (
CHANNEL_SMS,PARAM_NOTIFICATION,Q_NAME_APPROVER_CHANNELS_PREFIX, etc.) are private, and this one should be as well to minimize the public API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 116 - 123, Change the visibility of the constant Q_NAME_INITIATOR_CHANNELS_PREFIX in ApprovalTaskServiceImpl from public to private to match the other notification constants (CHANNEL_SMS, PARAM_NOTIFICATION, Q_NAME_APPROVER_CHANNELS_PREFIX, etc.) and reduce the class's public API surface; ensure no external references exist and keep the constant name and value unchanged so internal uses (e.g., where it's referenced within ApprovalTaskServiceImpl) continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 344-349: The code in ApprovalTaskServiceImpl accesses
parameter.getqName() without null-safety which can NPE; update the conditional
that sets approverNotificationChannels to first ensure parameter.getqName() is
not null (e.g., check parameter.getqName() != null) before calling startsWith,
keeping the existing checks for approverNotificationChannels == null and
parameter.getParamName().equalsIgnoreCase(PARAM_NOTIFICATION) and still
comparing against Q_NAME_APPROVER_CHANNELS_PREFIX; ensure the null-guard
preserves short-circuit evaluation so startsWith is only invoked when getqName()
is non-null.
- Around line 558-581: In buildApproverNotificationProperties validate the
results of getContact(...) and getUserClaimValue(...) before putting them into
properties: if approverContact or approverUsername is null (from
getContact/getUserClaimValue) throw a WorkflowEngineException with a clear
message including approverUserId, tenantId and claim/channel (use
resolveContactClaimUri(channel) to indicate which claim), otherwise populate
properties.put("send-to", approverContact) and properties.put("approverName",
approverUsername) as currently done; this prevents sending null destinations and
surfaces the missing-claim error early.
- Around line 1010-1029: The notification block currently throws a
WorkflowEngineServerException on any WorkflowEngineException from
getApprovalWorkflowParameters/triggerNotification, which can cause the API to
report failure after wsWorkflowCallBackService.onCallback() has already
committed the workflow; change this to make notifications best-effort: catch
exceptions around the calls to getApprovalWorkflowParameters(...) and
triggerNotification(...), log the error (using the class logger) including the
exception and context (workflowId/workflowRequestId/userId/status), and do not
re-throw WorkflowEngineServerException so execution returns normally after
logging; keep existing logic for resolving userId via Utils.resolveUserID(...)
and do not alter wsWorkflowCallBackService.onCallback().
- Around line 505-507: The call to
WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService() can
return null and lead to an NPE outside the IdentityEventException catch block;
before constructing/dispatching the Event (notificationEvent) check that
getIdentityEventService() is non-null and only call
handleEvent(notificationEvent) when the service exists (otherwise skip or log a
warning). Ensure the null-check wraps the invocation of handleEvent (referencing
getIdentityEventService(), handleEvent(...), Event/notificationEvent) so any
absent OSGi binding is safely handled.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 572-580: The notification property keys in ApprovalTaskServiceImpl
(used in buildApproverNotificationProperties and
buildRequesterNotificationProperties) are hardcoded string literals; extract
each unique key (e.g., "TEMPLATE_TYPE", "approverName", "send-to",
"tenant-domain", "approvalActionUrl", "workflowId", "requesterName",
"submittedDate", "workflowType", etc.) into private static final String
constants at the top of the class and replace the raw literals in both methods
with those constants to avoid typos and improve maintainability (apply the same
change for the other occurrence referenced around the 753-761 region).
- Around line 116-123: Change the visibility of the constant
Q_NAME_INITIATOR_CHANNELS_PREFIX in ApprovalTaskServiceImpl from public to
private to match the other notification constants (CHANNEL_SMS,
PARAM_NOTIFICATION, Q_NAME_APPROVER_CHANNELS_PREFIX, etc.) and reduce the
class's public API surface; ensure no external references exist and keep the
constant name and value unchanged so internal uses (e.g., where it's referenced
within ApprovalTaskServiceImpl) continue to work.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java (1)
479-480: MisleadingrecipientUserIdparameter name intriggerNotification.When
isInitialNotification=false(requester notification path), the value passed asrecipientUserIdis actually the approver's user ID (see line 1019–1020), not the notification recipient. The actual recipient is resolved internally fromworkflowRequest.getCreatedBy(). Consider renaming the parameter tocontextUserIdor splitting into two explicit parameters to avoid confusion in future maintenance.Also applies to: 733-738
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 479 - 480, The parameter name recipientUserId on triggerNotification is misleading because when isInitialNotification is false the value passed is actually the approver/context user (not the final recipient resolved from workflowRequest.getCreatedBy()); rename recipientUserId to contextUserId (or add a second parameter actualRecipientUserId) in the triggerNotification(String recipientUserId, String workflowRequestId, boolean isInitialNotification, String decision, String channel) signature, update all callers to pass the approver/context id as contextUserId (and the real recipient where you add a second param), and update internal uses and JavaDoc so the code explicitly resolves workflowRequest.getCreatedBy() as the recipient only when appropriate (ensure you change the two caller sites mentioned in the review to match the new parameter name/shape).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 460-467: getContact currently falls through to
getUserClaimValueByUsername when userId is blank even if username is null, which
can cause a UserStoreException; update getContact to defensively check that
username is not blank (use StringUtils.isNotBlank or equivalent) before calling
getUserClaimValueByUsername and return null or throw a
WorkflowEngineServerException with a clear message if both userId and username
are missing (this affects callers like buildRequesterNotificationProperties
which pass workflowRequest.getCreatedBy()); reference getContact,
getUserClaimValueByUsername, buildRequesterNotificationProperties, and
workflowRequest.getCreatedBy() when making the change.
---
Duplicate comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 345-349: In ApprovalTaskServiceImpl, protect against NPE by
checking that parameter.getqName() is not blank before calling startsWith:
inside the block where you check
parameter.getParamName().equalsIgnoreCase(PARAM_NOTIFICATION) (and where you
assign approverNotificationChannels), add a guard using
StringUtils.isNotBlank(parameter.getqName()) (same approach used in
completeWorkflowRequest) and only call
parameter.getqName().startsWith(Q_NAME_APPROVER_CHANNELS_PREFIX) when that guard
passes; ensure approverNotificationChannels is set only within that guarded
branch.
- Around line 996-1025: In completeWorkflowRequest, after calling
wsWorkflowCallBackService.onCallback (which commits the workflow), wrap the
retrieval of parameters (getApprovalWorkflowParameters) and the call to
triggerNotification in a try/catch that does not rethrow; instead catch
WorkflowEngineException (and any RuntimeException) and log the error with
context (e.g., workflowRequestId, workflowId, status, and
requesterNotificationChannels) so notification failures are best-effort and do
not propagate; remove the current throw new WorkflowEngineServerException in
that catch block so callers of handleApproval/handleReject see success when the
callback succeeded.
- Around line 500-502: ApprovalTaskServiceImpl currently calls
WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService().handleEvent(notificationEvent)
without guarding for a null IdentityEventService; add a null-check for
getIdentityEventService() before invoking handleEvent (e.g., store in a local
variable like identityEventService), and if it is null, log an error and throw
an IdentityEventException (or return appropriately) so you avoid an NPE and
still hit the IdentityEventException handling path.
- Around line 553-576: buildApproverNotificationProperties is inserting
potentially null values from getContact(...) and getUserClaimValue(...) into
properties ("send-to" and "approverName"), which can break notification
delivery; update buildApproverNotificationProperties (and similarly
buildRequesterNotificationProperties) to validate the results of getContact and
getUserClaimValue before putting them into properties — if null or empty, log a
warning via the workflow logger (or throw a WorkflowEngineException if
notification must not proceed) and skip adding the "send-to" / "approverName"
keys (or set an explicit safe fallback) so you never insert null into properties
and handlers receive a clear, non-null value or the flow fails fast.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 479-480: The parameter name recipientUserId on triggerNotification
is misleading because when isInitialNotification is false the value passed is
actually the approver/context user (not the final recipient resolved from
workflowRequest.getCreatedBy()); rename recipientUserId to contextUserId (or add
a second parameter actualRecipientUserId) in the triggerNotification(String
recipientUserId, String workflowRequestId, boolean isInitialNotification, String
decision, String channel) signature, update all callers to pass the
approver/context id as contextUserId (and the real recipient where you add a
second param), and update internal uses and JavaDoc so the code explicitly
resolves workflowRequest.getCreatedBy() as the recipient only when appropriate
(ensure you change the two caller sites mentioned in the review to match the new
parameter name/shape).
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java (1)
503-505:⚠️ Potential issue | 🟠 MajorPotential NullPointerException if
IdentityEventServiceis unavailable.
getIdentityEventService()can returnnullif the OSGi service is temporarily unbound (the reference usesDYNAMICpolicy). CallinghandleEvent(...)on a null reference will throw an unhandled NPE that bypasses theIdentityEventExceptioncatch block.🛡️ Proposed fix — add a null guard
try { // Build properties specific to this channel. Map<String, Object> notificationProperties = new HashMap<>(); String notificationChannel = getServerSupportedNotificationChannel(ch); notificationProperties.put("notificationChannel", notificationChannel); buildNotificationPropertiesForChannel(workflowRequestId, recipientUserId, isInitialNotification, decision, ch, notificationProperties); + var identityEventService = WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService(); + if (identityEventService == null) { + log.warn("IdentityEventService is not available. Skipping notification for channel: {}", ch); + continue; + } Event notificationEvent = new Event(eventName, notificationProperties); - WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService() - .handleEvent(notificationEvent); + identityEventService.handleEvent(notificationEvent); } catch (IdentityEventException e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 503 - 505, ApprovalTaskServiceImpl currently calls WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService().handleEvent(notificationEvent) without guarding for a null IdentityEventService; add a null check on the result of getIdentityEventService() before invoking handleEvent (e.g., retrieve the service into a local variable, if null then log a warning/error and either skip notification or throw an IdentityEventException), ensuring you reference the existing Event/notificationEvent and call to handleEvent so behavior remains consistent when the dynamic OSGi service is temporarily unavailable.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java (1)
396-399: Consider wrapping notification loop in try-catch for robustness.While
triggerNotificationinternally handlesIdentityEventExceptionandWorkflowEngineException, any unexpected runtime exception could still propagate and fail the workflow task creation. Since notifications are best-effort, consider wrapping the entire loop.♻️ Proposed fix
// Trigger notifications after all parameters are processed and channels are extracted. for (String approverIdentifier : approversToNotify) { + try { triggerNotification(approverIdentifier, workflowRequestId, true, null, approverNotificationChannels); + } catch (Exception e) { + log.error("Error while triggering notification for approver: {}", approverIdentifier, e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 396 - 399, The notification loop in ApprovalTaskServiceImpl calling triggerNotification should be made best-effort by wrapping each iteration (or the whole loop) in a try-catch to prevent unexpected runtime exceptions from escaping and failing task creation; catch Exception (or RuntimeException), log the error with context (approverIdentifier and workflowRequestId) using the existing logger, and continue to the next approver so failures in triggerNotification (method triggerNotification) do not abort the workflow task creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 503-505: ApprovalTaskServiceImpl currently calls
WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService().handleEvent(notificationEvent)
without guarding for a null IdentityEventService; add a null check on the result
of getIdentityEventService() before invoking handleEvent (e.g., retrieve the
service into a local variable, if null then log a warning/error and either skip
notification or throw an IdentityEventException), ensuring you reference the
existing Event/notificationEvent and call to handleEvent so behavior remains
consistent when the dynamic OSGi service is temporarily unavailable.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 396-399: The notification loop in ApprovalTaskServiceImpl calling
triggerNotification should be made best-effort by wrapping each iteration (or
the whole loop) in a try-catch to prevent unexpected runtime exceptions from
escaping and failing task creation; catch Exception (or RuntimeException), log
the error with context (approverIdentifier and workflowRequestId) using the
existing logger, and continue to the next approver so failures in
triggerNotification (method triggerNotification) do not abort the workflow task
creation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java (1)
504-506:⚠️ Potential issue | 🟠 MajorGuard
IdentityEventServicebefore invokinghandleEvent.Directly calling
getIdentityEventService().handleEvent(...)can throw NPE when the service is temporarily unavailable, which breaks the intended best-effort notification behavior.🛡️ Proposed fix
- Event notificationEvent = new Event(eventName, notificationProperties); - WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService() - .handleEvent(notificationEvent); + Event notificationEvent = new Event(eventName, notificationProperties); + if (WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService() == null) { + log.warn("IdentityEventService is not available. Skipping notification for channel: {}", ch); + continue; + } + WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService() + .handleEvent(notificationEvent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java` around lines 504 - 506, The code in ApprovalTaskServiceImpl calls WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService().handleEvent(notificationEvent) without a null check; guard the IdentityEventService reference before invoking handleEvent by retrieving it into a local variable (e.g., identityEventService = WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService()), check for null and only call identityEventService.handleEvent(notificationEvent) when non-null, and wrap the call in a try-catch to swallow/log unexpected exceptions so the notification remains best-effort; reference getIdentityEventService(), handleEvent(...), WorkflowEngineServiceDataHolder and notificationEvent when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 1013-1018: The loop in ApprovalTaskServiceImpl that iterates
parameterList only extracts requesterNotificationChannels when qName starts with
Q_NAME_INITIATOR_CHANNELS_PREFIX and ignores any configured events (e.g.,
onApproval); modify the logic in the block that checks PARAM_NOTIFICATION/qName
so that you parse the qName suffix or another parameter field to determine the
configured event (e.g., "onApproval") and only assign
requesterNotificationChannels when that event matches the current completion
event being processed (the same fix should be applied to the other occurrence
around the code handling lines 1023-1025); update the condition that sets
requesterNotificationChannels to validate both the channel prefix
(Q_NAME_INITIATOR_CHANNELS_PREFIX) and the event name before breaking out of the
loop.
- Around line 345-349: The code in ApprovalTaskServiceImpl only reads
PARAM_NOTIFICATION into approverNotificationChannels when parameter.getqName()
startsWith Q_NAME_APPROVER_CHANNELS_PREFIX, which ignores payload-level event
qualifiers (like onAssignment/onRelease) so notifications are always treated as
assignment-time and onRelease is never reached; update the parsing of
parameter.getqName() in the parameter loop (where parameter.getParamName() ==
PARAM_NOTIFICATION and where similar logic appears again) to extract the event
qualifier suffix (e.g., the token after Q_NAME_APPROVER_CHANNELS_PREFIX) and
store notification channels keyed by event (or separate variables such as
approverNotificationChannelsOnAssignment and
approverNotificationChannelsOnRelease), then when sending notifications check
the current event context (assignment vs release) and pick the corresponding
channels instead of always using approverNotificationChannels. Ensure you update
both occurrences referenced so payload-level event controls are honored.
- Around line 490-500: The code computes eventName from the raw channel variable
ch before applying server normalization, which can cause a channel/event
mismatch; change the order to first call
getServerSupportedNotificationChannel(ch) to obtain a normalized
notificationChannel, then call resolveEventName(notificationChannel) and use
that eventName; update any debug log that references ch to reflect
notificationChannel when applicable (e.g., log unsupported channel using
notificationChannel) and ensure notificationProperties still uses the normalized
notificationChannel variable.
---
Duplicate comments:
In
`@components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java`:
- Around line 504-506: The code in ApprovalTaskServiceImpl calls
WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService().handleEvent(notificationEvent)
without a null check; guard the IdentityEventService reference before invoking
handleEvent by retrieving it into a local variable (e.g., identityEventService =
WorkflowEngineServiceDataHolder.getInstance().getIdentityEventService()), check
for null and only call identityEventService.handleEvent(notificationEvent) when
non-null, and wrap the call in a try-catch to swallow/log unexpected exceptions
so the notification remains best-effort; reference getIdentityEventService(),
handleEvent(...), WorkflowEngineServiceDataHolder and notificationEvent when
making this change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
20 tasks
nilasini
commented
Mar 6, 2026
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
nilasini
commented
Mar 6, 2026
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Show resolved
Hide resolved
nilasini
commented
Mar 6, 2026
...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Resolves wso2/product-is#26873
Depends on wso2/identity-api-server#1063 and wso2/carbon-identity-framework#7812
DB storage
Parameters are retrieved as below.
Summary by CodeRabbit
New Features
Chores