MX-203: Fix the API 03. TRANSFER TO THIRD PARTY - #120
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 11 minutes and 57 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 (9)
📝 WalkthroughWalkthroughReworks self-service account transfer creation to accept raw JSON, validate it, and log a CommandWrapper via the command service instead of directly invoking the account-transfers API; adds transfer-related API constants, updates Swagger types (BigDecimal/String) and adds Liquibase changelogs to grant selfservice permissions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SelfServiceAPI as SelfAccountTransferApiResource
participant Validator as SelfAccountTransferDataValidator
participant CommandService as PortfolioCommandSourceWritePlatformService
participant DB as Database
Client->>SelfServiceAPI: POST /accounttransfers (raw JSON)
SelfServiceAPI->>Validator: validateCreate(type, rawJson)
Validator-->>SelfServiceAPI: validation result
SelfServiceAPI->>CommandService: logCommandSource(CommandWrapper(createAccountTransfer, rawJson))
CommandService->>DB: persist command / enqueue processing
DB-->>CommandService: ack
CommandService-->>SelfServiceAPI: CommandProcessingResult
SelfServiceAPI-->>Client: return CommandProcessingResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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: 3
🧹 Nitpick comments (2)
src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResource.java (2)
108-109: Add Jakarta validation on request body.Annotate the request object with
@Validand enforce DTO constraints at the API boundary.As per coding guidelines, "Validate inputs with
@Validand Jakarta validation annotations."🤖 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/api/SelfAccountTransferApiResource.java` around lines 108 - 109, In the SelfAccountTransferApiResource#create method, annotate the request parameter AccountTransferRequest apiRequestBodyAsJson with `@Valid` so Jakarta Bean Validation runs at the API boundary, add the corresponding import (jakarta.validation.Valid), and ensure AccountTransferRequest DTO has appropriate constraint annotations (e.g., `@NotNull/`@Size) to be enforced; this makes validation occur automatically when the create(...) method is invoked.
117-148: Move limit-check logic out of the controller.
checkForLimits(...)contains business rules and should live in an application/service component to keep controller responsibilities narrow.As per coding guidelines, "Controllers must delegate all logic to application or domain 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/api/SelfAccountTransferApiResource.java` around lines 117 - 148, Move the business rules out of the controller by extracting checkForLimits into an application/service class (e.g., SelfAccountTransferLimitService) and have SelfAccountTransferApiResource call that service; specifically, create a method validateTransferLimits(...) that contains the current logic which uses tptBeneficiaryReadPlatformService.getTransferLimit(...), configurationDomainService.isDailyTPTLimitEnabled()/getDailyTPTLimit(), and accountTransfersReadPlatformService.getTotalTransactionAmount(...), and preserve the thrown exceptions BeneficiaryTransferLimitExceededException and DailyTPTTransactionAmountLimitExceededException; inject the new service into SelfAccountTransferApiResource, replace the internal checkForLimits call with a delegate call to validateTransferLimits(fromAccount, toAccount, transactionDate, transactionAmount, userId) and remove business logic from the controller method.
🤖 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/api/SelfAccountTransferApiResource.java`:
- Around line 140-145: The current guard skips enforcement when
totTransactionAmount is null or zero; instead treat null as BigDecimal.ZERO and
always validate the single transfer and the aggregate. Replace the conditional
using totTransactionAmount with: compute BigDecimal currentTotal =
(totTransactionAmount == null ? BigDecimal.ZERO : totTransactionAmount) and then
if (dailyTPTLimitBD.compareTo(transactionAmount) < 0 ||
dailyTPTLimitBD.compareTo(currentTotal.add(transactionAmount)) < 0) throw the
existing DailyTPTTransactionAmountLimitExceededException (use
fromAccount.getAccountId() and fromAccount.getAccountType() as before). Ensure
you reference totTransactionAmount, transactionAmount, dailyTPTLimitBD and the
exception class in the updated check.
- Around line 108-110: The call to validateCreate currently passes
apiRequestBodyAsJson.toString(), which is not valid JSON; update the create
method to either (A) pass a real JSON string produced by a serializer (e.g.,
serialize AccountTransferRequest to JSON with your configured ObjectMapper) into
validateCreate(type, jsonString) so fromApiJsonHelper.parse() can succeed, or
(B) change validateCreate to accept the typed DTO and validate its fields
directly (e.g., add an overload validateCreate(String type,
AccountTransferRequest dto) or extract fields from AccountTransferRequest and
call existing validation helpers). Locate the create method in
SelfAccountTransferApiResource and adjust the call to validateCreate or the
validateCreate signature accordingly (references: AccountTransferRequest,
create(...), validateCreate(...), fromApiJsonHelper.parse()).
In
`@src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResourceSwagger.java`:
- Around line 131-132: The Schema example for the dateFormat field uses the
week-based year token "YYYY" which can be incorrect around year boundaries;
update the `@Schema` example on the public String dateFormat field in
SelfAccountTransferApiResourceSwagger to use the calendar-year token "yyyy"
(e.g., change "dd MMMM YYYY" to "dd MMMM yyyy").
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResource.java`:
- Around line 108-109: In the SelfAccountTransferApiResource#create method,
annotate the request parameter AccountTransferRequest apiRequestBodyAsJson with
`@Valid` so Jakarta Bean Validation runs at the API boundary, add the
corresponding import (jakarta.validation.Valid), and ensure
AccountTransferRequest DTO has appropriate constraint annotations (e.g.,
`@NotNull/`@Size) to be enforced; this makes validation occur automatically when
the create(...) method is invoked.
- Around line 117-148: Move the business rules out of the controller by
extracting checkForLimits into an application/service class (e.g.,
SelfAccountTransferLimitService) and have SelfAccountTransferApiResource call
that service; specifically, create a method validateTransferLimits(...) that
contains the current logic which uses
tptBeneficiaryReadPlatformService.getTransferLimit(...),
configurationDomainService.isDailyTPTLimitEnabled()/getDailyTPTLimit(), and
accountTransfersReadPlatformService.getTotalTransactionAmount(...), and preserve
the thrown exceptions BeneficiaryTransferLimitExceededException and
DailyTPTTransactionAmountLimitExceededException; inject the new service into
SelfAccountTransferApiResource, replace the internal checkForLimits call with a
delegate call to validateTransferLimits(fromAccount, toAccount, transactionDate,
transactionAmount, userId) and remove business logic from the controller method.
🪄 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: 5ee92fee-e6fd-42bb-941c-8c475817c9da
📒 Files selected for processing (6)
src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResource.javasrc/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResourceSwagger.javasrc/main/java/org/apache/fineract/selfservice/account/data/SelfAccountTransferDataValidator.javasrc/main/java/org/apache/fineract/selfservice/client/api/SelfClientsApiResourceSwagger.javasrc/main/java/org/apache/fineract/selfservice/loanaccount/api/SelfLoansApiResourceSwagger.javasrc/main/java/org/apache/fineract/selfservice/savings/api/SelfSavingsAccountApiResourceSwagger.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java (1)
48-60: Rename newly added constant fields toUPPER_CASE.These
public static finalfields are introduced in camelCase; please align them with Java constant convention.Proposed refactor
- public static final String toOfficeIdParamName = "toOfficeId"; - public static final String toClientIdParamName = "toClientId"; - public static final String toAccountTypeParamName = "toAccountType"; + public static final String TO_OFFICE_ID_PARAM_NAME = "toOfficeId"; + public static final String TO_CLIENT_ID_PARAM_NAME = "toClientId"; + public static final String TO_ACCOUNT_TYPE_PARAM_NAME = "toAccountType";As per coding guidelines, "Constants: UPPER_CASE."
🤖 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 48 - 60, The new public static final fields in SelfServiceApiConstants use camelCase; rename each to UPPER_CASE (e.g., toOfficeIdParamName -> TO_OFFICE_ID_PARAM_NAME, toClientIdParamName -> TO_CLIENT_ID_PARAM_NAME, toAccountTypeParamName -> TO_ACCOUNT_TYPE_PARAM_NAME, toAccountIdParamName -> TO_ACCOUNT_ID_PARAM_NAME, transferDateParamName -> TRANSFER_DATE_PARAM_NAME, transferAmountParamName -> TRANSFER_AMOUNT_PARAM_NAME, transferDescriptionParamName -> TRANSFER_DESCRIPTION_PARAM_NAME, dateFormatParamName -> DATE_FORMAT_PARAM_NAME, localeParamName -> LOCALE_PARAM_NAME, fromAccountIdParamName -> FROM_ACCOUNT_ID_PARAM_NAME, fromAccountTypeParamName -> FROM_ACCOUNT_TYPE_PARAM_NAME, fromClientIdParamName -> FROM_CLIENT_ID_PARAM_NAME, fromOfficeIdParamName -> FROM_OFFICE_ID_PARAM_NAME) and update all usages across the codebase to reference the new names.src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResource.java (2)
140-140: PreferBigDecimal.valueOf()overnew BigDecimal(Long).Same recommendation as above for consistency.
Proposed fix
- BigDecimal dailyTPTLimitBD = new BigDecimal(dailyTPTLimit); + BigDecimal dailyTPTLimitBD = BigDecimal.valueOf(dailyTPTLimit);🤖 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/api/SelfAccountTransferApiResource.java` at line 140, Replace the direct BigDecimal constructor usage for long-to-BigDecimal conversion to use BigDecimal.valueOf to avoid unexpected behavior; locate the instantiation of dailyTPTLimitBD in SelfAccountTransferApiResource (the line creating BigDecimal dailyTPTLimitBD = new BigDecimal(dailyTPTLimit)) and change it to use BigDecimal.valueOf(dailyTPTLimit) so the long value is converted safely and consistently.
131-135: PreferBigDecimal.valueOf()overnew BigDecimal(Long).Using
BigDecimal.valueOf(long)is preferred as it may reuse cached instances for small values.Proposed fix
if (transferLimit != null && transferLimit > 0) { - if (transactionAmount.compareTo(new BigDecimal(transferLimit)) > 0) { + if (transactionAmount.compareTo(BigDecimal.valueOf(transferLimit)) > 0) { throw new BeneficiaryTransferLimitExceededException(); } }🤖 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/api/SelfAccountTransferApiResource.java` around lines 131 - 135, The comparison uses new BigDecimal(transferLimit); replace that with BigDecimal.valueOf(transferLimit) to avoid creating unnecessary BigDecimal instances — locate the code inside SelfAccountTransferApiResource where transactionAmount is compared to new BigDecimal(transferLimit) (using the transferLimit variable) and change it to transactionAmount.compareTo(BigDecimal.valueOf(transferLimit)) so the logic and exception throw (BeneficiaryTransferLimitExceededException) remain the same.
🤖 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/api/SelfAccountTransferApiResource.java`:
- Line 69: Remove the unused field accountTransfersApiResource from the
SelfAccountTransferApiResource class and delete its corresponding import: locate
the private final AccountTransfersApiResource accountTransfersApiResource
declaration in SelfAccountTransferApiResource and remove it, and also remove the
AccountTransfersApiResource import statement since the class now uses
commandsSourceWritePlatformService.logCommandSource() and no longer references
accountTransfersApiResource.
In
`@src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java`:
- Line 47: The comment "Funds Tranfer parameters" in SelfServiceApiConstants
(the transfer section) contains a typo; change "Tranfer" to "Transfer" so it
reads "Funds Transfer parameters" and ensure the surrounding comment remains
consistent with other comments in the class (look for the existing comment near
the transfer-related constants).
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResource.java`:
- Line 140: Replace the direct BigDecimal constructor usage for
long-to-BigDecimal conversion to use BigDecimal.valueOf to avoid unexpected
behavior; locate the instantiation of dailyTPTLimitBD in
SelfAccountTransferApiResource (the line creating BigDecimal dailyTPTLimitBD =
new BigDecimal(dailyTPTLimit)) and change it to use
BigDecimal.valueOf(dailyTPTLimit) so the long value is converted safely and
consistently.
- Around line 131-135: The comparison uses new BigDecimal(transferLimit);
replace that with BigDecimal.valueOf(transferLimit) to avoid creating
unnecessary BigDecimal instances — locate the code inside
SelfAccountTransferApiResource where transactionAmount is compared to new
BigDecimal(transferLimit) (using the transferLimit variable) and change it to
transactionAmount.compareTo(BigDecimal.valueOf(transferLimit)) so the logic and
exception throw (BeneficiaryTransferLimitExceededException) remain the same.
In
`@src/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.java`:
- Around line 48-60: The new public static final fields in
SelfServiceApiConstants use camelCase; rename each to UPPER_CASE (e.g.,
toOfficeIdParamName -> TO_OFFICE_ID_PARAM_NAME, toClientIdParamName ->
TO_CLIENT_ID_PARAM_NAME, toAccountTypeParamName -> TO_ACCOUNT_TYPE_PARAM_NAME,
toAccountIdParamName -> TO_ACCOUNT_ID_PARAM_NAME, transferDateParamName ->
TRANSFER_DATE_PARAM_NAME, transferAmountParamName -> TRANSFER_AMOUNT_PARAM_NAME,
transferDescriptionParamName -> TRANSFER_DESCRIPTION_PARAM_NAME,
dateFormatParamName -> DATE_FORMAT_PARAM_NAME, localeParamName ->
LOCALE_PARAM_NAME, fromAccountIdParamName -> FROM_ACCOUNT_ID_PARAM_NAME,
fromAccountTypeParamName -> FROM_ACCOUNT_TYPE_PARAM_NAME, fromClientIdParamName
-> FROM_CLIENT_ID_PARAM_NAME, fromOfficeIdParamName ->
FROM_OFFICE_ID_PARAM_NAME) and update all usages across the codebase to
reference the new names.
🪄 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: 890dc3ad-2526-4177-beb4-0f5b027840fc
📒 Files selected for processing (7)
src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResource.javasrc/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResourceSwagger.javasrc/main/java/org/apache/fineract/selfservice/account/data/SelfAccountTransferDataValidator.javasrc/main/java/org/apache/fineract/selfservice/registration/SelfServiceApiConstants.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/006-grant-selfservice-savings-account-read-permissions.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/008-grant-selfservice-savings-account-create-transfer-permissions.xml
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/db/changelog/tenant/module/selfservice/parts/006-grant-selfservice-savings-account-read-permissions.xml
- src/main/resources/db/changelog/tenant/module/selfservice/parts/008-grant-selfservice-savings-account-create-transfer-permissions.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResourceSwagger.java
- src/main/java/org/apache/fineract/selfservice/account/data/SelfAccountTransferDataValidator.java
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/api/SelfAccountTransferApiResourceSwagger.java`:
- Line 27: Add Javadoc comments to the public nested Swagger classes to satisfy
the project's documentation standard: insert a brief Javadoc block above the
class declaration for GetAccountTransferTemplateResponse describing its purpose
and key fields/usage, and do the same for the other public nested Swagger
classes in this file (the other public nested classes referenced in the review).
Ensure each Javadoc follows the project's style (summary sentence, optional
`@since/`@author if used) and is placed directly above the class declaration so
all public classes have Javadoc.
- Around line 62-63: Update the misspelled example value "Jhon Doe" to "John
Doe" in the Swagger schema annotations inside
SelfAccountTransferApiResourceSwagger; locate the `@Schema`(example = "Jhon Doe")
occurrences (e.g., on the public String clientName field and the other similar
field around lines 89-90) and change the example to "John Doe" so API docs show
the correct name.
🪄 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: 5a15ffe7-5670-48cf-8652-4f0dd9f11954
📒 Files selected for processing (1)
src/main/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferApiResourceSwagger.java
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
1 participant
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.
MX-203: Fix the API 03. TRANSFER TO THIRD PARTY
Summary by CodeRabbit
New Features
Chores