Skip to content

Conversation

@ImalshaD
Copy link
Contributor

Related Issues

This pull request refactors and improves the event publishing logic in the user registration and recovery flow handlers. The main changes include standardizing how events are published, improving naming consistency, and ensuring correct event triggers during registration and recovery scenarios. The updates also enhance error handling and code readability.

Event Publishing Refactor:

  • Introduced a new publishEvent method in FlowRegistrationCompletionHandler.java to standardize event publishing, including all relevant user and event properties. This replaces previous ad-hoc event publishing logic.
  • Updated logic in handleEvent to use the new publishEvent method and ensure events are only published under the correct conditions, such as notification channel verification and configuration checks. [1] [2]

Naming and Consistency Improvements:

  • Renamed variables for clarity (e.g., domainName to userStoreDomain, isNotificationInternallyManage to isNotificationInternallyManaged) and ensured consistent usage throughout the handlers. [1] [2] [3]

Listener Event Handling Updates:

  • Refactored event publishing in FlowCompletionListener.java to use a single, parameterized publishEvent method, ensuring consistent event names and error handling across registration, invitation, and password recovery flows. [1] [2] [3] [4] [5]

Error Handling Enhancements:

  • Improved error logging when event publishing fails, making it easier to diagnose issues during registration and recovery flows. [1] [2]

Code Cleanup:

  • Removed redundant and unused event publishing methods, consolidating logic for better maintainability and readability.

Copilot AI review requested due to automatic review settings January 20, 2026 07:11
Copy link

@wso2-engineering wso2-engineering bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

  • 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
#### Log Improvement Suggestion No: 3
#### Log Improvement Suggestion No: 4
#### Log Improvement Suggestion No: 5
#### Log Improvement Suggestion No: 6

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors the event publishing logic in user registration and recovery flows to fix issues where events were not firing correctly. The changes standardize event publishing across handlers, improve naming consistency, and enhance error handling.

Changes:

  • Introduced a new publishEvent method in FlowRegistrationCompletionHandler to handle USER_REGISTRATION_SUCCESS events with proper conditions
  • Refactored FlowCompletionListener.publishEvent to accept an event name parameter and removed the overloaded variant, consolidating event publishing logic
  • Renamed variables from domainName to userStoreDomain and isNotificationInternallyManage to isNotificationInternallyManaged for consistency
  • Updated test cases to reflect new event publishing behavior with correct assertion counts

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/handler/FlowRegistrationCompletionHandler.java Added new publishEvent method, improved variable naming (domainName→userStoreDomain), and implemented logic to publish USER_REGISTRATION_SUCCESS event when notification channel is verified or confirmation is disabled
components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/listener/FlowCompletionListener.java Refactored publishEvent to take event name parameter, removed old overload, added separate error handling for IdentityEventException, and updated calls to use new signature with POST_ADD_NEW_PASSWORD event
components/org.wso2.carbon.identity.recovery/src/test/java/org/wso2/carbon/identity/recovery/listener/FlowCompletionListenerTest.java Updated test expectations to verify correct event publishing counts, added new test cases for various scenarios (SMS channel, external channel, event exceptions), and removed obsolete helper methods
components/org.wso2.carbon.identity.recovery/src/test/java/org/wso2/carbon/identity/recovery/handler/FlowRegistrationCompletionHandlerTest.java Added comprehensive test coverage for new event publishing logic including tests for account lock scenarios, notification channel verification, externally managed notifications, and event exception handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

❌ Patch coverage is 77.27273% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.61%. Comparing base (52d8c40) to head (98dc1cc).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
...ery/handler/FlowRegistrationCompletionHandler.java 82.75% 2 Missing and 3 partials ⚠️
...tity/recovery/listener/FlowCompletionListener.java 66.66% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1088      +/-   ##
============================================
+ Coverage     54.35%   54.61%   +0.26%     
- Complexity     3158     3187      +29     
============================================
  Files           316      316              
  Lines         21650    21675      +25     
  Branches       4173     4178       +5     
