MX-172: Implement atomic self enrollment flow#133
Merged
IOhacker merged 1 commit intoopenMF:developfrom Apr 12, 2026
Merged
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 6 minutes and 51 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 (14)
📝 WalkthroughWalkthroughAdds one-shot self-service enrollment: new POST endpoint Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as SelfServiceRegistrationApiResource
participant Service as SelfServiceRegistrationWritePlatformServiceImpl
participant ClientService as ClientWritePlatformService
participant Repo as RepositoryLayer
participant AuthContext as SpringSecurityContext
Client->>API: POST /v1/self/registration/client-user
API->>Service: selfEnroll(apiRequestBodyAsJson)
Service->>Service: Validate allowed parameters (SELF_ENROLLMENT_DATA_PARAMETERS)
Service->>Service: Validate username/password/auth mode & duplicates
alt Duplicate detected
Service->>API: Throw SelfServiceEnrollmentConflictException
API->>Client: 409 Conflict (mapped JSON error)
else Proceed
Service->>AuthContext: Replace auth with audit user
Service->>ClientService: createClient(normalizedPayload)
ClientService->>Repo: persist client
Repo-->>ClientService: client persisted
Service->>Repo: persist AppSelfServiceUser
Repo-->>Service: user persisted
Service->>Repo: persist client-user mapping
Service->>AuthContext: Restore previous authentication
Service-->>API: return created user id
API-->>Client: 200 OK (resource id)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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: 7
🤖 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/registration/api/SelfServiceRegistrationApiResource.java`:
- Around line 56-58: The createSelfServiceUser endpoint currently calls
createSelfServiceUserOrEnroll, which falls back to enrollment; change it to use
the legacy token-confirmation-only flow so /user cannot create new clients:
replace the call to
selfServiceRegistrationWritePlatformService.createSelfServiceUserOrEnroll(...)
inside createSelfServiceUser(...) with the legacy token-confirmation method (the
one that only validates requestId and authenticationToken and fails when they
are absent) or, if no such method exists, add/invoke a method on
selfServiceRegistrationWritePlatformService that strictly enforces presence of
requestId/authenticationToken and throws an error when missing; keep the
enrollment path confined to the new /client-user route and do not call
selfEnroll from createSelfServiceUser.
In
`@src/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.java`:
- Around line 9-27: Add Javadoc to the public class
SelfServiceEnrollmentConflictException, its public constructor
SelfServiceEnrollmentConflictException(String userMessageGlobalisationCode,
String defaultMessage, String parameterName), and its public accessor methods
getParameterName() and getUserMessageGlobalisationCode(), explaining the purpose
of the exception, what the constructor parameters represent (especially
userMessageGlobalisationCode, defaultMessage and parameterName), and what each
accessor returns; keep descriptions concise and follow project Javadoc style
including `@param` and `@return` tags where appropriate.
In
`@src/main/java/org/apache/fineract/selfservice/registration/exceptionmapper/SelfServiceEnrollmentConflictExceptionMapper.java`:
- Around line 21-24: Add Javadoc to the public class
SelfServiceEnrollmentConflictExceptionMapper and its public method
toResponse(SelfServiceEnrollmentConflictException) describing their
responsibility: the class maps SelfServiceEnrollmentConflictException to an HTTP
Response, and the toResponse method builds and returns the appropriate Response
for that exception. Include standard Javadoc tags for the method (`@param`
exception description) and (`@return` Response describing status/body), and add a
brief `@since` tag on the class (or method) to satisfy the project's documentation
guidelines.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.java`:
- Line 26: The public API method createSelfServiceUserOrEnroll in
SelfServiceRegistrationWritePlatformService lacks Javadoc; add a Javadoc block
above the method declaration that documents its purpose, the branching behavior
implied by "OrEnroll" (what conditions cause user creation vs enrollment), the
parameter apiRequestBodyAsJson (expected JSON structure/fields), the returned
AppSelfServiceUser (what it contains), any thrown runtime or checked exceptions,
and any side-effects (persistence, notifications), so callers understand the
contract and error semantics.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 494-531: The code currently trusts caller-supplied privileged
fields (officeId, active, activationDate) from originalJson; instead always
derive these from server-side policy/config and never copy them from the
anonymous payload. Update the sanitization in
SelfServiceRegistrationWritePlatformServiceImpl so that: do not read officeId,
active, or activationDate from originalJson; set officeId using
env.getProperty("fineract.selfservice.enrollment.default-office-id", "1") and
add it to sanitizedJson; set active to a server-determined default (e.g., false)
and add that to sanitizedJson; only set activationDate from server-side logic
(e.g., today) when the server activates the account (do not accept
client-provided activationDate); keep other non-privileged fields (e.g.,
legalFormId, locale, dateFormat) as before. Ensure references: sanitizedJson,
originalJson, env, SelfServiceApiConstants, selfEnroll(...)/createClient(...)
reflect these changes.
- Around line 315-320: createSelfServiceUserOrEnroll currently calls
selfEnroll() directly so Spring proxy-based `@Transactional` on selfEnroll() is
bypassed, causing TransactionSynchronizationManager.registerSynchronization() to
run without an active transaction; to fix, ensure enrollment runs inside a
Spring-managed transaction by either (A) annotating
createSelfServiceUserOrEnroll with `@Transactional` so the outer entrypoint is
transactional, or (B) move the enrollment logic into a new `@Service` bean method
(e.g., EnrollmentService.selfEnroll) and call that bean (so proxy applies) from
createSelfServiceUserOrEnroll; update calls to
TransactionSynchronizationManager.registerSynchronization() accordingly and keep
references to createSelfServiceUserOrEnroll and selfEnroll to locate the
changes.
In
`@src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java`:
- Around line 131-163: The testConcurrentRaceCondition() currently only asserts
HTTP statuses; update it to verify persisted state by querying the system after
r1/r2 complete: use numericId()/generateDuplicateMobilePayload(...) to get the
shared identity, then call the appropriate lookup (e.g., repository/service/REST
endpoint used elsewhere in tests or the same executeSelfEnrollment helper) to
assert that exactly one client↔user mapping exists for that phone and that no
orphan client record exists for a failed enrollment; keep the existing status
assertions but add assertions that the database/user-mapping count for the
shared mobile equals 1 and that any client record without a linked user does not
exist (or count is 0) to prove atomic commit/rollback.
🪄 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: 8b8065ae-8637-49e8-953b-a365fdd575e6
📒 Files selected for processing (14)
pom.xmlsrc/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.javasrc/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentRequest.javasrc/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.javasrc/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.javasrc/main/java/org/apache/fineract/selfservice/registration/exceptionmapper/SelfServiceEnrollmentConflictExceptionMapper.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/starter/SelfServiceSecurityConfiguration.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfServicePermissionEnforcementIntegrationTest.java
...ava/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java
Show resolved
Hide resolved
...ache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.java
Show resolved
Hide resolved
...t/selfservice/registration/exceptionmapper/SelfServiceEnrollmentConflictExceptionMapper.java
Show resolved
Hide resolved
...e/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.java
Show resolved
Hide resolved
...neract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java
Show resolved
Hide resolved
...neract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java
Outdated
Show resolved
Hide resolved
...a/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java
Show resolved
Hide resolved
DeathGun44
force-pushed
the
MX-172-Implement-Self-Enrollment-Flow
branch
from
49496ab to
970fed5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java (1)
70-85: Consider adding timeout toexecuteSelfEnrollmentwhen latch is used.If
latch.await()blocks indefinitely due to test failure, the test will hang. TheCompletableFuture.orTimeout()at lines 160 and 163 mitigates this at the caller level, but adding a timeout directly toawait()would be more defensive.Add timeout to await
private Response executeSelfEnrollment(String payload, CountDownLatch latch) { try { if (latch != null) { - latch.await(); + latch.await(30, java.util.concurrent.TimeUnit.SECONDS); } } catch (InterruptedException e) { Thread.currentThread().interrupt(); }🤖 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/api/SelfServiceEnrollmentIntegrationTest.java` around lines 70 - 85, The executeSelfEnrollment method can block indefinitely on latch.await(); change it to use a timed wait (e.g., latch.await(timeout, TimeUnit.SECONDS)) and handle the boolean result to proceed or fail the test (log/throw an assertion) if the wait timed out; keep the InterruptedException handling (reset interrupt flag) and ensure the rest of executeSelfEnrollment (the POST via given(...).post(ENROLLMENT_PATH)) runs only after a successful await or fails fast on timeout. Reference: executeSelfEnrollment and the CountDownLatch.await call.src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java (2)
63-65: Lowercase name variants duplicate existing constants.
firstnameParamName,middlenameParamName, andlastnameParamNameduplicate the semantics offirstNameParamName,middleNameParamName, andlastNameParamName. If both casing variants are required to handle different request formats, document this explicitly. Otherwise, normalize to a single convention.🤖 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/SelfServiceApiConstants.java` around lines 63 - 65, The three lowercase constants firstnameParamName, middlenameParamName, lastnameParamName in SelfServiceApiConstants duplicate firstNameParamName, middleNameParamName, lastNameParamName; remove the redundant lowercase constants and update callers to use the canonical camelCase constants (firstNameParamName, middleNameParamName, lastNameParamName), or if both variants are intentionally required, keep only one canonical constant and add clear javadoc/comments in SelfServiceApiConstants explaining why the lowercase aliases exist and implement the aliases as final references to the canonical constants to avoid divergence (e.g., set firstnameParamName = firstNameParamName) while updating usages to prefer the canonical names.
54-55: Add a clarifying comment explaining the intentional use of bothexternalIdParamNameandexternalIDParamName.Both constants are intentionally used to support different API request formats. The
copyFirstPresentmethod demonstrates that these constants handle requests with either "externalId" or "externalID" casing. Add a comment above these constants to document this dual-format support.🤖 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/SelfServiceApiConstants.java` around lines 54 - 55, Add a short clarifying comment above the two constants externalIdParamName and externalIDParamName explaining they intentionally coexist to support both "externalId" and "externalID" request parameter casings (used by copyFirstPresent), e.g., note that copyFirstPresent checks both keys and why both forms must be preserved for backward/format compatibility.src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java (1)
21-21: Import organization: group imports by package.The newly added imports are scattered (line 21 among jakarta imports, lines 30-35 after Spring imports). Consider grouping all
io.swaggerimports together and alljakarta.ws.rsimports together for consistency.Suggested import order
import io.swagger.v3.oas.annotations.tags.Tag; +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.media.Content; +import io.swagger.v3.oas.annotations.media.Schema; +import io.swagger.v3.oas.annotations.parameters.RequestBody; +import io.swagger.v3.oas.annotations.responses.ApiResponse; +import io.swagger.v3.oas.annotations.responses.ApiResponses; +import jakarta.ws.rs.Consumes; import jakarta.ws.rs.POST; import jakarta.ws.rs.Path; import jakarta.ws.rs.Produces; -import jakarta.ws.rs.Consumes; import jakarta.ws.rs.core.MediaType; import lombok.RequiredArgsConstructor; ... -import io.swagger.v3.oas.annotations.Operation; -import io.swagger.v3.oas.annotations.responses.ApiResponse; -import io.swagger.v3.oas.annotations.responses.ApiResponses; -import io.swagger.v3.oas.annotations.media.Content; -import io.swagger.v3.oas.annotations.media.Schema; -import io.swagger.v3.oas.annotations.parameters.RequestBody;Also applies to: 30-35
🤖 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/api/SelfServiceRegistrationApiResource.java` at line 21, Reorder the import statements in SelfServiceRegistrationApiResource so that imports are grouped by package (e.g., all jakarta.ws.rs imports together, all io.swagger imports together, then Spring and other packages) instead of being interspersed; locate the class SelfServiceRegistrationApiResource and the import block around the current jakarta.ws.rs and io.swagger lines (including the imports currently at lines ~30-35) and move/merge them into cohesive groups following the project's import ordering convention.
🤖 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/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java`:
- Around line 87-100: The queryCount method currently injects the parameter into
SQL via sql.replace("?", "'" + parameter + "'"), which is vulnerable to quotes;
fix by sanitizing the parameter before injection (replace any single quote '
with two single quotes ''), e.g. compute a sanitizedParam =
parameter.replace("'", "''") and use sql.replace("?", "'" + sanitizedParam +
"'") inside queryCount, or better, switch to passing a psql variable (using psql
-v and :'var' in the SQL) to avoid direct concatenation; update the queryCount
method accordingly (preserve Container.ExecResult usage and the exception
handling).
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java`:
- Line 21: Reorder the import statements in SelfServiceRegistrationApiResource
so that imports are grouped by package (e.g., all jakarta.ws.rs imports
together, all io.swagger imports together, then Spring and other packages)
instead of being interspersed; locate the class
SelfServiceRegistrationApiResource and the import block around the current
jakarta.ws.rs and io.swagger lines (including the imports currently at lines
~30-35) and move/merge them into cohesive groups following the project's import
ordering convention.
In
`@src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java`:
- Around line 63-65: The three lowercase constants firstnameParamName,
middlenameParamName, lastnameParamName in SelfServiceApiConstants duplicate
firstNameParamName, middleNameParamName, lastNameParamName; remove the redundant
lowercase constants and update callers to use the canonical camelCase constants
(firstNameParamName, middleNameParamName, lastNameParamName), or if both
variants are intentionally required, keep only one canonical constant and add
clear javadoc/comments in SelfServiceApiConstants explaining why the lowercase
aliases exist and implement the aliases as final references to the canonical
constants to avoid divergence (e.g., set firstnameParamName =
firstNameParamName) while updating usages to prefer the canonical names.
- Around line 54-55: Add a short clarifying comment above the two constants
externalIdParamName and externalIDParamName explaining they intentionally
coexist to support both "externalId" and "externalID" request parameter casings
(used by copyFirstPresent), e.g., note that copyFirstPresent checks both keys
and why both forms must be preserved for backward/format compatibility.
In
`@src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java`:
- Around line 70-85: The executeSelfEnrollment method can block indefinitely on
latch.await(); change it to use a timed wait (e.g., latch.await(timeout,
TimeUnit.SECONDS)) and handle the boolean result to proceed or fail the test
(log/throw an assertion) if the wait timed out; keep the InterruptedException
handling (reset interrupt flag) and ensure the rest of executeSelfEnrollment
(the POST via given(...).post(ENROLLMENT_PATH)) runs only after a successful
await or fails fast on timeout. Reference: executeSelfEnrollment and the
CountDownLatch.await call.
🪄 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: c3e9b52b-c660-4545-b203-3b51efc59c3d
📒 Files selected for processing (14)
pom.xmlsrc/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.javasrc/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentRequest.javasrc/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.javasrc/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.javasrc/main/java/org/apache/fineract/selfservice/registration/exceptionmapper/SelfServiceEnrollmentConflictExceptionMapper.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/starter/SelfServiceSecurityConfiguration.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfServicePermissionEnforcementIntegrationTest.java
✅ Files skipped from review due to trivial changes (5)
- pom.xml
- src/test/java/org/apache/fineract/selfservice/security/api/SelfServicePermissionEnforcementIntegrationTest.java
- src/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.java
- src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentRequest.java
- src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java
- src/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.java
- src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.java
- src/main/java/org/apache/fineract/selfservice/registration/exceptionmapper/SelfServiceEnrollmentConflictExceptionMapper.java
- src/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.java
...a/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java
Show resolved
Hide resolved
DeathGun44
force-pushed
the
MX-172-Implement-Self-Enrollment-Flow
branch
from
970fed5 to
da80ba2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java (1)
55-60: Consider adding@Consumesannotation for consistency.The new
selfEnrollendpoint includes@Consumes({MediaType.APPLICATION_JSON}), but the existingcreateSelfServiceRegistrationRequestandcreateSelfServiceUserendpoints do not. Adding@Consumesto all POST endpoints improves API documentation consistency and explicitly declares the expected content type.Proposed fix
`@POST` + `@Consumes`({MediaType.APPLICATION_JSON}) `@Produces`({MediaType.APPLICATION_JSON}) public String createSelfServiceRegistrationRequest(final String apiRequestBodyAsJson) {🤖 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/api/SelfServiceRegistrationApiResource.java` around lines 55 - 60, Add the missing `@Consumes`({MediaType.APPLICATION_JSON}) annotation to the POST endpoints in SelfServiceRegistrationApiResource to match the selfEnroll endpoint and make API docs explicit; specifically annotate the methods createSelfServiceRegistrationRequest and createSelfServiceUser with `@Consumes`({MediaType.APPLICATION_JSON}) (and ensure the javax.ws.rs MediaType/Consumes import is present) so the expected request content type is declared consistently.src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java (1)
40-42: Potential collision innumericId()generation.The
numericId()method generates an 8-digit number from a UUID hash modulo 100,000,000. With only ~100 million possible values and the birthday paradox, collision probability increases significantly when running many tests. Consider usingSystem.nanoTime()combined with a counter or atomic integer for better uniqueness guarantees in test scenarios.🤖 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/api/SelfServiceEnrollmentIntegrationTest.java` around lines 40 - 42, The numericId() method produces 8-digit IDs via UUID.hashCode() % 100000000 which risks collisions; replace its implementation to combine a high-resolution timestamp (System.nanoTime()) with a monotonically increasing AtomicInteger (or counter) and format the result to 8 digits to ensure better uniqueness in tests. Update the numericId() function to build the ID from (nanoTime + counter) or by concatenating parts, then mask/format to eight numeric characters, ensuring thread-safety by using java.util.concurrent.atomic.AtomicInteger for the counter.src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java (4)
334-336: Add Javadoc for public methodselfEnroll.This public method lacks Javadoc documentation. As per coding guidelines, "Public methods and classes MUST have Javadoc."
Proposed fix
+ /** + * Creates a Fineract client and linked self-service user in a single atomic operation. + * + * `@param` apiRequestBodyAsJson enrollment request payload as raw JSON + * `@return` the created self-service user + * `@throws` SelfServiceEnrollmentConflictException if username, mobile, or email already exists + */ `@Transactional`(rollbackFor = Exception.class) `@Override` public AppSelfServiceUser selfEnroll(String apiRequestBodyAsJson) {🤖 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 334 - 336, Add a Javadoc comment to the public method selfEnroll in class SelfServiceRegistrationWritePlatformServiceImpl: document the purpose of the method, describe the parameter apiRequestBodyAsJson (expected format/content), state the return type AppSelfServiceUser and what it represents, and note transactional behavior and any exceptions that may be thrown; ensure the Javadoc follows project style (summary sentence, `@param`, `@return`, and `@throws` as appropriate) so the public API is properly documented.
314-322: Add Javadoc for public methodcreateSelfServiceUserOrEnroll.This public method lacks Javadoc documentation. As per coding guidelines, "Public methods and classes MUST have Javadoc."
Proposed fix
+ /** + * Routes to the appropriate user creation flow based on request payload. + * + * `@param` apiRequestBodyAsJson request payload as raw JSON + * `@return` the created self-service user + */ `@Transactional`(rollbackFor = Exception.class) `@Override` public AppSelfServiceUser createSelfServiceUserOrEnroll(String apiRequestBodyAsJson) {🤖 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 314 - 322, The public method createSelfServiceUserOrEnroll lacks Javadoc; add a concise Javadoc block above the method createSelfServiceUserOrEnroll describing its purpose (decides between creating a user or self-enrolling based on JSON input), the parameters (apiRequestBodyAsJson as the raw JSON request), the return value (AppSelfServiceUser), and any thrown/transactional behavior; mention that it delegates to createSelfServiceUser when requestIdParamName or authenticationTokenParamName are present in the JSON and to selfEnroll otherwise, and include `@Transactional` and `@Override` annotations in the doc as appropriate.
334-446: Consider extracting sub-operations to reduce method length.The
selfEnrollmethod spans over 100 lines. While the logic is correct and sequential, extracting distinct phases (validation, security context setup, client creation, user creation) into separate private methods would improve readability and testability.🤖 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 334 - 446, The selfEnroll method is too long; extract its distinct phases into private helper methods to improve readability and testability: move the input validation block (Gson parsing, fromApiJsonHelper extracts, baseDataValidator checks and the dataValidationErrors check, and validateForDuplicateUsernameForEnrollment) into a private validateSelfEnrollmentPayload(JsonElement) method; move the audit authentication setup (fetching auditUsername/auditUserId, saving previousAuth, setting SecurityContextHolder, and TransactionSynchronizationManager.registerSynchronization) into a private setupAuditAuthentication() method that returns the previous Authentication so it can be restored; and move the client creation and account/user creation logic (JsonCommand creation, clientWritePlatformService.createClient, finding client/role, building AppSelfServiceUser, calling userDomainService.create and saving mapping) into a private createClientAndSelfServiceUser(JsonElement, String username, String password, String email) method that returns the created AppSelfServiceUser; replace the inlined blocks in selfEnroll with calls to these new private methods (keep existing exception handling in selfEnroll).
387-391: Audit user lookup should provide a clearer error message.If the configured audit user (
fineract.selfservice.enrollment.audit-user) does not exist in the database,queryForObjectreturns null (causing NPE) orappUserRepository.findById(...).orElseThrow()throws a genericNoSuchElementException. A descriptive exception would aid troubleshooting deployment misconfigurations.Proposed fix
String auditUsername = env.getProperty("fineract.selfservice.enrollment.audit-user", "mifos"); - Long auditUserId = jdbcTemplate.queryForObject("SELECT id FROM m_appuser WHERE username = ?", Long.class, auditUsername); - final Authentication previousAuth = SecurityContextHolder.getContext().getAuthentication(); - AppUser auditUser = appUserRepository.findById(auditUserId).orElseThrow(); + Long auditUserId = jdbcTemplate.queryForObject("SELECT id FROM m_appuser WHERE username = ?", Long.class, auditUsername); + if (auditUserId == null) { + throw new IllegalStateException("Audit user '" + auditUsername + "' not found. Check fineract.selfservice.enrollment.audit-user configuration."); + } + final Authentication previousAuth = SecurityContextHolder.getContext().getAuthentication(); + AppUser auditUser = appUserRepository.findById(auditUserId) + .orElseThrow(() -> new IllegalStateException("Audit user with ID " + auditUserId + " not found in repository."));🤖 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 387 - 391, The audit-user lookup can produce a NullPointerException or a generic NoSuchElementException; replace the current blind jdbcTemplate.queryForObject and appUserRepository.findById(...) usage with explicit checks and descriptive errors: read the configured audit username via env.getProperty("fineract.selfservice.enrollment.audit-user", "mifos"), attempt to fetch the user id with jdbcTemplate.queryForObject but catch EmptyResultDataAccessException or check for null and throw a new IllegalStateException (or similar) that includes the configured username, then use appUserRepository.findById(auditUserId).orElseThrow(() -> new IllegalStateException(...)) with a clear message referencing the username and auditUserId so misconfiguration is obvious; update references to auditUserId, jdbcTemplate.queryForObject, and appUserRepository.findById accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java`:
- Around line 55-60: Add the missing `@Consumes`({MediaType.APPLICATION_JSON})
annotation to the POST endpoints in SelfServiceRegistrationApiResource to match
the selfEnroll endpoint and make API docs explicit; specifically annotate the
methods createSelfServiceRegistrationRequest and createSelfServiceUser with
`@Consumes`({MediaType.APPLICATION_JSON}) (and ensure the javax.ws.rs
MediaType/Consumes import is present) so the expected request content type is
declared consistently.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 334-336: Add a Javadoc comment to the public method selfEnroll in
class SelfServiceRegistrationWritePlatformServiceImpl: document the purpose of
the method, describe the parameter apiRequestBodyAsJson (expected
format/content), state the return type AppSelfServiceUser and what it
represents, and note transactional behavior and any exceptions that may be
thrown; ensure the Javadoc follows project style (summary sentence, `@param`,
`@return`, and `@throws` as appropriate) so the public API is properly documented.
- Around line 314-322: The public method createSelfServiceUserOrEnroll lacks
Javadoc; add a concise Javadoc block above the method
createSelfServiceUserOrEnroll describing its purpose (decides between creating a
user or self-enrolling based on JSON input), the parameters
(apiRequestBodyAsJson as the raw JSON request), the return value
(AppSelfServiceUser), and any thrown/transactional behavior; mention that it
delegates to createSelfServiceUser when requestIdParamName or
authenticationTokenParamName are present in the JSON and to selfEnroll
otherwise, and include `@Transactional` and `@Override` annotations in the doc as
appropriate.
- Around line 334-446: The selfEnroll method is too long; extract its distinct
phases into private helper methods to improve readability and testability: move
the input validation block (Gson parsing, fromApiJsonHelper extracts,
baseDataValidator checks and the dataValidationErrors check, and
validateForDuplicateUsernameForEnrollment) into a private
validateSelfEnrollmentPayload(JsonElement) method; move the audit authentication
setup (fetching auditUsername/auditUserId, saving previousAuth, setting
SecurityContextHolder, and
TransactionSynchronizationManager.registerSynchronization) into a private
setupAuditAuthentication() method that returns the previous Authentication so it
can be restored; and move the client creation and account/user creation logic
(JsonCommand creation, clientWritePlatformService.createClient, finding
client/role, building AppSelfServiceUser, calling userDomainService.create and
saving mapping) into a private createClientAndSelfServiceUser(JsonElement,
String username, String password, String email) method that returns the created
AppSelfServiceUser; replace the inlined blocks in selfEnroll with calls to these
new private methods (keep existing exception handling in selfEnroll).
- Around line 387-391: The audit-user lookup can produce a NullPointerException
or a generic NoSuchElementException; replace the current blind
jdbcTemplate.queryForObject and appUserRepository.findById(...) usage with
explicit checks and descriptive errors: read the configured audit username via
env.getProperty("fineract.selfservice.enrollment.audit-user", "mifos"), attempt
to fetch the user id with jdbcTemplate.queryForObject but catch
EmptyResultDataAccessException or check for null and throw a new
IllegalStateException (or similar) that includes the configured username, then
use appUserRepository.findById(auditUserId).orElseThrow(() -> new
IllegalStateException(...)) with a clear message referencing the username and
auditUserId so misconfiguration is obvious; update references to auditUserId,
jdbcTemplate.queryForObject, and appUserRepository.findById accordingly.
In
`@src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java`:
- Around line 40-42: The numericId() method produces 8-digit IDs via
UUID.hashCode() % 100000000 which risks collisions; replace its implementation
to combine a high-resolution timestamp (System.nanoTime()) with a monotonically
increasing AtomicInteger (or counter) and format the result to 8 digits to
ensure better uniqueness in tests. Update the numericId() function to build the
ID from (nanoTime + counter) or by concatenating parts, then mask/format to
eight numeric characters, ensuring thread-safety by using
java.util.concurrent.atomic.AtomicInteger for the counter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cabebe53-f13a-4b2d-8744-cfcc2b05860d
📒 Files selected for processing (14)
pom.xmlsrc/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.javasrc/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentRequest.javasrc/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.javasrc/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.javasrc/main/java/org/apache/fineract/selfservice/registration/exceptionmapper/SelfServiceEnrollmentConflictExceptionMapper.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/starter/SelfServiceSecurityConfiguration.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfServicePermissionEnforcementIntegrationTest.java
✅ Files skipped from review due to trivial changes (3)
- pom.xml
- src/test/java/org/apache/fineract/selfservice/security/api/SelfServicePermissionEnforcementIntegrationTest.java
- src/main/java/org/apache/fineract/selfservice/registration/exception/SelfServiceEnrollmentConflictException.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.java
- src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationIntegrationTest.java
- src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.java
- src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentRequest.java
- src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java
DeathGun44
force-pushed
the
MX-172-Implement-Self-Enrollment-Flow
branch
from
da80ba2 to
18b5cda
Compare
This was referenced Apr 12, 2026
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.
closes: https://mifosforge.jira.com/browse/MX-172
Summary by CodeRabbit
New Features
Security
Tests