Sandbox: Test new coderabbit and migrated code (draft pr)#117
Draft
ansh-varshney wants to merge 22 commits intoopenMF:developfrom
Draft
Sandbox: Test new coderabbit and migrated code (draft pr)#117ansh-varshney wants to merge 22 commits intoopenMF:developfrom
ansh-varshney wants to merge 22 commits intoopenMF:developfrom
Conversation
added 2 commits
April 9, 2026 11:11
- Switch profile from 'chill' to 'assertive' to align with request_changes_workflow enforcement - Ban @Autowired on fields explicitly; require constructor injection - Add explicit forbidden import list for domain layer (Spring, JPA, Hibernate, Jersey, Jackson, Swagger namespaces) - Define standard SS-XXXX error response schema in API layer rules - Add dedicated sections for handler (CQRS), Liquibase migrations, and split pom.xml / README.md into separate path rules - Consolidate cross-cutting rules (logging ban, secrets, magic numbers) into global section to eliminate duplication - Enforce @transactional(readOnly=true) on read-only service methods - Add finishing_touches: true for auto Javadoc/test suggestions
|
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 5 minutes and 18 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 (2)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors domain entities to be persistence-agnostic, adds JPA entity and repository adapters, updates services/configuration to use adapters and transactional semantics, removes password from registration payloads and database column, enhances repository and test code, and introduces comprehensive repository-level CodeRabbit review configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as SelfServiceWriteService
participant Domain as DomainObject
participant RepoAdapter as RepositoryAdapter
participant JpaRepo as JPARepository
participant DB as Database
Service->>Domain: create or update domain object
Service->>RepoAdapter: save/saveAndFlush(domain)
RepoAdapter->>RepoAdapter: if domain.id == null -> fromNewDomain(domain) else -> findById(id)
alt new entity
RepoAdapter->>JpaRepo: save(entity)
JpaRepo->>DB: INSERT
JpaRepo-->>RepoAdapter: persisted entity (id assigned)
else update existing
RepoAdapter->>JpaRepo: findById(id)
JpaRepo->>DB: SELECT
JpaRepo-->>RepoAdapter: entity
RepoAdapter->>RepoAdapter: applyChangesFrom(domain)
RepoAdapter->>JpaRepo: save(entity)
JpaRepo->>DB: UPDATE
end
RepoAdapter-->>Service: domain = entity.toDomain()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java (1)
143-155:⚠️ Potential issue | 🟠 MajorKeep API field names out of the domain entity.
update()still returns keys fromSelfBeneficiariesTPTApiConstants, so this class remains coupled to theapipackage after the refactor. Move that change-set formatting to the application/API layer, or define domain-owned field identifiers here.As per coding guidelines, "Domain must not import from api, service, handler, or starter packages."
🤖 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/account/domain/SelfBeneficiariesTPT.java` around lines 143 - 155, The update(...) method currently returns API-specific keys (e.g., NAME_PARAM_NAME, TRANSFER_LIMIT_PARAM_NAME) which couples the domain to the api package; change update(String newName, Long newTransferLimit) to return domain-owned identifiers instead (for example define private static final String NAME_FIELD = "name" and TRANSFER_LIMIT_FIELD = "transferLimit" in SelfBeneficiariesTPT or return a Set<String> of changed domain field names), update references to NAME_PARAM_NAME and TRANSFER_LIMIT_PARAM_NAME inside update(...) to use those domain constants (or the Set approach) and move any API-level key formatting/translation out of this class into the application/API layer that constructs responses. Ensure no imports from api packages remain in SelfBeneficiariesTPT.
🧹 Nitpick comments (4)
src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationRepository.java (2)
32-38: Avoid nullable returns in this public repository API.Returning
nullhere pushes sentinel handling to every caller and makes NPEs easier to miss.Optional<SelfServiceRegistration>is a better fit for the new contract.Proposed API shape
- SelfServiceRegistration getRequestByIdAndAuthenticationToken( - Long id, String authenticationToken); + Optional<SelfServiceRegistration> getRequestByIdAndAuthenticationToken( + Long id, String authenticationToken);As per coding guidelines, "Prefer returning Optional or sealed result types instead of null."
🤖 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/SelfServiceRegistrationRepository.java` around lines 32 - 38, Change the getRequestByIdAndAuthenticationToken contract to return Optional instead of possibly-null SelfServiceRegistration: update the repository interface method signature for getRequestByIdAndAuthenticationToken(Long id, String authenticationToken) to return java.util.Optional<SelfServiceRegistration>, and adjust all implementing classes and callers to wrap/unwrap using Optional (e.g., Optional.ofNullable where applicable and orElse/orElseThrow at call sites). Ensure imports are added for java.util.Optional and that any null-returning implementations now return Optional.empty() instead of null.
25-30: Keep flush semantics out of the domain contract.
saveAndFlush(...)still exposes a persistence-specific concern from a...domain...interface. Prefer a domain-levelsave(...)contract and let the infrastructure adapter decide whether an explicit flush is needed.Proposed API shape
- SelfServiceRegistration saveAndFlush(SelfServiceRegistration entity); + SelfServiceRegistration save(SelfServiceRegistration entity);As per coding guidelines, "Avoid framework coupling in domain or application layers."
🤖 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/SelfServiceRegistrationRepository.java` around lines 25 - 30, The domain interface SelfServiceRegistrationRepository currently exposes persistence detail via saveAndFlush(...) — change the domain contract to a framework-agnostic save(...) method (i.e., replace saveAndFlush with save in SelfServiceRegistrationRepository) and update the infrastructure adapter/implementation class to perform any required flush there (call saveAndFlush on the JPA repository or invoke entityManager.flush() inside the adapter). Also update all references/usages of SelfServiceRegistrationRepository.saveAndFlush(...) to use the new save(...) method and ensure tests and transactional boundaries remain correct in the adapter.src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.java (1)
45-50: Do not returnnullfrom the repository contract.This reintroduces nullable behavior into the public boundary you just decoupled from Spring Data. Prefer
Optional<SelfServiceRegistration>or throw a domain-specific not-found exception.As per coding guidelines, "Never return
nullfrom a public method. UseOptional<T>or throw a domain exception."🤖 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/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.java` around lines 45 - 50, The method getRequestByIdAndAuthenticationToken in SelfServiceRegistrationRepositoryAdapter must not return null; change its contract to return Optional<SelfServiceRegistration> (or throw a domain-specific NotFound exception) and update the implementation to wrap the JPA result with Optional.ofNullable(entity). Specifically, update the method signature in SelfServiceRegistrationRepositoryAdapter (and the corresponding repository interface) to return Optional<SelfServiceRegistration>, call jpaRepository.findByIdAndAuthenticationToken(...), convert the returned SelfServiceRegistrationJpaEntity to domain via toDomain() only when present, and return Optional.ofNullable(...) so callers receive an Optional instead of a nullable value.src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPTRepository.java (1)
34-39: Removeflushsemantics from the domain repository.
saveAndFlushexposes persistence timing concerns at the domain boundary. Keep the domain contract tosave(...)and let the service transaction decide when flushing happens.As per coding guidelines, "Avoid framework coupling in domain or application layers."
🤖 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/account/domain/SelfBeneficiariesTPTRepository.java` around lines 34 - 39, Remove the saveAndFlush persistence method from the SelfBeneficiariesTPTRepository domain interface and replace it with a plain save(SelfBeneficiariesTPT entity) signature (or use the existing save if already present); update any callers to call save(...) instead and rely on the service-layer transaction to control flushing rather than exposing flush semantics at the repository boundary (look for usages of SelfBeneficiariesTPTRepository.saveAndFlush and change them to save and let transactional services handle commit/flush).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 290-323: The forbidden-imports rule is currently applied to the
infrastructure path and must be moved to target the domain layer; update the
path pattern from 'src/main/java/**/infrastructure/**/*.java' to
'src/main/java/**/domain/**/*.java' (or equivalent project domain glob) in the
.coderabbit.yaml block containing the forbidden import prefixes
(`org.springframework.`, `jakarta.persistence.`, `javax.persistence.`,
`org.hibernate.`, `jakarta.ws.rs.`, `javax.ws.rs.`, `jakarta.servlet.`,
`com.fasterxml.jackson.`, `io.swagger.`) so the ban applies to domain sources
rather than infrastructure code that legitimately uses JPA/Jackson/Spring.
- Around line 212-257: The controller-focused rule block (validation,
`@Valid/`@NotNull/etc., `@Operation/`@ApiResponse, SS- error schema and
ControllerAdvice enforcement) is attached to the application-targeted rule set;
move these checks into the controller-targeted rule set so they apply only to
controller sources, and remove or relax them from the application rules. Update
the YAML so the controller rules reference the controller file-matching pattern
and ensure the ControllerAdvice/SS- schema enforcement, `@Operation/`@ApiResponse
requirements, and request-body validation checks are only present in that
controller-specific block.
- Line 15: The reviews.finishing_touches entry is currently a boolean but the
schema expects a mapping; update the .coderabbit.yaml so that
reviews.finishing_touches is an object (mapping) rather than true/false — e.g.,
replace the scalar `finishing_touches: true` with a mapping `finishing_touches:`
containing the appropriate subkeys (or an empty mapping) so that the
reviews.finishing_touches structure conforms to the schema.
In @.vscode/settings.json:
- Line 2: The workspace setting "java.compile.nullAnalysis.mode" is disabling
IDE null-analysis for all contributors; update the setting by removing it or
changing its value from "disabled" to "automatic" so VS Code provides
null-safety diagnostics—edit the entry for java.compile.nullAnalysis.mode in the
.vscode settings to apply the change.
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 43-49: Remove the protected no-arg constructor
SelfBeneficiariesTPT() that initializes all identifier fields to null; delete
that constructor and ensure only constructors that require and set the mandatory
fields (appUserId, officeId, clientId, accountId, accountType) remain so
instances cannot be created in an invalid state, and update any callers or tests
that relied on the no-arg constructor to use the constructor that supplies
required identifiers (also review the update() method to assume non-null
identifiers).
In
`@src/main/java/org/apache/fineract/selfservice/account/infrastructure/persistence/SelfBeneficiariesTPTRepositoryAdapter.java`:
- Around line 60-67: Replace the generic IllegalStateException in
SelfBeneficiariesTPTRepositoryAdapter where you call
jpaRepository.findById(domain.getId()).orElseThrow(...) with the module's
domain-specific not-found exception (e.g., SelfBeneficiariesNotFoundException or
the repository/module's established NotFound exception), keeping the informative
message that includes domain.getId(); update imports and the thrown exception
type in the lambda so the existing error-mapping/handler recognizes and
translates stale-update cases correctly.
In
`@src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java`:
- Around line 84-113: The reconstruction constructor
SelfServiceRegistration(Long id, Long clientId, String accountNumber, String
firstName, String middleName, String lastName, String mobileNumber, String
email, String authenticationToken, String username, String password,
LocalDateTime createdDate) is public but intended only for infrastructure
hydration; change its visibility to package-private (remove the public modifier)
so it is not part of the public API and only repository/adapter code in the same
package can call it, leaving the signature and Javadoc intact.
- Around line 58-82: The SelfServiceRegistration constructors currently accept
null clientId and createdDate which allows invalid instances; update the public
constructors (the one taking clientId, accountNumber, ... createdDate and the
other persisted/new factory constructor) to validate and reject null clientId
and null createdDate at construction time (throw an appropriate runtime
exception such as IllegalArgumentException or NullPointerException with a clear
message), ensuring the class invariants are enforced in the
SelfServiceRegistration constructor(s) rather than relying on callers to catch
the error later.
In
`@src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationJpaEntity.java`:
- Around line 66-67: The entity SelfServiceRegistrationJpaEntity currently
persists the raw field "password" (mapped to request_audit_table); stop
persisting plaintext credentials by removing the persistent mapping for the
"password" field and any code paths that rehydrate it into the domain model, or
replace persistence with a non-reversible safe representation (e.g., store only
a secure hash or a boolean/metadata flag) and ensure any references in
constructors or mappers (the JPA field "password" and the code that reads/writes
it between SelfServiceRegistrationJpaEntity and the domain model) are updated to
use the hashed/tokenized value or removed so raw passwords are never written to
or read from the DB.
In
`@src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.java`:
- Around line 37-43: The current saveAndFlush implementation always creates a
new JPA entity via SelfServiceRegistrationJpaEntity.fromNewDomain(domain,
client), discarding any existing domain.getId() and causing inserts instead of
updates; modify saveAndFlush to preserve the domain id when updating by mapping
to an entity that retains the existing id (e.g., use or add a
fromDomain/fromExistingDomain factory that accepts the domain id or set the id
on the created SelfServiceRegistrationJpaEntity before calling
jpaRepository.saveAndFlush), then call jpaRepository.saveAndFlush(...) and
return .toDomain() so updates update the existing row rather than inserting a
new one.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 136-148: createRegistrationRequest currently persists
SelfServiceRegistration and immediately calls sendAuthorizationToken inside the
same `@Transactional` method, causing notifications to be sent even if the
transaction rolls back; change this by deferring the side-effect until after
commit: after saving SelfServiceRegistration in createRegistrationRequest,
either publish a domain event containing the registration id/flags and handle it
in a separate `@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT)
handler that calls sendAuthorizationToken, or register a
TransactionSynchronization
(TransactionSynchronizationManager.registerSynchronization) that invokes
sendAuthorizationToken in afterCommit; ensure the handler receives
SelfServiceRegistration (or its id and isEmailAuth) and calls the existing
sendAuthorizationToken(selfServiceRegistration, client, isEmailAuth) so
notifications are only sent after successful commit.
- Around line 279-282: The persistence/authentication catch passes
ExceptionUtils.getRootCause(dve.getCause()) which can be null; update the catch
in SelfServiceRegistrationWritePlatformServiceImpl so it computes a non-null
throwable (e.g., Throwable root = ExceptionUtils.getRootCause(dve.getCause());
if (root == null) root = dve.getCause() != null ? dve.getCause() : dve;) and
pass that to handleDataIntegrityIssues, and also modify
handleDataIntegrityIssues to null-guard realCause before calling
realCause.getMessage() (fallback to the passed exception's message or a default
string) so no NPE can occur when extracting the message.
In
`@src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.java`:
- Around line 17-35: Update the TODO comment in AppSelfServiceUser (the
top-of-file migration note) to reference the actual config filename used in this
PR: replace `.coderabbit.yaml` with `.coderabbit.yml` so the guidance points to
the correct CodeRabbit config file.
In
`@src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUserClientMapping.java`:
- Line 19: Update the configuration filename reference from ".coderabbit.yaml"
to ".coderabbit.yml" in the comment inside the AppSelfServiceUserClientMapping
class so it matches the repository's actual config file; locate the offending
comment in the class AppSelfServiceUserClientMapping and replace
".coderabbit.yaml" with ".coderabbit.yml".
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 143-155: The update(...) method currently returns API-specific
keys (e.g., NAME_PARAM_NAME, TRANSFER_LIMIT_PARAM_NAME) which couples the domain
to the api package; change update(String newName, Long newTransferLimit) to
return domain-owned identifiers instead (for example define private static final
String NAME_FIELD = "name" and TRANSFER_LIMIT_FIELD = "transferLimit" in
SelfBeneficiariesTPT or return a Set<String> of changed domain field names),
update references to NAME_PARAM_NAME and TRANSFER_LIMIT_PARAM_NAME inside
update(...) to use those domain constants (or the Set approach) and move any
API-level key formatting/translation out of this class into the application/API
layer that constructs responses. Ensure no imports from api packages remain in
SelfBeneficiariesTPT.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPTRepository.java`:
- Around line 34-39: Remove the saveAndFlush persistence method from the
SelfBeneficiariesTPTRepository domain interface and replace it with a plain
save(SelfBeneficiariesTPT entity) signature (or use the existing save if already
present); update any callers to call save(...) instead and rely on the
service-layer transaction to control flushing rather than exposing flush
semantics at the repository boundary (look for usages of
SelfBeneficiariesTPTRepository.saveAndFlush and change them to save and let
transactional services handle commit/flush).
In
`@src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationRepository.java`:
- Around line 32-38: Change the getRequestByIdAndAuthenticationToken contract to
return Optional instead of possibly-null SelfServiceRegistration: update the
repository interface method signature for
getRequestByIdAndAuthenticationToken(Long id, String authenticationToken) to
return java.util.Optional<SelfServiceRegistration>, and adjust all implementing
classes and callers to wrap/unwrap using Optional (e.g., Optional.ofNullable
where applicable and orElse/orElseThrow at call sites). Ensure imports are added
for java.util.Optional and that any null-returning implementations now return
Optional.empty() instead of null.
- Around line 25-30: The domain interface SelfServiceRegistrationRepository
currently exposes persistence detail via saveAndFlush(...) — change the domain
contract to a framework-agnostic save(...) method (i.e., replace saveAndFlush
with save in SelfServiceRegistrationRepository) and update the infrastructure
adapter/implementation class to perform any required flush there (call
saveAndFlush on the JPA repository or invoke entityManager.flush() inside the
adapter). Also update all references/usages of
SelfServiceRegistrationRepository.saveAndFlush(...) to use the new save(...)
method and ensure tests and transactional boundaries remain correct in the
adapter.
In
`@src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.java`:
- Around line 45-50: The method getRequestByIdAndAuthenticationToken in
SelfServiceRegistrationRepositoryAdapter must not return null; change its
contract to return Optional<SelfServiceRegistration> (or throw a domain-specific
NotFound exception) and update the implementation to wrap the JPA result with
Optional.ofNullable(entity). Specifically, update the method signature in
SelfServiceRegistrationRepositoryAdapter (and the corresponding repository
interface) to return Optional<SelfServiceRegistration>, call
jpaRepository.findByIdAndAuthenticationToken(...), convert the returned
SelfServiceRegistrationJpaEntity to domain via toDomain() only when present, and
return Optional.ofNullable(...) so callers receive an Optional instead of a
nullable value.
🪄 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: b3c1a424-de1e-4aee-9991-78ae5f5535f0
📒 Files selected for processing (21)
.coderabbit.yaml.vscode/settings.jsonsrc/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.javasrc/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPTRepository.javasrc/main/java/org/apache/fineract/selfservice/account/infrastructure/persistence/SelfBeneficiariesTPTJpaEntity.javasrc/main/java/org/apache/fineract/selfservice/account/infrastructure/persistence/SelfBeneficiariesTPTJpaRepository.javasrc/main/java/org/apache/fineract/selfservice/account/infrastructure/persistence/SelfBeneficiariesTPTRepositoryAdapter.javasrc/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/account/starter/SelfAccountConfiguration.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/infrastructure/persistence/SelfServiceRegistrationJpaEntity.javasrc/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationJpaRepository.javasrc/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationReadPlatformServiceImpl.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/savings/data/SelfSavingsDataValidator.javasrc/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUserClientMapping.java
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
Outdated
Show resolved
Hide resolved
...ervice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.java
Show resolved
Hide resolved
...neract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java
Show resolved
Hide resolved
...neract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java
Show resolved
Hide resolved
src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.java
Show resolved
Hide resolved
...g/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUserClientMapping.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationTest.java (1)
25-37: Assert exactcreatedDatemapping instead of only non-null.At Line 25 and Line 54,
LocalDateTime.now()is passed inline, but only non-null is verified (Line 37). This can miss regressions where the factory ignores the provided timestamp. Capture a fixed value and assert equality.Proposed test refinement
void instance_shouldCreateRegistrationWithAllFields() { + java.time.LocalDateTime createdDate = java.time.LocalDateTime.of(2026, 1, 1, 10, 0); SelfServiceRegistration reg = SelfServiceRegistration.instance( 1L, "000000001", "Pedro", "Marmol", "Perez", "5522649498", "pedro@test.com", "1234", "pedro.marmol", "SecurePass123#", - java.time.LocalDateTime.now()); + createdDate); assertEquals(1L, reg.getClientId()); @@ - assertNotNull(reg.getCreatedDate()); + assertEquals(createdDate, reg.getCreatedDate()); } `@Test` void instance_shouldHandleNullMiddleName() { + java.time.LocalDateTime createdDate = java.time.LocalDateTime.of(2026, 1, 1, 10, 0); SelfServiceRegistration reg = SelfServiceRegistration.instance( 1L, "000000002", "John", null, "Doe", null, "john@test.com", "5678", "john.doe", "Pass456#", - java.time.LocalDateTime.now()); + createdDate);Also applies to: 54-55
🤖 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/domain/SelfServiceRegistrationTest.java` around lines 25 - 37, Test currently passes LocalDateTime.now() inline and only asserts reg.getCreatedDate() is not null; change to capture a fixed LocalDateTime value (e.g., LocalDateTime.of(...)) into a variable, pass that variable into the factory/constructor used in SelfServiceRegistrationTest, and replace assertNotNull(reg.getCreatedDate()) with assertEquals(fixedDate, reg.getCreatedDate()); apply the same change to the other occurrence around lines 54-55 so both tests assert exact createdDate equality.
🤖 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/test/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationTest.java`:
- Around line 25-37: Test currently passes LocalDateTime.now() inline and only
asserts reg.getCreatedDate() is not null; change to capture a fixed
LocalDateTime value (e.g., LocalDateTime.of(...)) into a variable, pass that
variable into the factory/constructor used in SelfServiceRegistrationTest, and
replace assertNotNull(reg.getCreatedDate()) with assertEquals(fixedDate,
reg.getCreatedDate()); apply the same change to the other occurrence around
lines 54-55 so both tests assert exact createdDate equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40021276-0992-45e9-9f84-207cca236db5
📒 Files selected for processing (2)
src/test/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java
added 7 commits
April 10, 2026 12:44
Implements strictly secure password capture by omitting password strings from the registration step to prevent RequestAuditTable database caching. Password regex validations and logic shifted definitively to Step 2 user credential creation workflow. Creates liquibase update payload for request_audit_table schema update.
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java (1)
41-58:⚠️ Potential issue | 🟠 MajorEnforce domain invariants in both constructors.
Removing the no-arg constructor helped, but these constructors still accept
nullfor required identifiers andname, so callers can create an invalid domain object. That also leavesupdate()vulnerable to an NPE whennamewas hydrated or constructed asnull. Guard the required fields here and reject invalid values up front.Proposed fix
+import java.util.Objects; + public SelfBeneficiariesTPT( Long appUserId, String name, Long officeId, Long clientId, Long accountId, Integer accountType, Long transferLimit) { - this.appUserId = appUserId; - this.name = name; - this.officeId = officeId; - this.clientId = clientId; - this.accountId = accountId; - this.accountType = accountType; + this.appUserId = Objects.requireNonNull(appUserId, "appUserId must not be null"); + this.name = Objects.requireNonNull(name, "name must not be null"); + this.officeId = Objects.requireNonNull(officeId, "officeId must not be null"); + this.clientId = Objects.requireNonNull(clientId, "clientId must not be null"); + this.accountId = Objects.requireNonNull(accountId, "accountId must not be null"); + this.accountType = Objects.requireNonNull(accountType, "accountType must not be null"); + if (transferLimit != null && transferLimit < 0) { + throw new IllegalArgumentException("transferLimit must not be negative"); + } this.transferLimit = transferLimit; this.isActive = true; } @@ public SelfBeneficiariesTPT( Long id, Long appUserId, String name, Long officeId, Long clientId, Long accountId, Integer accountType, Long transferLimit, boolean isActive) { this.id = id; - this.appUserId = appUserId; - this.name = name; - this.officeId = officeId; - this.clientId = clientId; - this.accountId = accountId; - this.accountType = accountType; + this.appUserId = Objects.requireNonNull(appUserId, "appUserId must not be null"); + this.name = Objects.requireNonNull(name, "name must not be null"); + this.officeId = Objects.requireNonNull(officeId, "officeId must not be null"); + this.clientId = Objects.requireNonNull(clientId, "clientId must not be null"); + this.accountId = Objects.requireNonNull(accountId, "accountId must not be null"); + this.accountType = Objects.requireNonNull(accountType, "accountType must not be null"); + if (transferLimit != null && transferLimit < 0) { + throw new IllegalArgumentException("transferLimit must not be negative"); + } this.transferLimit = transferLimit; this.isActive = isActive; }As per coding guidelines, "Validate and sanitize all incoming data (use Bean Validation with
@Validand constraints)." and "Entities should encapsulate behavior and invariants."Also applies to: 64-83
🤖 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/account/domain/SelfBeneficiariesTPT.java` around lines 41 - 58, The constructors in SelfBeneficiariesTPT must enforce domain invariants by rejecting null/blank required fields; update both constructors (the Long-arg constructor shown and the other constructor around lines 64-83) to validate incoming parameters using Objects.requireNonNull for required identifiers (appUserId, officeId, clientId, accountId, accountType) and throw IllegalArgumentException if name is null or blank (trim and check length), and update the update(...) method to defensively check/requireNonNull the name before assigning to avoid NPEs; include clear exception messages naming the offending parameter.src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java (1)
283-285:⚠️ Potential issue | 🟠 MajorDo not drop the direct cause when extracting the root cause.
ExceptionUtils.getRootCause(dve.getCause())still loses single-level causes. When that happens,handleDataIntegrityIssues(...)evaluates the wrapper exception instead of the actual persistence/authentication cause, so duplicate-username errors can be misclassified as generic integrity failures.Proposed change
} catch (final PersistenceException | AuthenticationServiceException dve) { - throw handleDataIntegrityIssues(command, ExceptionUtils.getRootCause(dve.getCause()), dve, username); + final Throwable rootCause = ExceptionUtils.getRootCause(dve); + final Throwable effectiveCause = + rootCause != null ? rootCause : (dve.getCause() != null ? dve.getCause() : dve); + throw handleDataIntegrityIssues(command, effectiveCause, dve, username); }For Apache Commons Lang 3.x, what does ExceptionUtils.getRootCause(Throwable) return when the supplied throwable has no nested cause, and would calling ExceptionUtils.getRootCause(dve.getCause()) drop a direct cause with no inner cause?🤖 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 283 - 285, The catch block in SelfServiceRegistrationWritePlatformServiceImpl currently calls ExceptionUtils.getRootCause(dve.getCause()), which can drop a direct cause; change it to derive the actual cause by calling ExceptionUtils.getRootCause(dve) and if that returns null fall back to dve (or dve.getCause() as appropriate) before passing it into handleDataIntegrityIssues(command, ..., dve, username) so the real PersistenceException/AuthenticationServiceException cause (used to detect duplicate-username) is preserved.
🧹 Nitpick comments (2)
.coderabbit.yml (1)
404-420: Keep the changelog block focused on Liquibase rules.The opening half of this block is about build files, README, and application config, even though the path only matches Liquibase changelogs. Splitting that content into the existing build/README/config paths will make migration reviews much less noisy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yml around lines 404 - 420, The changelog rule block currently mixes general project standards with Liquibase-specific rules; extract the unrelated items and keep this block focused on Liquibase. Edit the block that starts with path: 'src/main/resources/db/changelog/**' and remove the build/README/application.yml entries (the sections about pom.xml/build.gradle, README.md, and application.yml), leaving only Liquibase-related guidance (e.g., changelog formatting, id conventions, labels, rollback requirements, and testing/migration verification). Create or append the removed items into their appropriate existing blocks for build files, README, and application config to avoid duplication and noisy migration reviews.src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationJpaEntity.java (1)
103-145: Reduce the visibility of the mapper helpers.
fromNewDomain(...),toDomain(), andupdateFromDomain(...)look adapter-internal, butpublicmakes them part of the infrastructure API unnecessarily. Package-private is enough here.Proposed change
- public static SelfServiceRegistrationJpaEntity fromNewDomain( + static SelfServiceRegistrationJpaEntity fromNewDomain( SelfServiceRegistration domain, Client client) { @@ - public SelfServiceRegistration toDomain() { + SelfServiceRegistration toDomain() { @@ - public void updateFromDomain(SelfServiceRegistration domain, Client client) { + void updateFromDomain(SelfServiceRegistration domain, Client client) {As per coding guidelines, "Limit public methods in classes; prefer package-private visibility for internal logic."
🤖 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/infrastructure/persistence/SelfServiceRegistrationJpaEntity.java` around lines 103 - 145, Make the three mapper/helper methods in SelfServiceRegistrationJpaEntity package-private rather than public: change the visibility of fromNewDomain(SelfServiceRegistration, Client), toDomain(), and updateFromDomain(SelfServiceRegistration, Client) to have no access modifier (remove "public") so they remain accessible inside the package but are not exposed on the infrastructure API; keep signatures and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yml:
- Around line 73-90: Remove the generic placeholder package layout block that
defines "com.company.project" and its tree; instead delete or replace that
subsection with the existing Fineract-specific module layout so reviews only
validate against the real package structure (refer to the placeholder symbol
"com.company.project" and the valid package "org.apache.fineract.selfservice" in
the file). Ensure the instructions under the path pattern no longer assert the
fictional tree and, if needed, merge relevant guidance into the Fineract module
guidance so there is no conflicting package layout text.
- Around line 27-72: The global '**' rules block contains Java/Spring-specific
checks (e.g., references to `@Valid`, Spring Security, HikariCP,
`@Cacheable/`@CacheEvict, `@Async`, and `@Slf4j`) and must be restricted to
repository-wide, language-agnostic standards only; remove or relocate
framework-specific lines from the '**' block into a Java-specific path section
(e.g., path: '**/*.java' or 'path: src/main/java/**') and keep only neutral
rules like "no hard-coded secrets", "no System.out/println", and general hygiene
items in the global block so non-Java files do not inherit Spring/Java-specific
guidance.
- Around line 325-340: The policy block mixes distinct rules for the
infrastructure layer and the command handler layer causing contradictory
guidance; split this single rule set into two separate entries: one for the
infrastructure path (referencing JpaRepository, JPA entities, adapters,
`@Repository/`@Component/@Configuration, mappers, and transaction management at
service level) and a second for the handler path (referencing
src/main/java/**/handler/**/*.java, one-command-per-class handlers, no direct
repository access, thin handlers that delegate to domain/application services,
annotate handlers with `@Component` and follow CommandProcessingResult
conventions); ensure each block only contains the rules relevant to that layer
so handlers aren’t given infrastructure instructions.
- Around line 280-288: The YAML rule for path
'src/main/java/**/infrastructure/**/*.java' currently contains domain-layer
guidance (entities, value objects, domain events) which incorrectly flags
infrastructure artifacts; update that rule so it contains
infrastructure-specific guidance (e.g., allow Spring components, JPA entities,
repositories, adapters, configuration classes, and integration code) or move the
existing domain guidance into the domain path rule (e.g.,
'src/main/java/**/domain/**/*.java'); ensure the
'src/main/java/**/infrastructure/**/*.java' block explicitly permits framework
dependencies and repository/adaptor patterns instead of domain-only constraints.
- Around line 372-402: The YAML currently attaches test conventions to
build/docs files via the path entry "path:
'{pom.xml,build.gradle,README.md,application.yml,application.properties}'" so
the rules never apply to test sources; move the rules body out of those config
files and reattach it to the test source glob (e.g., change the path to
something like src/test/java/** and/or src/test/resources/**) or place the rules
into a test-specific file under src/test/resources (e.g., test-rules.md) that
your linter/CI will read; ensure you update the YAML path entry and keep the
same rule sections ("Tooling", "Naming & File Suffixes", "Scope Rules", etc.) so
they are evaluated against actual test classes rather than pom/README/config
files.
In `@postman/SELF` SERVICE PLUGIN.postman_collection.json:
- Line 549: The sample flow is inconsistent: the request body shown sets the
tenant password to "MifosInitiative2026#", but the later "03 LOGIN - MIFOS BANK
1" request still uses the old `password` value; update the login request to use
the same password string (or replace both places with a single
collection/environment variable like {{mifos_bank_1_password}}) so the tenant
creation payload (requestId 4 with "password": "MifosInitiative2026#") and the
"03 LOGIN - MIFOS BANK 1" authentication request match; also update any
environment/collection variables or other requests that reference the tenant
password to the same identifier.
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 85-87: Add a Javadoc comment for the public method getId() in
class SelfBeneficiariesTPT: place a brief descriptive sentence and an `@return`
tag stating it returns the identifier (id) of the SelfBeneficiariesTPT entity
(Long), matching the style used for other public methods/constructors in the
class to satisfy the "Public methods and classes MUST have Javadoc" guideline.
In
`@src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java`:
- Around line 121-137: The reconstruct factory currently allows a null id which
breaks the invariant for persisted SelfServiceRegistration objects; update the
reconstruct method to validate id as non-null by calling
java.util.Objects.requireNonNull(id, "id cannot be null") (alongside the
existing checks for clientId and createdDate) so that the reconstructed
SelfServiceRegistration instance always has a non-null id; ensure this change
targets the reconstruct(...) method in class SelfServiceRegistration and does
not alter its return signature or constructor invocation.
---
Duplicate comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 41-58: The constructors in SelfBeneficiariesTPT must enforce
domain invariants by rejecting null/blank required fields; update both
constructors (the Long-arg constructor shown and the other constructor around
lines 64-83) to validate incoming parameters using Objects.requireNonNull for
required identifiers (appUserId, officeId, clientId, accountId, accountType) and
throw IllegalArgumentException if name is null or blank (trim and check length),
and update the update(...) method to defensively check/requireNonNull the name
before assigning to avoid NPEs; include clear exception messages naming the
offending parameter.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 283-285: The catch block in
SelfServiceRegistrationWritePlatformServiceImpl currently calls
ExceptionUtils.getRootCause(dve.getCause()), which can drop a direct cause;
change it to derive the actual cause by calling ExceptionUtils.getRootCause(dve)
and if that returns null fall back to dve (or dve.getCause() as appropriate)
before passing it into handleDataIntegrityIssues(command, ..., dve, username) so
the real PersistenceException/AuthenticationServiceException cause (used to
detect duplicate-username) is preserved.
---
Nitpick comments:
In @.coderabbit.yml:
- Around line 404-420: The changelog rule block currently mixes general project
standards with Liquibase-specific rules; extract the unrelated items and keep
this block focused on Liquibase. Edit the block that starts with path:
'src/main/resources/db/changelog/**' and remove the build/README/application.yml
entries (the sections about pom.xml/build.gradle, README.md, and
application.yml), leaving only Liquibase-related guidance (e.g., changelog
formatting, id conventions, labels, rollback requirements, and testing/migration
verification). Create or append the removed items into their appropriate
existing blocks for build files, README, and application config to avoid
duplication and noisy migration reviews.
In
`@src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationJpaEntity.java`:
- Around line 103-145: Make the three mapper/helper methods in
SelfServiceRegistrationJpaEntity package-private rather than public: change the
visibility of fromNewDomain(SelfServiceRegistration, Client), toDomain(), and
updateFromDomain(SelfServiceRegistration, Client) to have no access modifier
(remove "public") so they remain accessible inside the package but are not
exposed on the infrastructure API; keep signatures and behavior unchanged.
🪄 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: 59da977e-1967-4bc4-ae88-53233c7207da
📒 Files selected for processing (17)
.coderabbit.yml.gitignorepostman/SELF SERVICE PLUGIN.postman_collection.jsonsrc/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.javasrc/main/java/org/apache/fineract/selfservice/account/exception/SelfBeneficiariesTPTNotFoundException.javasrc/main/java/org/apache/fineract/selfservice/account/infrastructure/persistence/SelfBeneficiariesTPTRepositoryAdapter.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/infrastructure/persistence/SelfServiceRegistrationJpaEntity.javasrc/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUserClientMapping.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/007-drop-password-request-audit.xmlsrc/test/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUserClientMapping.java
- src/main/resources/db/changelog/tenant/module/selfservice/parts/007-drop-password-request-audit.xml
- src/main/java/org/apache/fineract/selfservice/account/exception/SelfBeneficiariesTPTNotFoundException.java
- src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/apache/fineract/selfservice/registration/infrastructure/persistence/SelfServiceRegistrationRepositoryAdapter.java
- src/main/java/org/apache/fineract/selfservice/account/infrastructure/persistence/SelfBeneficiariesTPTRepositoryAdapter.java
- src/test/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistrationTest.java
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
Show resolved
Hide resolved
src/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.java
Show resolved
Hide resolved
added 5 commits
April 11, 2026 20:46
IOhacker
requested changes
Apr 11, 2026
Contributor
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Please keep the password as password, the mifos bank 1 should keep it simple for testing of the mobile apps team.
Satisfies the project guideline that all public methods must have Javadoc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ct() A persisted entity must always have a DB-assigned id. Without this guard, a null id passed by the infrastructure adapter would silently produce an object that looks new/unsaved, breaking the aggregate invariant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e team Maintainer requested keeping the mifos-bank-1 tenant password as "password" to keep testing simple for the mobile apps team. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SelfBeneficiariesTPT.java: keep pure domain entity (no JPA annotations) as this branch tests the DDD migration away from @entity coupling - module-changelog-master.xml: include both 007 entries; renumber 007-drop-password-request-audit to 008 to avoid Liquibase ID collision with develop's 007-grant-selfservice-beneficiary-tpt-permissions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/account/domain/SelfBeneficiariesTPT.java`:
- Around line 41-83: Make the constructors on SelfBeneficiariesTPT validate and
enforce non-null invariants: in the creation constructor
(SelfBeneficiariesTPT(Long appUserId, String name, Long officeId, Long clientId,
Long accountId, Integer accountType, Long transferLimit)) require non-null for
appUserId, name, officeId, clientId, accountId (and any other mandatory fields)
and throw an IllegalArgumentException or use Objects.requireNonNull with clear
messages if any are null; in the reconstruction constructor
(SelfBeneficiariesTPT(Long id, Long appUserId, String name, Long officeId, Long
clientId, Long accountId, Integer accountType, Long transferLimit, boolean
isActive)) additionally require a non-null id and the same mandatory fields,
failing fast with descriptive exceptions to ensure the entity cannot be
instantiated in an invalid state.
- Around line 138-149: SelfBeneficiariesTPT.update currently treats null as "set
to null" because it only receives values, so calls like update(name,
transferLimit) will overwrite fields when the caller omitted them; change the
method signature on SelfBeneficiariesTPT.update to accept presence flags (e.g.,
update(boolean hasName, String newName, boolean hasTransferLimit, Long
newTransferLimit)) and only modify this.name when hasName is true and only
modify this.transferLimit when hasTransferLimit is true, returning the changes
map accordingly; then update the caller
SelfBeneficiariesTPTWritePlatformServiceImpl to call
beneficiary.update(params.containsKey(NAME_PARAM_NAME), name,
params.containsKey(TRANSFER_LIMIT_PARAM_NAME), transferLimit).
🪄 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: 62f1bc6a-e7fd-4ed7-9632-968be31af329
📒 Files selected for processing (7)
.coderabbit.ymlpostman/SELF SERVICE PLUGIN.postman_collection.jsonsrc/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.javasrc/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/008-drop-password-request-audit.xml
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/changelog/tenant/module/selfservice/parts/008-drop-password-request-audit.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xml
- postman/SELF SERVICE PLUGIN.postman_collection.json
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
Show resolved
Hide resolved
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
Show resolved
Hide resolved
…tructors A beneficiary cannot be valid without its core identifiers. Without these guards, null values silently propagate into authorization checks and persistence, causing cryptic failures far from the construction site. - Creation constructor: require appUserId, name, officeId, clientId, accountId, accountType (transferLimit stays nullable = no limit) - Reconstruction constructor: additionally require id since a hydrated entity must always have a DB-assigned key Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…T.update() update() previously treated null as "set to null", so a request that only sent transferLimit would silently clear name (and vice versa) because params.get() returns null for absent keys. Fix: add boolean presence flags (hasName, hasTransferLimit) so the method only mutates a field when the caller explicitly included it. Switch to Objects.equals() to handle null safely on both sides. Caller (SelfBeneficiariesTPTWritePlatformServiceImpl) updated to pass params.containsKey(...) for each flag. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java (1)
112-114:⚠️ Potential issue | 🟡 Minor
setName()allows null, violating constructor invariant.The constructors require
nameto be non-null viaObjects.requireNonNull, but this setter permits null assignment, breaking the invariant.Proposed fix
public void setName(String name) { + Objects.requireNonNull(name, "name cannot be null"); this.name = name; }As per coding guidelines, "Entities should encapsulate behavior and invariants."
🤖 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/account/domain/SelfBeneficiariesTPT.java` around lines 112 - 114, The setter setName in SelfBeneficiariesTPT allows assigning null and breaks the non-null invariant enforced by the constructors (Objects.requireNonNull). Update setName(String name) to enforce the same invariant—call Objects.requireNonNull(name, "name must not be null") (or throw a clear NPE/IllegalArgumentException) before assigning to this.name so the class invariant remains consistent with the constructors.
🧹 Nitpick comments (2)
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java (2)
132-150: Missing Javadoc on remaining public getters.The methods
getAppUserId(),getOfficeId(),getClientId(),getAccountId(), andgetAccountType()lack documentation.As per coding guidelines, "Public methods and classes MUST have Javadoc."
🤖 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/account/domain/SelfBeneficiariesTPT.java` around lines 132 - 150, Add Javadoc comments for each public getter in SelfBeneficiariesTPT: getAppUserId(), getOfficeId(), getClientId(), getAccountId(), and getAccountType(). For each method, write a one-line Javadoc description (e.g., "Returns the application user identifier.") and include `@return` tags describing the returned value type and semantics; place the comments directly above each method declaration in the SelfBeneficiariesTPT class to satisfy the project's "Public methods MUST have Javadoc" guideline.
108-130: Missing Javadoc on public getter/setter methods.Public methods
getName(),setName(),getTransferLimit(),setTransferLimit(),isActive(), andsetActive()lack documentation.As per coding guidelines, "Public methods and classes MUST have Javadoc."
🤖 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/account/domain/SelfBeneficiariesTPT.java` around lines 108 - 130, Add Javadoc comments to the public getters/setters in class SelfBeneficiariesTPT: document getName(), setName(String name), getTransferLimit(), setTransferLimit(Long transferLimit), isActive(), and setActive(boolean isActive). For each getter include a brief description and `@return` tag, and for each setter include a brief description and `@param` tag describing the parameter; ensure the Javadoc is concise, follows existing project style, and is placed immediately above each method declaration.
🤖 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/account/domain/SelfBeneficiariesTPT.java`:
- Around line 152-165: The update method currently allows name to be set to null
when hasName is true; modify
org.apache.fineract.selfservice.account.domain.SelfBeneficiariesTPT.update to
enforce the non-null invariant by validating newName when hasName is true
(either throw a IllegalArgumentException or reject the update) instead of
assigning null to this.name; keep the existing presence-flag behavior for
transferLimit and ensure the validation aligns with the constructors that
require a non-null name so the entity never ends up with a null name.
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 112-114: The setter setName in SelfBeneficiariesTPT allows
assigning null and breaks the non-null invariant enforced by the constructors
(Objects.requireNonNull). Update setName(String name) to enforce the same
invariant—call Objects.requireNonNull(name, "name must not be null") (or throw a
clear NPE/IllegalArgumentException) before assigning to this.name so the class
invariant remains consistent with the constructors.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 132-150: Add Javadoc comments for each public getter in
SelfBeneficiariesTPT: getAppUserId(), getOfficeId(), getClientId(),
getAccountId(), and getAccountType(). For each method, write a one-line Javadoc
description (e.g., "Returns the application user identifier.") and include
`@return` tags describing the returned value type and semantics; place the
comments directly above each method declaration in the SelfBeneficiariesTPT
class to satisfy the project's "Public methods MUST have Javadoc" guideline.
- Around line 108-130: Add Javadoc comments to the public getters/setters in
class SelfBeneficiariesTPT: document getName(), setName(String name),
getTransferLimit(), setTransferLimit(Long transferLimit), isActive(), and
setActive(boolean isActive). For each getter include a brief description and
`@return` tag, and for each setter include a brief description and `@param` tag
describing the parameter; ensure the Javadoc is concise, follows existing
project style, and is placed immediately above each method declaration.
🪄 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: 6bdd9f1f-87b6-4eb5-b27c-12c36e632dbf
📒 Files selected for processing (2)
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.javasrc/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java
src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
Show resolved
Hide resolved
Both mutation paths could bypass the non-null invariant enforced by the constructors, allowing this.name to become null silently. - setName(): add Objects.requireNonNull guard before assignment - update(): when hasName is true, validate newName before the equality check so a null name is rejected rather than written to the entity Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
Hi @IOhacker, i am done with the coderabbit comments and overall migration (took some help from claude code to resolve some issues). I have addressed your comments also. Kindly review once and trigger the github actions to check. |
Contributor
|
@ansh-varshney let us complete the fixes (this weekend) so then you can continue with the efforts :) Thank you! |
Author
|
Yes, Victor! Let me know if there is need for any help :) |
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.
Summary by CodeRabbit
New Features
Configuration
Behavioral Change
Documentation