============================================
+ Hits          11768    11838      +70     
+ Misses         8376     8336      -40     
+ Partials       1506     1501       -5     
Flag Coverage Δ
unit 45.85% <77.27%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ImalshaD
Copy link
Contributor Author

Verification Flow 1: (Self registration without in flow Notification channel Verification)

  • Event Published after self register.
  • Flow Completion Email Sent.
NormalScenario.mp4

Verification Flow 2: (Self registration without in flow Notification channel Verification + account Lock)

  • Verification Email sent.
  • Event Published after Account unlock.
AccountLockScenario.mp4

Verification Flow 3: (Self registration without in flow Notification channel Verification + account confirm)

  • Verification Email sent.
  • Event Published after Confirmation. [This is the existing behavior in governance self registration]

Verification Flow 4: (Self Registration + workflow)

  • Event Published after approval.
  • Flow Completion Email Sent.
WorkFlowEngaged.mp4

Verification Flow 5: (Self Registration + workflow + account lock)

  • Verification Email sent.
  • Event Published after Account unlock.
AccountLockOnWorkFlow.mp4

Verification Flow 6: (Self registration without in flow Notification channel Verification)

  • Event Published after self register.
  • Flow Completion Email Sent.
VerifeidNotificationChannelScenario.mp4

Verification Flow 7: (Self Registration with federated IDP (Email channel is considered verified)

  • Event Published after self register.
  • Flow Completion Email Sent.
FederatedScenario.mp4

Verification Flow 8: (Password Reset)

  • Event Published after password reset.[credentialUpdate]
  • Flow Completion Email Sent.
credentialUpdateEventNotFired.mp4

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ZiyamSanthosh
ZiyamSanthosh previously approved these changes Jan 20, 2026
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/21164595937

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/21164595937
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/21164595937

@ImalshaD ImalshaD force-pushed the imalshaD/fix_events_not_fired branch from e4a1b3b to 278c229 Compare January 20, 2026 12:32
@ImalshaD
Copy link
Contributor Author

ImalshaD commented Jan 20, 2026

Note

For the Scenario where Self registration is enabled with email confirmation without account locking

As discussed offline with @ashanthamara, this change currently results in two registrationSuccess events being fired. The event triggered during the confirmation step should be removed, as the event introduced here represents the correct and intended registrationSuccess event.

Will be tracked with wso2/product-is#26567

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/21172101859

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/21172101859
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/21172101859

}
publishEvent(user, confirmationCode, recoveryScenario);
} catch (UserStoreException | IdentityRecoveryException | IdentityEventException | FlowMgtServerException e) {
publishEvent(user, confirmationCode, recoveryScenario, IdentityEventConstants.Event.POST_ADD_NEW_PASSWORD);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure whether the POST_ADD_NEW_PASSWORD correct event name for this case ?

Copy link
Contributor Author

@ImalshaD ImalshaD Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, According to webhook implementations (https://github.com/wso2-extensions/identity-webhook-event-handlers/blob/4947f1747783207fe2b3b2546f5b068d35403eca/components/org.wso2.identity.webhook.common.event.handler/src/main/java/org/wso2/identity/webhook/common/event/handler/internal/handler/RegistrationEventHookHandler.java#L167) this is captured from webhook side and converted to registrationSuccess event.

Please refert to below comment for verification on event publishing on new invited user registration flow.
#1088 (comment)

@ImalshaD ImalshaD force-pushed the imalshaD/fix_events_not_fired branch from 278c229 to 98dc1cc Compare January 21, 2026 03:33
@ImalshaD
Copy link
Contributor Author

Verification on correct event publishing on invited user reg flow.

Screen.Recording.2026-01-21.at.09.08.32.mp4

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/21197098117

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/21197098117
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/21197098117

@ashanthamara ashanthamara merged commit 5d7559d into wso2-extensions:master Jan 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants