MX-241: Implement asynchronous notification system#149
Merged
IOhacker merged 1 commit intoopenMF:developfrom Apr 14, 2026
Merged
MX-241: Implement asynchronous notification system#149IOhacker merged 1 commit intoopenMF:developfrom
IOhacker merged 1 commit intoopenMF:developfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (40)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive notification system for self-service user events, including account activation, successful login, and failed login attempts. It adds asynchronous event publishing and processing infrastructure with email and SMS delivery capabilities, integrates event publishing into authentication and registration flows, and provides corresponding Thymeleaf templates and internationalized messages. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as SelfAuthenticationApiResource
participant Provider as DaoAuthenticationProvider
participant Checker as SelfServiceUserDetailsChecker
participant Context as NotificationContext
participant Publisher as ApplicationEventPublisher
participant Handler as SelfServiceNotificationService
participant Email as EmailService
participant SMS as SmsService
User->>API: POST /authenticate
API->>Provider: authenticate(credentials)
Provider->>Checker: check(userDetails)
alt User Disabled or Locked
Checker->>Context: bind(LOGIN_FAILURE)
Checker->>Publisher: publishEvent(SelfServiceNotificationEvent)
Publisher->>Handler: handleNotification(event)
Handler->>Handler: Check cooldown cache
Handler->>Email: sendDefinedEmail(...)
Handler->>SMS: sendMessage(...)
Checker->>Context: clear()
else User Authenticated
Provider-->>API: AuthenticationToken
API->>Context: bind(LOGIN_SUCCESS)
API->>Publisher: publishEvent(SelfServiceNotificationEvent)
Publisher->>Handler: handleNotification(event)
Handler->>Handler: Check cooldown cache
Handler->>Email: sendDefinedEmail(...)
Handler->>SMS: sendMessage(...)
API->>Context: clear()
API-->>User: Success response
end
sequenceDiagram
participant Service as SelfServiceRegistrationWritePlatformServiceImpl
participant Repository as SelfServiceRegistrationRepository
participant UserSvc as AppSelfServiceUserService
participant Sync as TransactionSynchronization
participant Publisher as ApplicationEventPublisher
participant Handler as SelfServiceNotificationService
participant Email as EmailService
participant SMS as SmsService
Service->>Repository: Get registration request
Service->>Repository: Mark consumed (within transaction)
Service->>UserSvc: Enable user
Service->>UserSvc: Save user
Service->>Sync: Register afterCommit synchronization
Note over Repository,Sync: Transaction commits
Sync->>Publisher: publishEvent(SelfServiceNotificationEvent.USER_ACTIVATED)
Publisher->>Handler: handleNotification(event)
Handler->>Handler: Check cooldown cache
Handler->>Handler: Determine delivery mode
alt Email mode
Handler->>Email: sendDefinedEmail(...)
else SMS mode
Handler->>SMS: sendMessage(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceRegistrationNotFoundException.java (1)
28-39:⚠️ Potential issue | 🔴 CriticalDuplicate constructor with identical signature - compilation error.
There are two constructors with the same signature
(String):
- Lines 28-32
- Lines 34-39
This will cause a compilation failure. One constructor must be removed or the signature must be differentiated.
Proposed fix - remove duplicate constructor
public SelfServiceRegistrationNotFoundException(String externalAuthenticationToken) { super( "error.msg.self.service.registration.not.found", - "Self service request not found for provided external authentication token"); + "Self service registration not found with external authentication token : " + + externalAuthenticationToken); } - - public SelfServiceRegistrationNotFoundException(String externalAuthenticationToken) { - super( - "error.msg.self.service.registration.not.found", - "Self service registration not found with external authentication token : " - + externalAuthenticationToken); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceRegistrationNotFoundException.java` around lines 28 - 39, The class SelfServiceRegistrationNotFoundException defines two identical constructors SelfServiceRegistrationNotFoundException(String externalAuthenticationToken) causing a duplicate-signature compile error; remove the redundant constructor (keep the clearer message variant) or change one constructor signature to accept additional context (e.g., Throwable cause or a different parameter name/type) so only one constructor signature exists; update usages of SelfServiceRegistrationNotFoundException accordingly if you alter the signature.src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java (1)
108-120:⚠️ Potential issue | 🟠 MajorReject blank credentials before calling the authentication provider.
This guard only rejects
null. Payloads like{"username":""}and{"password":""}still reach the provider, so the endpoint no longer returns the intended validation error for empty credentials.Suggested fix
- if (request.username == null || request.password == null) { + if (StringUtils.isBlank(request.username) || StringUtils.isBlank(request.password)) { throw new IllegalArgumentException("Username or Password is null in JSON (see FINERACT-726) of POST to /authentication: " + apiRequestBodyAsJson + "; username=" + request.username + ", password=" + request.password); }As per coding guidelines, "Validate and sanitize all incoming data (use Bean Validation with
@Validand constraints)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java` around lines 108 - 120, The authenticate method in SelfAuthenticationApiResource currently only checks for nulls and allows empty-string credentials; update validation to reject blank username/password before invoking the authentication provider by validating AuthenticateRequest (either add Bean Validation annotations to AuthenticateRequest and annotate the method param with `@Valid` or explicitly check StringUtils.isBlank(request.username) || StringUtils.isBlank(request.password) after deserialization), and throw a clear IllegalArgumentException (or appropriate WebApplicationException) including the incoming payload and which field is blank; ensure references: authenticate(...), SelfAuthenticationApiResource, and AuthenticateRequest so the guard runs prior to any provider call.
🧹 Nitpick comments (15)
src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java (2)
198-200: Silent false return for nullnowparameter may mask bugs.Returning
falsewhennowisnullsilently treats invalid input as "not expired." While current callers always pass non-null values viaDateUtils.getLocalDateTimeOfSystem(), consider throwingIllegalArgumentExceptionfor null input to fail fast on misuse.Alternative: Validate input
public boolean isExpired(LocalDateTime now) { - return this.expiresAt != null && now != null && !now.isBefore(this.expiresAt); + if (now == null) { + throw new IllegalArgumentException("now must not be null"); + } + return this.expiresAt != null && !now.isBefore(this.expiresAt); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java` around lines 198 - 200, The isExpired method currently returns false when the now parameter is null which can mask caller bugs; update SelfServiceRegistration.isExpired(LocalDateTime now) to validate input by throwing an IllegalArgumentException if now is null, then keep the existing expiry check using this.expiresAt (i.e., ensure the method first checks now != null and throws with a clear message, then return this.expiresAt != null && !now.isBefore(this.expiresAt)).
162-168: Duplicate getters with inconsistent naming may cause confusion.Both
getExternalAuthenticationToken()andgetExternalAuthorizationToken()return the same field (externalAuthorizationToken). The naming inconsistency between "Authentication" and "Authorization" in the method names versus the field name could lead to maintenance confusion.Consider deprecating one method and standardizing on a single naming convention.
Option: Deprecate the old getter
+ /** + * `@deprecated` Use {`@link` `#getExternalAuthenticationToken`()} instead. + */ + `@Deprecated` public String getExternalAuthorizationToken() { return this.externalAuthorizationToken; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java` around lines 162 - 168, There are two duplicate getters returning the same field externalAuthorizationToken: getExternalAuthenticationToken() and getExternalAuthorizationToken(), which is confusing; standardize to a single getter name (prefer getExternalAuthorizationToken to match the field) and mark the other as deprecated. Specifically, keep getExternalAuthorizationToken() as the canonical method, annotate getExternalAuthenticationToken() with `@Deprecated` and have it delegate to getExternalAuthorizationToken(), update any call sites to use getExternalAuthorizationToken(), and add a short Javadoc on the deprecated method pointing to the preferred getter.src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceUserDetailsChecker.java (1)
29-34: Markdelegatefield asfinal.Constructor-injected fields should be marked
finalper coding guidelines.Proposed fix
public class SelfServiceUserDetailsChecker implements UserDetailsChecker { - private final UserDetailsChecker delegate; + private final UserDetailsChecker delegate; public SelfServiceUserDetailsChecker(UserDetailsChecker delegate) { this.delegate = delegate; }Note: The field is already
finalin the code. No change needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceUserDetailsChecker.java` around lines 29 - 34, The review requests marking the constructor-injected field 'delegate' as final, but in SelfServiceUserDetailsChecker the field 'delegate' is already declared final; no code changes are needed—leave the existing 'private final UserDetailsChecker delegate' and the constructor SelfServiceUserDetailsChecker(UserDetailsChecker delegate) as-is and dismiss the comment.src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java (2)
57-63: Minor concurrency consideration input()method.The sequence
pruneExpiredEntries()→ size check →removeOneEntry()→entries.put()is not atomic. Under high concurrency, the cache size may temporarily exceedmaximumSize. For a rate-limiting cooldown cache, this is acceptable behavior, but document this as an intentional soft limit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java` around lines 57 - 63, The put() method currently calls pruneExpiredEntries() then checks entries.size() against maximumSize and possibly calls removeOneEntry() before entries.put(), which is not atomic and can briefly allow entries to exceed maximumSize under concurrency; update documentation/comments near put(), pruneExpiredEntries(), removeOneEntry(), entries, maximumSize, ttl and clock.instant() to state this is an intentional soft limit for the cooldown cache (i.e., occasional temporary overshoot under high concurrency is acceptable) and note that if strict max enforcement is later required the methods should be reworked to use a thread-safe atomic eviction strategy or external synchronization.
70-75:removeOneEntry()evicts an arbitrary entry rather than the oldest.When at capacity, this method removes whichever entry the iterator returns first, which is not deterministic for
ConcurrentHashMap. Consider whether LRU or FIFO eviction would better serve the use case, or document the current behavior as acceptable for cooldown purposes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java` around lines 70 - 75, The current removeOneEntry() removes an arbitrary key from the ConcurrentHashMap entries (not the oldest), which is non-deterministic; change eviction to deterministic FIFO or LRU instead of removing iterator.next(). Either (A) implement FIFO: add a ConcurrentLinkedQueue<String> insertionOrderQueue and on put(key, value) offer the key to the queue, then in removeOneEntry() poll() the queue and remove that key from entries, avoiding stale queue entries; or (B) implement LRU: replace the map with a synchronized LinkedHashMap<String,Value> with accessOrder=true and override removeEldestEntry to evict when size > capacity (or wrap it with Collections.synchronizedMap and ensure all access uses the map), and update code that references entries accordingly; update methods that insert/remove to maintain the chosen order structure and ensure thread-safety for concurrent access in NotificationCooldownCache and in methods put/remove/get that use entries.src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceDisabledException.java (1)
25-34: Missing Javadoc on public class.Public classes require Javadoc documentation per coding guidelines.
Add class-level Javadoc
+/** + * Exception thrown when a self-service user account is disabled, carrying the + * associated {`@link` AppSelfServiceUser} for notification event publishing. + */ `@Getter` public class SelfServiceDisabledException extends DisabledException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceDisabledException.java` around lines 25 - 34, Add a class-level Javadoc comment above the public class SelfServiceDisabledException that succinctly describes the purpose of this exception (a DisabledException wrapper used when a self-service user is disabled), mentions the stored AppSelfServiceUser payload (field user) and when it is thrown/used (constructed via the SelfServiceDisabledException(String msg, AppSelfServiceUser user) constructor), and any relevant behavior inherited from DisabledException; keep it brief and follow project Javadoc style.src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceLockedException.java (1)
25-34: Missing Javadoc on public class.Public classes require Javadoc documentation per coding guidelines.
Add class-level Javadoc
+/** + * Exception thrown when a self-service user account is locked, carrying the + * associated {`@link` AppSelfServiceUser} for notification event publishing. + */ `@Getter` public class SelfServiceLockedException extends LockedException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceLockedException.java` around lines 25 - 34, Add a class-level Javadoc to SelfServiceLockedException describing its purpose (a LockedException wrapper that carries an AppSelfServiceUser), document the constructor parameters (msg and user) and any relevant tags (e.g., `@see` LockedException, `@since`) so the public class meets coding guidelines; place the Javadoc immediately above the "public class SelfServiceLockedException" declaration and mention the stored field "user" and its role.src/main/resources/notification/templates/text/login-failure.txt (1)
3-3: Timestamp format inconsistent with other templates.This template uses
'yyyy-MM-dd HH:mm:ss'without timezone, whilelogin-success.txtandlogin-success.htmluse'yyyy-MM-dd HH:mm:ss z'with timezone. For consistency in user notifications, consider aligning the format.Proposed fix
-There was a failed attempt to login to your account ([[${username}]]) at [[${`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss')}]]. +There was a failed attempt to login to your account ([[${username}]]) at [[${`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss z')}]].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/notification/templates/text/login-failure.txt` at line 3, Update the timestamp format in the login-failure template to match the other login templates: change the format string used in the [[${`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss')}]] expression to include the timezone (use 'yyyy-MM-dd HH:mm:ss z') so the notification uses the same timezone-aware format as login-success.txt and login-success.html.src/main/resources/notification/templates/html/user-activated.html (1)
1-9: Template lacks<head>section and meta tags.Unlike
login-success.htmlwhich includes<head>,<meta charset>, and<meta viewport>, this template omits these elements. For email rendering consistency and proper character encoding, consider adding them.Proposed structure alignment
<!DOCTYPE html> <html xmlns:th="http://www.thymeleaf.org"> +<head> + <meta charset="UTF-8"/> + <meta name="viewport" content="width=device-width, initial-scale=1.0"/> + <title>Account Activated</title> +</head> <body> <h2>Welcome <span th:text="${firstName}">User</span>,</h2>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/notification/templates/html/user-activated.html` around lines 1 - 9, The user-activated.html email template is missing a <head> section and essential meta tags which can cause inconsistent rendering; update the template (user-activated.html) to include a <head> block mirroring login-success.html with at least <meta charset="utf-8"> and a <meta name="viewport" content="width=device-width, initial-scale=1"> (and any other shared head elements used in login-success.html) so the document has proper encoding and responsive behavior while leaving the existing body and Thymeleaf expressions (<span th:text="${firstName}"> and <strong th:text="${username}">) unchanged.src/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.java (1)
110-111: Use an explicit import instead of inline fully qualified name.The
ApplicationEventPublisheris referenced using its fully qualified name. For consistency with the rest of the file, add an import statement.Proposed fix
Add import at the top of the file:
import org.springframework.context.ApplicationEventPublisher;Then update the parameter:
- org.springframework.context.ApplicationEventPublisher applicationEventPublisher) { + ApplicationEventPublisher applicationEventPublisher) {As per coding guidelines: "Avoid wildcard imports; import explicitly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.java` around lines 110 - 111, Replace the inline fully-qualified type with an explicit import: add "import org.springframework.context.ApplicationEventPublisher;" at the top of the file and change the constructor parameter signature in SelfRegistrationConfiguration from "org.springframework.context.ApplicationEventPublisher applicationEventPublisher" to "ApplicationEventPublisher applicationEventPublisher" (keeping the other parameters like SelfServiceAuthorizationTokenService unchanged) so the class uses a consistent explicit import rather than the fully-qualified name.src/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java (1)
94-112: Debugging helper is useful; consider usingCollectors.joining()for efficiency.The
fineractLogTail()method improves test failure diagnostics. However, usingreduce()for string concatenation is less efficient thanCollectors.joining(System.lineSeparator()).Proposed fix
String filtered = logs.lines() .filter(line -> line.contains("ERROR") || line.contains("Exception") || line.contains("Caused by:") || line.contains("SelfForgotPassword") || line.contains("/self/password/request")) - .reduce((left, right) -> left + System.lineSeparator() + right) - .orElse(""); + .collect(java.util.stream.Collectors.joining(System.lineSeparator()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java` around lines 94 - 112, The string concatenation in fineractLogTail() uses Stream.reduce which is inefficient; replace the reduce(...) that builds 'filtered' with use of Collectors.joining(System.lineSeparator()) on logs.lines().filter(...) and assign that result to 'filtered', adding the necessary import for java.util.stream.Collectors; keep the same filter predicates and the subsequent blank-check and truncation logic unchanged (references: fineractLogTail(), fineract.getLogs(), variable 'logs' and 'filtered').src/main/resources/notification/templates/html/login-failure.html (1)
5-5: Timestamp lacks timezone indicator for the recipient.The formatted timestamp does not convey timezone information to the user. Consider appending the timezone abbreviation (e.g.,
'yyyy-MM-dd HH:mm:ss z') or converting to the user's locale timezone if available, to prevent confusion about when the event occurred.Proposed fix
-<p>There was a failed attempt to login to your account (<strong th:text="${username}">username</strong>) at <th:block th:text="${`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss')}"></th:block>.</p> +<p>There was a failed attempt to login to your account (<strong th:text="${username}">username</strong>) at <th:block th:text="${`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss z')}"></th:block>.</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/notification/templates/html/login-failure.html` at line 5, The timestamp in the login-failure template uses `#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss') and therefore omits timezone info; update the Thymeleaf expression in login-failure.html (the <th:block> that formats eventTimestamp) to include timezone, e.g. change the format pattern to 'yyyy-MM-dd HH:mm:ss z' or convert eventTimestamp to the recipient's ZoneId before formatting (e.g. obtain a ZonedDateTime from eventTimestamp and pass that to `#temporals.format`) so the rendered timestamp includes a timezone/offset.src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java (1)
213-244: Consider verifying event publication in user creation tests.The
createSelfServiceUser_returnsUserWithIdand related tests do not verify thatapplicationEventPublisher.publishEvent()is called with the expectedSelfServiceNotificationEvent. Adding verification would confirm the notification integration works as intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java` around lines 213 - 244, Add verification that applicationEventPublisher.publishEvent(...) is invoked with a SelfServiceNotificationEvent in the createSelfServiceUser_returnsUserWithId test (and similar user-creation tests): after calling service.createSelfServiceUser("...") capture or verify the published event by calling Mockito.verify(applicationEventPublisher).publishEvent(argThat(e -> e instanceof SelfServiceNotificationEvent && /* optionally inspect properties like getRegistrationId()/getUsername() matching registration.getUsername() */)); if you need to inspect the event payload more precisely, use an ArgumentCaptor<SelfServiceNotificationEvent> to capture the argument and assert its fields (e.g., registration id/username/email) match the mocked SelfServiceRegistration; keep existing tenant cleanup in the finally block.src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java (2)
22-22: Rename integration test class to*ITto match project test naming conventions.
SelfServiceNotificationAsyncIntegrationTestshould follow the repository integration-test naming pattern (e.g.,SelfServiceNotificationAsyncIT).As per coding guidelines, "Name tests
ClassNameTestorClassNameIT(for integration tests)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java` at line 22, The test class SelfServiceNotificationAsyncIntegrationTest should be renamed to SelfServiceNotificationAsyncIT: update the class declaration (public class SelfServiceNotificationAsyncIntegrationTest → public class SelfServiceNotificationAsyncIT), rename the file accordingly, and update any references/imports or build/test configurations that reference the old class name so the test follows the repository integration-test naming convention.
37-44: ThepublishDuration < 500msassertion is prone to CI flakiness.Line 43 depends on machine/load timing rather than synchronization state. Prefer asserting non-blocking publish via latch/future coordination instead of a fixed wall-clock threshold.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java` around lines 37 - 44, The test's publishDuration < 500ms timing check is flaky; instead create a CompletableFuture (or CountDownLatch) that the async listener completes when it finishes handling SelfServiceNotificationEvent, register that listener in the test, call applicationEventPublisher.publishEvent(new SelfServiceNotificationEvent(...)), then immediately assert that the future is not yet completed (assertFalse(future.isDone()) or latch.getCount() == 1) to prove publish was non-blocking, and finally wait with a bounded timeout for the future to complete (future.get(500, TimeUnit.MILLISECONDS) or latch.await(500, TimeUnit.MILLISECONDS)) to verify the listener ran; remove the publishDuration variable and timing assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java`:
- Around line 104-125: The code assumes event.isEmailMode() maps to an available
destination; instead, before sending verify the actual destination is present:
when event.isEmailMode() is true check event.getEmail() is non-null/non-empty
(and only then call buildRecipientName(...) and
emailService.sendDefinedEmail(...)), otherwise log a warning and return;
likewise when emailMode is false check event.getMobileNumber() is
non-null/non-empty before creating/saving SmsMessage and calling
smsMessageRepository.save(...) and
smsScheduledJobService.sendTriggeredMessage(...), otherwise log a warning and
return; keep existing template processing but perform the destination checks
immediately before the send/save calls to skip the channel cleanly.
- Around line 85-90: The cooldown is currently started too early by calling
notificationCooldownCache.put(cacheKey) immediately after computing cacheKey in
SelfServiceNotificationService; move this call so the cache is populated only
after a successful handoff (i.e., after template rendering, provider lookup and
the send or scheduling call completes successfully). Update the logic around
notificationCooldownCache.put(cacheKey) so it is invoked only on successful
delivery/enqueue (or in the success callback/future completion for async sends)
and ensure failures/exceptions during rendering, provider resolution, or
send/schedule do not call put, so retries are not suppressed.
In
`@src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java`:
- Around line 48-61: The notificationExecutor bean currently does not propagate
thread-local state to worker threads causing repository calls (e.g.,
smsMessageRepository.save invoked from handleNotification via
`@Async`("notificationExecutor")) to run without the active tenant/business-date;
fix by calling executor.setTaskDecorator(...) on the ThreadPoolTaskExecutor
returned by notificationExecutor and implement a TaskDecorator that captures
ThreadLocalContextUtil.getTenant() and ThreadLocalContextUtil.getBusinessDates()
before running the delegate, sets them via ThreadLocalContextUtil.setTenant(...)
and setBusinessDates(...) inside the decorated Runnable, and clears the
tenant/business-date (e.g., ThreadLocalContextUtil.clearTenant()) in a finally
block after runnable.run() so context is restored/cleared for each worker
thread.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 500-521: The USER_ACTIVATED event is currently published inside
the transaction (see applicationEventPublisher.publishEvent(...) in
SelfServiceRegistrationWritePlatformServiceImpl, called from confirmEnrollment),
which can trigger notifications even if the transaction later rolls back; change
this to register a post-commit callback using
TransactionSynchronizationManager.registerSynchronization() that publishes the
org.apache.fineract.selfservice.notification.SelfServiceNotificationEvent (the
same parameters currently passed to applicationEventPublisher.publishEvent)
inside afterCommit(), so the event is only published once the transaction
successfully commits.
In
`@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java`:
- Around line 133-139: The LOGIN_SUCCESS notification is being published too
early (inside NotificationContext.bind and via
applicationEventPublisher.publishEvent for SelfServiceNotificationEvent) so
users who hit SelfServicePasswordResetRequiredException still receive a success
event; modify the flow in SelfAuthenticationApiResource so that the
NotificationContext.Scope bind and the publishEvent for
SelfServiceNotificationEvent.Type.LOGIN_SUCCESS occur only after the
password-renewal check/branch completes successfully (i.e., after the code that
can throw SelfServicePasswordResetRequiredException), or guard the publish with
an explicit check that the password-renewal path did not trigger, ensuring no
LOGIN_SUCCESS event is emitted when SelfServicePasswordResetRequiredException is
raised.
- Around line 140-143: The warning log in the catch block that handles failures
to publish the login success notification currently logs request.username
(sensitive PII); in the SelfAuthenticationApiResource catch for the
publish-login-success notification, remove request.username from the log call
and replace it with a non-PII identifier or a generic event message (e.g.,
"login success notification" or an internal user id if a non-PII id variable is
available), i.e., update the log.warn invocation that currently references
request.username to only include safe context and the exception `e`.
- Around line 233-245: The extractClientIp method currently trusts the raw
X-Forwarded-For header which is client-controllable; change it to prefer the
container-provided client address (httpRequest.getRemoteAddr()) and only use
X-Forwarded-For when a trusted-proxy/preprocessing step has already validated or
normalized it (i.e., document/guard with a central trusted-proxy flag or use a
framework-provided forwarded-header filter), and sanitize any forwarded value
before returning; update extractClientIp to return httpRequest.getRemoteAddr()
by default and only parse X-Forwarded-For under a verified/trusted condition (or
remove its use here) to prevent attacker-controlled IPs from being recorded.
- Around line 145-176: The current handlers for SelfServiceDisabledException and
SelfServiceLockedException call applicationEventPublisher.publishEvent(...)
inside the same try/finally that ends by rethrowing ex, so if publishEvent
throws the notification error the original exception is lost; to fix, wrap only
the publishEvent call (and any NotificationContext.set/clear) in its own
try/catch that catches/logs notification publishing exceptions (e.g., log the
failure) but does not swallow or replace the original
SelfServiceDisabledException/SelfServiceLockedException, then after that inner
block rethrow the original ex; reference the existing symbols
SelfServiceDisabledException, SelfServiceLockedException,
NotificationContext.set/clear, and applicationEventPublisher.publishEvent to
locate and modify the code.
In
`@src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.java`:
- Around line 406-412: AppSelfServiceUser contains two identical-signature
methods named enable(); remove the duplicate that only sets enabled = true (the
shorter enable() method) or consolidate so there's a single public void enable()
that sets enabled = true, accountNonExpired = true, accountNonLocked = true,
credentialsNonExpired = true, and deleted = false; update any callers if they
relied on the shorter version to ensure they get the consolidated behavior and
keep only one enable() method in the class.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java`:
- Around line 21-22: The test currently only wires AsyncTestConfig and a
TestNotificationListener, so it never exercises the production listener; modify
the test context to load the actual SelfServiceNotificationService instead of
the test listener by importing or component-scanning the production component
(ensure AsyncTestConfig still provides the executor/template via
SelfServiceNotificationConfig), remove or replace the TestNotificationListener
`@Bean`, and update the assertions to validate that SelfServiceNotificationService
is invoked asynchronously (e.g., by observing the same async side-effects used
before). Locate AsyncTestConfig, SelfServiceNotificationConfig,
SelfServiceNotificationService and TestNotificationListener in the test to make
these changes.
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceRegistrationNotFoundException.java`:
- Around line 28-39: The class SelfServiceRegistrationNotFoundException defines
two identical constructors SelfServiceRegistrationNotFoundException(String
externalAuthenticationToken) causing a duplicate-signature compile error; remove
the redundant constructor (keep the clearer message variant) or change one
constructor signature to accept additional context (e.g., Throwable cause or a
different parameter name/type) so only one constructor signature exists; update
usages of SelfServiceRegistrationNotFoundException accordingly if you alter the
signature.
In
`@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java`:
- Around line 108-120: The authenticate method in SelfAuthenticationApiResource
currently only checks for nulls and allows empty-string credentials; update
validation to reject blank username/password before invoking the authentication
provider by validating AuthenticateRequest (either add Bean Validation
annotations to AuthenticateRequest and annotate the method param with `@Valid` or
explicitly check StringUtils.isBlank(request.username) ||
StringUtils.isBlank(request.password) after deserialization), and throw a clear
IllegalArgumentException (or appropriate WebApplicationException) including the
incoming payload and which field is blank; ensure references: authenticate(...),
SelfAuthenticationApiResource, and AuthenticateRequest so the guard runs prior
to any provider call.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java`:
- Around line 57-63: The put() method currently calls pruneExpiredEntries() then
checks entries.size() against maximumSize and possibly calls removeOneEntry()
before entries.put(), which is not atomic and can briefly allow entries to
exceed maximumSize under concurrency; update documentation/comments near put(),
pruneExpiredEntries(), removeOneEntry(), entries, maximumSize, ttl and
clock.instant() to state this is an intentional soft limit for the cooldown
cache (i.e., occasional temporary overshoot under high concurrency is
acceptable) and note that if strict max enforcement is later required the
methods should be reworked to use a thread-safe atomic eviction strategy or
external synchronization.
- Around line 70-75: The current removeOneEntry() removes an arbitrary key from
the ConcurrentHashMap entries (not the oldest), which is non-deterministic;
change eviction to deterministic FIFO or LRU instead of removing
iterator.next(). Either (A) implement FIFO: add a ConcurrentLinkedQueue<String>
insertionOrderQueue and on put(key, value) offer the key to the queue, then in
removeOneEntry() poll() the queue and remove that key from entries, avoiding
stale queue entries; or (B) implement LRU: replace the map with a synchronized
LinkedHashMap<String,Value> with accessOrder=true and override removeEldestEntry
to evict when size > capacity (or wrap it with Collections.synchronizedMap and
ensure all access uses the map), and update code that references entries
accordingly; update methods that insert/remove to maintain the chosen order
structure and ensure thread-safety for concurrent access in
NotificationCooldownCache and in methods put/remove/get that use entries.
In
`@src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java`:
- Around line 198-200: The isExpired method currently returns false when the now
parameter is null which can mask caller bugs; update
SelfServiceRegistration.isExpired(LocalDateTime now) to validate input by
throwing an IllegalArgumentException if now is null, then keep the existing
expiry check using this.expiresAt (i.e., ensure the method first checks now !=
null and throws with a clear message, then return this.expiresAt != null &&
!now.isBefore(this.expiresAt)).
- Around line 162-168: There are two duplicate getters returning the same field
externalAuthorizationToken: getExternalAuthenticationToken() and
getExternalAuthorizationToken(), which is confusing; standardize to a single
getter name (prefer getExternalAuthorizationToken to match the field) and mark
the other as deprecated. Specifically, keep getExternalAuthorizationToken() as
the canonical method, annotate getExternalAuthenticationToken() with `@Deprecated`
and have it delegate to getExternalAuthorizationToken(), update any call sites
to use getExternalAuthorizationToken(), and add a short Javadoc on the
deprecated method pointing to the preferred getter.
In
`@src/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.java`:
- Around line 110-111: Replace the inline fully-qualified type with an explicit
import: add "import org.springframework.context.ApplicationEventPublisher;" at
the top of the file and change the constructor parameter signature in
SelfRegistrationConfiguration from
"org.springframework.context.ApplicationEventPublisher
applicationEventPublisher" to "ApplicationEventPublisher
applicationEventPublisher" (keeping the other parameters like
SelfServiceAuthorizationTokenService unchanged) so the class uses a consistent
explicit import rather than the fully-qualified name.
In
`@src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceDisabledException.java`:
- Around line 25-34: Add a class-level Javadoc comment above the public class
SelfServiceDisabledException that succinctly describes the purpose of this
exception (a DisabledException wrapper used when a self-service user is
disabled), mentions the stored AppSelfServiceUser payload (field user) and when
it is thrown/used (constructed via the SelfServiceDisabledException(String msg,
AppSelfServiceUser user) constructor), and any relevant behavior inherited from
DisabledException; keep it brief and follow project Javadoc style.
In
`@src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceLockedException.java`:
- Around line 25-34: Add a class-level Javadoc to SelfServiceLockedException
describing its purpose (a LockedException wrapper that carries an
AppSelfServiceUser), document the constructor parameters (msg and user) and any
relevant tags (e.g., `@see` LockedException, `@since`) so the public class meets
coding guidelines; place the Javadoc immediately above the "public class
SelfServiceLockedException" declaration and mention the stored field "user" and
its role.
In
`@src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceUserDetailsChecker.java`:
- Around line 29-34: The review requests marking the constructor-injected field
'delegate' as final, but in SelfServiceUserDetailsChecker the field 'delegate'
is already declared final; no code changes are needed—leave the existing
'private final UserDetailsChecker delegate' and the constructor
SelfServiceUserDetailsChecker(UserDetailsChecker delegate) as-is and dismiss the
comment.
In `@src/main/resources/notification/templates/html/login-failure.html`:
- Line 5: The timestamp in the login-failure template uses
`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss') and therefore omits
timezone info; update the Thymeleaf expression in login-failure.html (the
<th:block> that formats eventTimestamp) to include timezone, e.g. change the
format pattern to 'yyyy-MM-dd HH:mm:ss z' or convert eventTimestamp to the
recipient's ZoneId before formatting (e.g. obtain a ZonedDateTime from
eventTimestamp and pass that to `#temporals.format`) so the rendered timestamp
includes a timezone/offset.
In `@src/main/resources/notification/templates/html/user-activated.html`:
- Around line 1-9: The user-activated.html email template is missing a <head>
section and essential meta tags which can cause inconsistent rendering; update
the template (user-activated.html) to include a <head> block mirroring
login-success.html with at least <meta charset="utf-8"> and a <meta
name="viewport" content="width=device-width, initial-scale=1"> (and any other
shared head elements used in login-success.html) so the document has proper
encoding and responsive behavior while leaving the existing body and Thymeleaf
expressions (<span th:text="${firstName}"> and <strong th:text="${username}">)
unchanged.
In `@src/main/resources/notification/templates/text/login-failure.txt`:
- Line 3: Update the timestamp format in the login-failure template to match the
other login templates: change the format string used in the
[[${`#temporals.format`(eventTimestamp, 'yyyy-MM-dd HH:mm:ss')}]] expression to
include the timezone (use 'yyyy-MM-dd HH:mm:ss z') so the notification uses the
same timezone-aware format as login-success.txt and login-success.html.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java`:
- Line 22: The test class SelfServiceNotificationAsyncIntegrationTest should be
renamed to SelfServiceNotificationAsyncIT: update the class declaration (public
class SelfServiceNotificationAsyncIntegrationTest → public class
SelfServiceNotificationAsyncIT), rename the file accordingly, and update any
references/imports or build/test configurations that reference the old class
name so the test follows the repository integration-test naming convention.
- Around line 37-44: The test's publishDuration < 500ms timing check is flaky;
instead create a CompletableFuture (or CountDownLatch) that the async listener
completes when it finishes handling SelfServiceNotificationEvent, register that
listener in the test, call applicationEventPublisher.publishEvent(new
SelfServiceNotificationEvent(...)), then immediately assert that the future is
not yet completed (assertFalse(future.isDone()) or latch.getCount() == 1) to
prove publish was non-blocking, and finally wait with a bounded timeout for the
future to complete (future.get(500, TimeUnit.MILLISECONDS) or latch.await(500,
TimeUnit.MILLISECONDS)) to verify the listener ran; remove the publishDuration
variable and timing assertion.
In
`@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java`:
- Around line 213-244: Add verification that
applicationEventPublisher.publishEvent(...) is invoked with a
SelfServiceNotificationEvent in the createSelfServiceUser_returnsUserWithId test
(and similar user-creation tests): after calling
service.createSelfServiceUser("...") capture or verify the published event by
calling Mockito.verify(applicationEventPublisher).publishEvent(argThat(e -> e
instanceof SelfServiceNotificationEvent && /* optionally inspect properties like
getRegistrationId()/getUsername() matching registration.getUsername() */)); if
you need to inspect the event payload more precisely, use an
ArgumentCaptor<SelfServiceNotificationEvent> to capture the argument and assert
its fields (e.g., registration id/username/email) match the mocked
SelfServiceRegistration; keep existing tenant cleanup in the finally block.
In
`@src/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java`:
- Around line 94-112: The string concatenation in fineractLogTail() uses
Stream.reduce which is inefficient; replace the reduce(...) that builds
'filtered' with use of Collectors.joining(System.lineSeparator()) on
logs.lines().filter(...) and assign that result to 'filtered', adding the
necessary import for java.util.stream.Collectors; keep the same filter
predicates and the subsequent blank-check and truncation logic unchanged
(references: fineractLogTail(), fineract.getLogs(), variable 'logs' and
'filtered').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0bb8a1e-17f8-4ab5-9147-bc2f65bb25fc
📒 Files selected for processing (34)
pom.xmlsrc/main/java/org/apache/fineract/selfservice/notification/NotificationContext.javasrc/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.javasrc/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationRepository.javasrc/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceRegistrationNotFoundException.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceDisabledException.javasrc/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceLockedException.javasrc/main/java/org/apache/fineract/selfservice/security/service/SelfServiceUserDetailsChecker.javasrc/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.javasrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_es.propertiessrc/main/resources/notification/messages/NotificationMessages.propertiessrc/main/resources/notification/messages/NotificationMessages_es.propertiessrc/main/resources/notification/templates/html/login-failure.htmlsrc/main/resources/notification/templates/html/login-success.htmlsrc/main/resources/notification/templates/html/user-activated.htmlsrc/main/resources/notification/templates/text/login-failure.txtsrc/main/resources/notification/templates/text/login-success.txtsrc/main/resources/notification/templates/text/user-activated.txtsrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResourceTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.java
Comment on lines
+21
to
+22
| @SpringJUnitConfig(SelfServiceNotificationAsyncIntegrationTest.AsyncTestConfig.class) | ||
| public class SelfServiceNotificationAsyncIntegrationTest { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TEST_FILE="$(fd 'SelfServiceNotificationAsyncIntegrationTest.java' src/test/java | head -n1)"
SERVICE_FILE="$(fd 'SelfServiceNotificationService.java' src/main/java | head -n1)"
echo "== Test context wiring =="
rg -n '@SpringJUnitConfig|@Import|class AsyncTestConfig|class TestNotificationListener|@EventListener|@Async' "$TEST_FILE" -C2
echo
echo "== Production listener wiring =="
rg -n 'class SelfServiceNotificationService|@EventListener|@Async|SelfServiceNotificationEvent' "$SERVICE_FILE" -C2Repository: openMF/selfservice-plugin
Length of output: 2169
🏁 Script executed:
cat -n src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java | head -100Repository: openMF/selfservice-plugin
Length of output: 4871
🏁 Script executed:
fd 'SelfServiceNotificationConfig.java' src/main/java | xargs -I {} cat -n {} | head -80Repository: openMF/selfservice-plugin
Length of output: 4405
Test does not exercise the production async notification listener.
The test context loads only AsyncTestConfig with an @Import of SelfServiceNotificationConfig. The latter provides infrastructure beans (executor, template engine) but no component scanning to load SelfServiceNotificationService. The test (lines 78–80) defines a separate TestNotificationListener registered via @Bean, not the production SelfServiceNotificationService. Consequently, the assertions only validate the test listener's async execution, not whether the production listener is wired or operates asynchronously.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java`
around lines 21 - 22, The test currently only wires AsyncTestConfig and a
TestNotificationListener, so it never exercises the production listener; modify
the test context to load the actual SelfServiceNotificationService instead of
the test listener by importing or component-scanning the production component
(ensure AsyncTestConfig still provides the executor/template via
SelfServiceNotificationConfig), remove or replace the TestNotificationListener
`@Bean`, and update the assertions to validate that SelfServiceNotificationService
is invoked asynchronously (e.g., by observing the same async side-effects used
before). Locate AsyncTestConfig, SelfServiceNotificationConfig,
SelfServiceNotificationService and TestNotificationListener in the test to make
these changes.
DeathGun44
force-pushed
the
MX-241-implement-notifications
branch
2 times, most recently
from
965177a to
f31f28b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResourceTest.java (1)
77-118:⚠️ Potential issue | 🟡 MinorAssert the new notification side effects in these auth tests.
These tests now build the resource with
ApplicationEventPublisher, but they still never assert publisher interactions. The happy path could stop emittingLOGIN_SUCCESS, or the password-reset/locked/disabled branches could emit the wrong events, and this suite would still pass. Add publisher verifications for the success path and explicit no-interaction / failure-branch assertions where appropriate. As per coding guidelines, "Verify both happy path and edge cases."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResourceTest.java` around lines 77 - 118, Tests do not assert interactions with ApplicationEventPublisher, so add verifications: in authenticate_returnsUserDataOnSuccess() verify the injected ApplicationEventPublisher.publishEvent(...) was called once with a LoginEvent (or the event type/enum representing LOGIN_SUCCESS) after calling resource.authenticate(...); in authenticate_throwsPasswordResetExceptionWhenResetRequired() assert that publishEvent was not called (verify(applicationEventPublisher, never()).publishEvent(any())) or that it was called with the specific password-reset event if your design expects that; locate the tests using resource.authenticate(...) and the injected ApplicationEventPublisher mock and add the corresponding verify(...) or verify(..., never()) assertions to cover both happy path and failure branch event emissions.
♻️ Duplicate comments (1)
src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java (1)
26-27:⚠️ Potential issue | 🟠 MajorLoad the production notification listener in this integration test.
This context imports
SelfServiceNotificationConfigbut only registersTestNotificationListener, so it proves that some@Async@EventListener`` can run onnotificationExecutor, not that `src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService` is wired or asynchronous. Replace the bespoke listener with the production bean and assert its observable side effect instead.Also applies to: 65-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java` around lines 26 - 27, The test currently wires SelfServiceNotificationConfig but registers a custom TestNotificationListener; replace that bespoke listener registration in SelfServiceNotificationAsyncIntegrationTest.AsyncTestConfig with the production listener/bean from SelfServiceNotificationConfig (i.e., ensure the actual SelfServiceNotificationService and its `@Async` `@EventListener` bean are loaded) and change assertions to verify the real observable side-effect produced by SelfServiceNotificationService (instead of TestNotificationListener behavior); update any bean definitions or `@Import` statements so the production listener is used and assert its side-effect to prove the real wiring and async execution.
🧹 Nitpick comments (2)
src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenService.java (1)
77-83: Decouple default length resolution by token type.Line 78 uses
DEFAULT_NUMERIC_LENGTHfor all token types. This ties string-token defaults to numeric-policy changes. Resolve token type once and apply type-specific defaults before clamping.Proposed refactor
+ private static final int DEFAULT_STRING_LENGTH = 32; @@ public String generateToken() { - return switch (resolveTokenType()) { - case "string" -> randomString(resolveTokenLength()); - case "numeric" -> randomNumericToken(resolveTokenLength()); + String tokenType = resolveTokenType(); + return switch (tokenType) { + case "string" -> randomString(resolveTokenLength(tokenType)); + case "numeric" -> randomNumericToken(resolveTokenLength(tokenType)); default -> uuidV7(); }; } @@ - private int resolveTokenLength() { - int length = env.getProperty("mifos.self.service.token.length", Integer.class, DEFAULT_NUMERIC_LENGTH); + private int resolveTokenLength(String tokenType) { + int defaultLength = "numeric".equals(tokenType) ? DEFAULT_NUMERIC_LENGTH : DEFAULT_STRING_LENGTH; + int length = env.getProperty("mifos.self.service.token.length", Integer.class, defaultLength); length = Math.min(length, MAX_TOKEN_LENGTH); - if ("numeric".equals(resolveTokenType())) { + if ("numeric".equals(tokenType)) { return Math.max(length, MIN_NUMERIC_LENGTH); } return Math.max(length, 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenService.java` around lines 77 - 83, In resolveTokenLength(), determine the token type once via resolveTokenType() and use a type-specific default (e.g., use DEFAULT_NUMERIC_LENGTH for "numeric" and a separate DEFAULT_STRING_LENGTH for non-numeric) when reading env.getProperty, then clamp the result between MIN_NUMERIC_LENGTH (for numeric) or 1 (for string) and MAX_TOKEN_LENGTH; update resolveTokenLength() to read the property with the appropriate default based on resolveTokenType(), and ensure constants DEFAULT_NUMERIC_LENGTH, MIN_NUMERIC_LENGTH, MAX_TOKEN_LENGTH and the method resolveTokenType() are used to locate and implement the change.src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java (1)
29-37: Add a boundary test for numeric length clamping.Line 31 and Line 36 validate the happy path for 8 digits, but this change also affects minimum-length behavior. Please add a test with
mifos.self.service.token.lengthset below 8 (for example,4) and assert that generated numeric tokens are still 8 digits.As per coding guidelines, "Verify both happy path and edge cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java` around lines 29 - 37, Add an edge-case test in SelfServiceAuthorizationTokenServiceTest to verify numeric token length clamping: in the new test (e.g., generateToken_respectsNumericConfiguration_minBelowMinClampsTo8) mock env.getProperty("mifos.self.service.token.type") to return "numeric" and env.getProperty("mifos.self.service.token.length", Integer.class, 8) to return a value below the minimum (e.g., 4), construct SelfServiceAuthorizationTokenService with that env, call service.generateToken(), and assert the returned token still matches "\\d{8}" (8 digits) to confirm the minimum-length behavior enforced by generateToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@postman/SELF` SERVICE PLUGIN.postman_collection.json:
- Around line 2326-2328: Several Authorization headers in the Postman collection
are still using hardcoded Basic credentials; locate every JSON object where
"key": "Authorization" and its "value" is a literal "Basic ..." string (examples
shown in the diff as the Authorization header entry) and replace the hardcoded
Base64 basic-auth values with a parameterized environment variable (e.g., use
"value": "{{selfservice_auth_token}}" or another aptly named variable), remove
any embedded plaintext credentials from the collection, and ensure corresponding
environment/secret variables are documented or added to Postman environments so
tests use the parameter instead of committing secrets.
- Line 44: The request body currently serializes the useTLS field as a string
("{{smtp_use_tls}}") which sends a string instead of a JSON boolean; update the
raw JSON payload for the request body so the useTLS property is unquoted
(useTLS: {{smtp_use_tls}}) to send a boolean, locating the change in the raw
body JSON where the "useTLS" key appears in the Postman collection entry.
In
`@src/main/java/org/apache/fineract/selfservice/notification/NotificationContext.java`:
- Around line 93-108: The Scope.close() currently always clears the
NotificationContext losing any outer eventType; modify bind(String eventType) to
capture the current thread-local value (e.g., call the existing getter before
calling set(eventType)) and store it in the returned Scope instance, then change
Scope to hold that previousValue and on close restore it (call
set(previousValue) if non-null or call clear() only when previousValue was null)
so nested bind(...) calls restore the outer context instead of discarding it;
update Scope constructor and close() accordingly while keeping method names
bind, set, clear and Scope intact.
In
`@src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java`:
- Around line 84-90: SELF_ENROLLMENT_DATA_PARAMETERS currently includes
activationDateParamName but normalizeSelfEnrollmentClientPayload() does not
preserve callers' activationDate (it is overwritten when active=true), so either
remove activationDateParamName from the allowlist or propagate it end-to-end;
fix by updating SelfServiceApiConstants.SELF_ENROLLMENT_DATA_PARAMETERS to drop
activationDateParamName if you choose not to support it yet, or alternatively
modify
SelfServiceRegistrationWritePlatformServiceImpl.normalizeSelfEnrollmentClientPayload()
to read and validate activationDateParamName from the incoming payload, copy it
into the normalized map/DTO, and avoid unconditionally overriding it when active
is true (also add appropriate validation/sanitization for the date format).
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 782-790: The helper isEmailMode(SelfServiceRegistration request)
must not fall back to env property
fineract.selfservice.notification.login.delivery-preference when both email and
mobile are present; instead read the persisted enrollment/activation channel
selected with the registration (e.g., a field like request.getEnrollmentMode()
or request.getActivationChannel()) and return true for "email" based on that
value; if that field is absent, fall back to the original single-contact logic
(hasEmail/hasMobile) but never use the login.delivery-preference. Update
isEmailMode to reference the registration's persisted choice and remove reliance
on env.getProperty for this decision.
- Around line 526-543: The afterCommit callback registered via
TransactionSynchronizationManager.registerSynchronization currently calls
applicationEventPublisher.publishEvent(...) (creating a
SelfServiceNotificationEvent) without guarding against runtime failures; modify
the anonymous TransactionSynchronization.afterCommit implementation to wrap the
publishEvent call in a try/catch that catches Throwable (or Exception), logs the
failure via the class logger with context (e.g. USER_ACTIVATED, appUser.getId(),
username) and does not rethrow so the API response remains successful after the
transaction commits; keep the SelfServiceNotificationEvent creation and publish
logic intact but ensure any exception from
applicationEventPublisher.publishEvent(...) is swallowed after logging.
In
`@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java`:
- Around line 163-164: In the BadCredentialsException catch block inside
SelfAuthenticationApiResource (catch (BadCredentialsException ex)), publish a
LOGIN_FAILURE event before rethrowing: resolve request.username to check for an
existing self-service user (to avoid user-enumeration) and only publish the
LOGIN_FAILURE when a matching self-service user is found, then rethrow the
BadCredentialsException as before; update the catch to perform the lookup (via
your existing self-user lookup helper/service) and call the same event-publish
path used for disabled/locked account failures.
---
Outside diff comments:
In
`@src/test/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResourceTest.java`:
- Around line 77-118: Tests do not assert interactions with
ApplicationEventPublisher, so add verifications: in
authenticate_returnsUserDataOnSuccess() verify the injected
ApplicationEventPublisher.publishEvent(...) was called once with a LoginEvent
(or the event type/enum representing LOGIN_SUCCESS) after calling
resource.authenticate(...); in
authenticate_throwsPasswordResetExceptionWhenResetRequired() assert that
publishEvent was not called (verify(applicationEventPublisher,
never()).publishEvent(any())) or that it was called with the specific
password-reset event if your design expects that; locate the tests using
resource.authenticate(...) and the injected ApplicationEventPublisher mock and
add the corresponding verify(...) or verify(..., never()) assertions to cover
both happy path and failure branch event emissions.
---
Duplicate comments:
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.java`:
- Around line 26-27: The test currently wires SelfServiceNotificationConfig but
registers a custom TestNotificationListener; replace that bespoke listener
registration in SelfServiceNotificationAsyncIntegrationTest.AsyncTestConfig with
the production listener/bean from SelfServiceNotificationConfig (i.e., ensure
the actual SelfServiceNotificationService and its `@Async` `@EventListener` bean are
loaded) and change assertions to verify the real observable side-effect produced
by SelfServiceNotificationService (instead of TestNotificationListener
behavior); update any bean definitions or `@Import` statements so the production
listener is used and assert its side-effect to prove the real wiring and async
execution.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenService.java`:
- Around line 77-83: In resolveTokenLength(), determine the token type once via
resolveTokenType() and use a type-specific default (e.g., use
DEFAULT_NUMERIC_LENGTH for "numeric" and a separate DEFAULT_STRING_LENGTH for
non-numeric) when reading env.getProperty, then clamp the result between
MIN_NUMERIC_LENGTH (for numeric) or 1 (for string) and MAX_TOKEN_LENGTH; update
resolveTokenLength() to read the property with the appropriate default based on
resolveTokenType(), and ensure constants DEFAULT_NUMERIC_LENGTH,
MIN_NUMERIC_LENGTH, MAX_TOKEN_LENGTH and the method resolveTokenType() are used
to locate and implement the change.
In
`@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java`:
- Around line 29-37: Add an edge-case test in
SelfServiceAuthorizationTokenServiceTest to verify numeric token length
clamping: in the new test (e.g.,
generateToken_respectsNumericConfiguration_minBelowMinClampsTo8) mock
env.getProperty("mifos.self.service.token.type") to return "numeric" and
env.getProperty("mifos.self.service.token.length", Integer.class, 8) to return a
value below the minimum (e.g., 4), construct
SelfServiceAuthorizationTokenService with that env, call
service.generateToken(), and assert the returned token still matches "\\d{8}" (8
digits) to confirm the minimum-length behavior enforced by generateToken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f7727e6-8407-472b-bd87-d25eed8e3da6
📒 Files selected for processing (36)
pom.xmlpostman/SELF SERVICE PLUGIN.postman_collection.jsonsrc/main/java/org/apache/fineract/selfservice/notification/NotificationContext.javasrc/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.javasrc/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationRepository.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenService.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceDisabledException.javasrc/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceLockedException.javasrc/main/java/org/apache/fineract/selfservice/security/service/SelfServiceUserDetailsChecker.javasrc/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.javasrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_es.propertiessrc/main/resources/notification/messages/NotificationMessages.propertiessrc/main/resources/notification/messages/NotificationMessages_es.propertiessrc/main/resources/notification/templates/html/login-failure.htmlsrc/main/resources/notification/templates/html/login-success.htmlsrc/main/resources/notification/templates/html/user-activated.htmlsrc/main/resources/notification/templates/text/login-failure.txtsrc/main/resources/notification/templates/text/login-success.txtsrc/main/resources/notification/templates/text/user-activated.txtsrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationAsyncIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResourceTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.java
✅ Files skipped from review due to trivial changes (13)
- src/main/resources/notification/templates/text/user-activated.txt
- src/main/resources/notification/templates/text/login-failure.txt
- src/main/resources/notification/templates/html/login-success.html
- src/main/resources/notification/templates/html/login-failure.html
- src/main/resources/notification/templates/html/user-activated.html
- src/main/resources/notification/templates/text/login-success.txt
- src/main/resources/i18n/messages_es.properties
- src/main/resources/notification/messages/NotificationMessages.properties
- src/main/resources/notification/messages/NotificationMessages_es.properties
- src/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java
- src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceLockedException.java
- src/main/resources/i18n/messages.properties
- src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (11)
- src/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.java
- src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java
- src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.java
- src/main/java/org/apache/fineract/selfservice/security/exception/SelfServiceDisabledException.java
- src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java
- src/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.java
- src/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.java
- pom.xml
- src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java
- src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java
- src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java
Comment on lines
+782
to
+790
| private boolean isEmailMode(SelfServiceRegistration request) { | ||
| boolean hasEmail = StringUtils.isNotBlank(request.getEmail()); | ||
| boolean hasMobile = StringUtils.isNotBlank(request.getMobileNumber()); | ||
|
|
||
| if (hasEmail && !hasMobile) return true; | ||
| if (hasMobile && !hasEmail) return false; | ||
|
|
||
| String pref = env.getProperty("fineract.selfservice.notification.login.delivery-preference", "email"); | ||
| return "email".equalsIgnoreCase(pref); |
There was a problem hiding this comment.
Do not guess the activation channel from the login preference.
When an enrollment request carries both email and mobile, this helper falls back to fineract.selfservice.notification.login.delivery-preference. That setting is unrelated to USER_ACTIVATED, so activation notices can go to a different channel than the one the user selected during enrollment. Persist the chosen enrollment mode and use that here instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`
around lines 782 - 790, The helper isEmailMode(SelfServiceRegistration request)
must not fall back to env property
fineract.selfservice.notification.login.delivery-preference when both email and
mobile are present; instead read the persisted enrollment/activation channel
selected with the registration (e.g., a field like request.getEnrollmentMode()
or request.getActivationChannel()) and return true for "email" based on that
value; if that field is absent, fall back to the original single-contact logic
(hasEmail/hasMobile) but never use the login.delivery-preference. Update
isEmailMode to reference the registration's persisted choice and remove reliance
on env.getProperty for this decision.
DeathGun44
force-pushed
the
MX-241-implement-notifications
branch
from
f31f28b to
4068181
Compare
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
2 participants
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.
implements: https://mifosforge.jira.com/browse/MX-241
Summary by CodeRabbit
New Features
Bug Fixes
Documentation