MX-234: Add integration tests for Self Service Transfer to Third Party (TPT) - #130
Merged
IOhacker merged 1 commit intoApr 12, 2026
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 43 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)
📝 WalkthroughWalkthroughAdds a new integration test that performs an end-to-end self-service Third-Party Transfer (TPT) flow, including setup/teardown of configuration and permissions, seeding clients/accounts, inserting/authenticating a self-service user, creating a beneficiary, executing the TPT, and asserting balances. Also renames an existing TPT beneficiary test class. Changes
Sequence Diagram(s)(Skipped — changes are test additions and a class rename; no new runtime control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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: 4
🧹 Nitpick comments (1)
src/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.java (1)
197-425: Extract the shared seeding flow into test support.The client/savings/self-service-user bootstrap here overlaps heavily with
src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java, especially the direct user seeding and authentication path. Pull that fixture code intoorg.apache.fineract.selfservice.testing.supportso these tests only describe scenario-specific setup and assertions.As per coding guidelines,
**: Apply DRY, SOLID, and Clean Architecture principles.🤖 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/account/api/SelfAccountTransferTPTIntegrationTest.java` around lines 197 - 425, Extract the duplicated seeding/bootstrap logic (seedClientWithFundedSavings, seedClientWithActiveSavings, createClientAndExtract, createZeroFeeSavingsProduct, openSavingsAccount, approveSavingsAccount, activateSavingsAccount, depositToSavingsAccount, insertSelfServiceUserDirectly, authenticateSelfUser) into a new test support class under package org.apache.fineract.selfservice.testing.support (e.g., SelfServiceTestFixture) and implement reusable methods for client creation, savings product/account lifecycle, DB self-service user insertion and authentication; refactor both SelfAccountTransferTPTIntegrationTest and SelfBeneficiaryTPTIntegrationTest to call these support methods (passing in parameters like label, today) so tests only perform scenario-specific actions/assertions and delete the duplicated private helper methods from the tests. Ensure the support class uses the existing SelfServiceTestUtils helpers and preserves current behavior and exceptions.
🤖 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/account/api/SelfAccountTransferTPTIntegrationTest.java`:
- Around line 178-195: The grantTransferPermissionToSelfServiceRole method
mutates the shared role; update it to snapshot the role's existing permissions
before modifying and restore them after the test (use try/finally or test
teardown). Specifically: call the ADMIN_ROLES_PATH GET (using
SELF_SERVICE_USER_ROLE to locate roleId) to extract the current permissions map,
then PUT the modified permissions (adding CREATE_ACCOUNTTRANSFER) for the test,
and in the finally/teardown PUT the original permissions map back to
ADMIN_ROLES_PATH + "/" + roleId + "/permissions to restore state. Ensure the
snapshot variable is local to this method and that restoration runs even if
assertions fail.
- Around line 149-176: The test helper disableDailyTptLimit currently flips the
global "daily-tpt-limit" config without restoring it; modify it to read and
store the original configuration value (the enabled flag and/or full config
object) when querying ADMIN_CONFIGURATIONS_PATH, then disable it, and ensure the
original value is restored in a finally block or an `@AfterEach` teardown; update
the method (disableDailyTptLimit) to either return the original state or make a
paired restoreDailyTptLimit(original) method and call that in test cleanup so
the global state is reset after the test.
- Around line 348-416: The method insertSelfServiceUserDirectly currently uses a
plain JDBC Connection (conn) with auto-commit on, causing each INSERT to commit
independently; disable auto-commit on conn (conn.setAutoCommit(false)) before
running the multiple INSERTs, call conn.commit() after all statements succeed,
and ensure conn.rollback() is invoked in the exception path (or a finally block)
to revert partial changes; update the try-with-resources structure around
Connection/PreparedStatement/ResultSet so the transaction boundaries are managed
for the sequence of statements (INSERT INTO m_appuser, m_appuser_role,
m_appselfservice_user, m_appselfservice_user_role,
m_selfservice_user_client_mapping) and rethrow the original exception as
currently done.
- Around line 35-36: The test uses JVM defaults for locale/timezone causing
flaky month names and date offsets; update SelfAccountTransferTPTIntegrationTest
to pin both: change FORMATTER to DateTimeFormatter.ofPattern(DATE_FORMAT,
Locale.ENGLISH) and ensure any date construction/formatting uses UTC (e.g., use
ZonedDateTime.now(ZoneOffset.UTC) or LocalDate.now(ZoneId.of("UTC")) when
producing the request date) so DATE_FORMAT and FORMATTER produce consistent
English month names and UTC-based dates.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.java`:
- Around line 197-425: Extract the duplicated seeding/bootstrap logic
(seedClientWithFundedSavings, seedClientWithActiveSavings,
createClientAndExtract, createZeroFeeSavingsProduct, openSavingsAccount,
approveSavingsAccount, activateSavingsAccount, depositToSavingsAccount,
insertSelfServiceUserDirectly, authenticateSelfUser) into a new test support
class under package org.apache.fineract.selfservice.testing.support (e.g.,
SelfServiceTestFixture) and implement reusable methods for client creation,
savings product/account lifecycle, DB self-service user insertion and
authentication; refactor both SelfAccountTransferTPTIntegrationTest and
SelfBeneficiaryTPTIntegrationTest to call these support methods (passing in
parameters like label, today) so tests only perform scenario-specific
actions/assertions and delete the duplicated private helper methods from the
tests. Ensure the support class uses the existing SelfServiceTestUtils helpers
and preserves current behavior and exceptions.
🪄 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: 223b223d-24ee-4fa2-a3cd-73c993280a76
📒 Files selected for processing (2)
src/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java
DeathGun44
force-pushed
the
MX-234-Add-Transfer-To-Third-Party-Integration-Tests
branch
from
April 12, 2026 08:26
e8b909a to
dcbd4e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.java (2)
35-35: Rename the integration test class to*IT(and file accordingly).Use the integration-test naming pattern to align with repository test standards.
As per coding guidelines,
src/test/java/**: Name tests ClassNameTest or ClassNameIT (for integration).🤖 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/account/api/SelfAccountTransferTPTIntegrationTest.java` at line 35, Rename the test class SelfAccountTransferTPTIntegrationTest to follow the integration-test pattern by changing the class name to SelfAccountTransferTPTIT and renaming the file accordingly; update the class declaration (class SelfAccountTransferTPTIT extends SelfServiceIntegrationTestBase), rename the Java file to match the new class name, and update any references/imports/usages (test suites, CI configs, or other classes referencing SelfAccountTransferTPTIntegrationTest) to the new identifier so compilation and test discovery continue to work.
10-11: Use AssertJ assertions instead of JUnit Assertions/Hamcrest in test assertions.This class mixes JUnit
Assertionsand Hamcrest matchers; switch assertions to AssertJ for consistency with project testing standards.As per coding guidelines,
src/test/java/**: Use JUnit 5, Mockito, and AssertJ.Also applies to: 31-31, 122-149
🤖 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/account/api/SelfAccountTransferTPTIntegrationTest.java` around lines 10 - 11, The test class SelfAccountTransferTPTIntegrationTest currently uses JUnit Assertions and Hamcrest (e.g., static import equalTo) — replace those with AssertJ: remove Hamcrest and JUnit assert imports and add org.assertj.core.api.Assertions.assertThat; convert patterns like assertThat(actual, equalTo(expected)) or Assertions.assertEquals(expected, actual) to assertThat(actual).isEqualTo(expected), and replace other Hamcrest matchers with the equivalent AssertJ fluent assertions (isTrue/isFalse/isNotNull/contains/...); apply these changes across all assertions in this class (including assertions referenced around the other occurrences) so the tests consistently use AssertJ.
🤖 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/account/api/SelfAccountTransferTPTIntegrationTest.java`:
- Around line 58-61: The setup mutates global state before the try block (calls
to disableDailyTptLimit() and grantTransferPermissionToSelfServiceRole()), so if
either fails the corresponding restoration never runs; move the state-mutation
calls into a protected section and ensure both
restoreDailyTptLimit(originalTptLimit) and
restoreTransferPermission(originalPermission) are invoked from a finally block
that always executes (and call each restore inside its own try/catch so a
failure in one does not prevent the other). Update
SelfAccountTransferTPTIntegrationTest to capture the original values, perform
mutations inside the try, and in finally attempt both restoreDailyTptLimit and
restoreTransferPermission regardless of earlier failures, logging or swallowing
individual restore exceptions so both are attempted.
- Around line 53-154: Add at least one negative integration test in
SelfAccountTransferTPTIntegrationTest (e.g., new method
transferToThirdParty_exceedsBeneficiaryLimit_returns4xx or
transferToThirdParty_insufficientBalance_returns4xx) that follows the same setup
steps as transferToThirdParty_movesExpectedFunds_returns200 (use
seedClientWithFundedSavings/seedClientWithActiveSavings,
insertSelfServiceUserDirectly, authenticateSelfUser, create a beneficiary via
BENEFICIARIES_PATH) but modifies the scenario to fail (set
beneficiary.transferLimit smaller than transferAmount, or revoke permission via
restoreTransferPermission/grantTransferPermissionToSelfServiceRole, or use a
sender with insufficient balance), perform the POST to ACCOUNT_TRANSFERS_PATH +
"?type=tpt", assert the expected error status code and message, and verify
balances remain unchanged; ensure you clean up by restoring DailyTptLimit and
permissions with restoreDailyTptLimit/restoreTransferPermission as in the
existing test.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.java`:
- Line 35: Rename the test class SelfAccountTransferTPTIntegrationTest to follow
the integration-test pattern by changing the class name to
SelfAccountTransferTPTIT and renaming the file accordingly; update the class
declaration (class SelfAccountTransferTPTIT extends
SelfServiceIntegrationTestBase), rename the Java file to match the new class
name, and update any references/imports/usages (test suites, CI configs, or
other classes referencing SelfAccountTransferTPTIntegrationTest) to the new
identifier so compilation and test discovery continue to work.
- Around line 10-11: The test class SelfAccountTransferTPTIntegrationTest
currently uses JUnit Assertions and Hamcrest (e.g., static import equalTo) —
replace those with AssertJ: remove Hamcrest and JUnit assert imports and add
org.assertj.core.api.Assertions.assertThat; convert patterns like
assertThat(actual, equalTo(expected)) or Assertions.assertEquals(expected,
actual) to assertThat(actual).isEqualTo(expected), and replace other Hamcrest
matchers with the equivalent AssertJ fluent assertions
(isTrue/isFalse/isNotNull/contains/...); apply these changes across all
assertions in this class (including assertions referenced around the other
occurrences) so the tests consistently use AssertJ.
🪄 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: 0054240a-d559-4cf8-afd9-849d4febc139
📒 Files selected for processing (2)
src/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java
DeathGun44
force-pushed
the
MX-234-Add-Transfer-To-Third-Party-Integration-Tests
branch
from
April 12, 2026 08:32
dcbd4e7 to
6fe0ab2
Compare
…y (TPT) - Explicitly specify UTC timezone and format locale for API dates - Extract Beneficiary creation and validation boundaries - Manage state cleanly by storing/restoring global configs and privileges - Implement explicitly bounded JDBC transactions for local database seeding - Add out-of-bounds transfer edge case coverage
DeathGun44
force-pushed
the
MX-234-Add-Transfer-To-Third-Party-Integration-Tests
branch
from
April 12, 2026 08:33
6fe0ab2 to
ae3c9f9
Compare
This was referenced Apr 13, 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.
fixes https://mifosforge.jira.com/browse/MX-234
Summary by CodeRabbit