MX-233: Fix ClassCastExceptions in self-service beneficiary operations - #119
Merged
IOhacker merged 1 commit intoApr 11, 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 9 minutes and 13 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 (8)
📝 WalkthroughWalkthroughGenerifies an entity ID type, removes redundant casts, adds a command-scoped security-context override for self-service principals, grants TPT-beneficiary permissions via Liquibase, introduces E2E integration tests and test-container classpath startup adjustments, and expands security-context unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (RestAssured)
participant API as SelfService API
participant Sec as SelfServiceCompatibleSecurityContext
participant Service as Beneficiary Service
participant DB as Database (Postgres)
Test->>API: POST /api/v1/self/beneficiaries/tpt (with token)
API->>Sec: authenticatedUser(CommandWrapper)
Sec->>DB: lookup principal / stub mapping
DB-->>Sec: principal data
Sec-->>API: AppUser stub
API->>Service: create beneficiary with AppUser
Service->>DB: insert beneficiary row
DB-->>Service: inserted id
Service-->>API: creation result (resourceId)
API-->>Test: HTTP 200 { resourceId }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 3
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/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java (1)
175-175:⚠️ Potential issue | 🟡 MinorTypo in log message.
"occured" should be "occurred".
Proposed fix
- log.error("Error occured.", dae); + log.error("Error occurred.", dae);🤖 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/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java` at line 175, Fix the typo in the log message inside SelfBeneficiariesTPTWritePlatformServiceImpl where the error is logged; change the log.error call that currently uses "Error occured." to use the correct spelling "Error occurred." (keep passing the same exception variable, e.g., dae).
🧹 Nitpick comments (5)
src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java (3)
38-38: Test class naming convention.Per coding guidelines, integration tests should be named
*IT. Consider renaming toSelfBeneficiaryTPTIT.🤖 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/SelfBeneficiaryTPTIntegrationTest.java` at line 38, The test class SelfBeneficiaryTPTIntegrationTest violates the project's IT naming convention; rename the class SelfBeneficiaryTPTIntegrationTest to SelfBeneficiaryTPTIT (update the class declaration and the Java filename to match) and update any references/imports or test suite entries that instantiate or refer to SelfBeneficiaryTPTIntegrationTest to the new SelfBeneficiaryTPTIT name so the build and test discovery continue to work.
155-156: Implicit dependency on mifos user's password.The seeded user copies
mifos's password hash (line 155) and authenticates with plaintext "password" (line 200). This works only because the defaultmifospassword is "password". Consider adding a comment documenting this assumption.🤖 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/SelfBeneficiaryTPTIntegrationTest.java` around lines 155 - 156, The test inserts a user by copying the password hash from the existing m_appuser row for username 'mifos' (the SQL VALUES string in SelfBeneficiaryTPTIntegrationTest) but later authenticates with the plaintext "password", which implicitly relies on the default mifos password; add a concise comment next to that SQL/insert (in SelfBeneficiaryTPTIntegrationTest) stating this assumption (that the test depends on the default 'mifos' password being "password") and, optionally, note that a more robust fix would be to set a known password explicitly or derive it from the test seed.
229-234: Consider using AssertJ for consistency.The test uses JUnit
Assertionswhile the coding guidelines recommend AssertJ. The unit tests in this PR use AssertJ's fluent assertions. Consider using AssertJ for consistency:Example with AssertJ
+import static org.assertj.core.api.Assertions.assertThat; ... - Assertions.assertEquals( - 200, - response.statusCode(), - "Expected 200 but got: " + response.statusCode() + ". Body: " + response.body().asString()); - Integer resourceId = response.jsonPath().getInt("resourceId"); - Assertions.assertNotNull(resourceId, "resourceId should be present in response"); + assertThat(response.statusCode()) + .as("Expected 200 but got: %d. Body: %s", response.statusCode(), response.body().asString()) + .isEqualTo(200); + assertThat(response.jsonPath().getInt("resourceId")) + .as("resourceId should be present in response") + .isNotNull();🤖 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/SelfBeneficiaryTPTIntegrationTest.java` around lines 229 - 234, Replace the JUnit Assertions calls in SelfBeneficiaryTPTIntegrationTest (the assertions on response.statusCode() and resourceId) with AssertJ fluent assertions to match the project's style: assert the status code with AssertJ's assertThat(response.statusCode()).isEqualTo(200) (include a failure message via withFailMessage or as a description if needed) and assert the resourceId with assertThat(resourceId).isNotNull(); locate the two statements that use Assertions.assertEquals and Assertions.assertNotNull and switch them to the corresponding AssertJ methods.src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContext.java (1)
112-120: Reflection-based ID injection is fragile.This approach depends on the superclass field name remaining "id". Consider adding a comment documenting this coupling and why it's necessary, or verify the field exists in a test to catch breaking changes early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContext.java` around lines 112 - 120, The reflection-based injection in setId(AppUser stub, Long id) relies on the superclass having a field named "id", which is fragile; update the setId method to include a clear comment above it explaining why reflection is used, the dependency on the superclass field name "id", and the risk if the superclass changes, and add a small unit test (targeting SelfServiceCompatibleSecurityContext.setId behavior) that asserts the superclass declares a field named "id" (failing fast if the name changes) so future refactors surface the breakage early; reference the setId method, AppUser class, and the id field in both the comment and the test.src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java (1)
127-139: Potential NullPointerException ifnewNameis null.Line 129 calls
this.name.equals(newName). IfnewNameis null, this works correctly. However, reversing toObjects.equals(this.name, newName)would be more defensive and handle the case wherethis.namecould theoretically be null.Proposed defensive null handling
+import java.util.Objects; ... public Map<String, Object> update(String newName, Long newTransferLimit) { Map<String, Object> changes = new HashMap<>(); - if (!this.name.equals(newName)) { + if (!Objects.equals(this.name, newName)) { this.name = newName; changes.put(NAME_PARAM_NAME, newName); } - if ((this.transferLimit != null && !this.transferLimit.equals(newTransferLimit)) - || (this.transferLimit == null && newTransferLimit != null)) { + if (!Objects.equals(this.transferLimit, newTransferLimit)) { this.transferLimit = newTransferLimit; changes.put(TRANSFER_LIMIT_PARAM_NAME, newTransferLimit); } return changes; }🤖 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 127 - 139, In the update method of SelfBeneficiariesTPT, make the name comparison null-safe by replacing the direct this.name.equals(newName) check with Objects.equals(this.name, newName) (negated) so the method won't NPE if either side is null; keep the existing behavior of setting this.name and putting NAME_PARAM_NAME into the changes map when a real change occurs, and leave the existing transferLimit comparison logic unchanged. Ensure you import java.util.Objects if not already imported.
🤖 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/resources/db/changelog/tenant/module/selfservice/parts/007-grant-selfservice-beneficiary-tpt-permissions.xml`:
- Around line 21-33: Add a new migration run before the 007 file that inserts
the four missing permission records into m_permission (codes
CREATE_SSBENEFICIARYTPT, UPDATE_SSBENEFICIARYTPT, DELETE_SSBENEFICIARYTPT,
READ_SSBENEFICIARYTPT) so the subsequent INSERT in
007-grant-selfservice-beneficiary-tpt-permissions.xml can find them; follow the
same INSERT pattern and columns used in the existing
005-grant-selfservice-report-read-permissions.xml migration (populate code,
name/description, module/context fields and created metadata) and ensure the new
migration is idempotent (skip if permission code already exists).
In
`@src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java`:
- Around line 161-164: The code calls rs.next() unguarded after
ps.executeQuery() and then reads rs.getLong(1) into appUserId; change the block
that opens ResultSet rs from ps.executeQuery() so you check the boolean return
of rs.next() and handle the no-row case (e.g., throw a descriptive
assertion/exception or fail the test) before calling rs.getLong(1) to avoid
unclear exceptions — update the ResultSet handling in
SelfBeneficiaryTPTIntegrationTest where ps.executeQuery(), rs.next(), and
appUserId are used.
In
`@src/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java`:
- Around line 103-113: The entrypoint string in the
withCreateContainerCmdModifier block is placing JVM system properties after the
main class so they become application args; update the cmd.withEntrypoint
invocation (the lambda that builds the "sh -c" command) so that all -D JVM flags
(-Duser.home=..., -Dfile.encoding=..., -Duser.timezone=...,
-Djava.security.egd=...) appear before the main class name
org.apache.fineract.ServerApplication (i.e., include them in the java command
directly after $JAVA_TOOL_OPTIONS and before
org.apache.fineract.ServerApplication) and keep cmd.withCmd() as-is.
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java`:
- Line 175: Fix the typo in the log message inside
SelfBeneficiariesTPTWritePlatformServiceImpl where the error is logged; change
the log.error call that currently uses "Error occured." to use the correct
spelling "Error occurred." (keep passing the same exception variable, e.g.,
dae).
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java`:
- Around line 127-139: In the update method of SelfBeneficiariesTPT, make the
name comparison null-safe by replacing the direct this.name.equals(newName)
check with Objects.equals(this.name, newName) (negated) so the method won't NPE
if either side is null; keep the existing behavior of setting this.name and
putting NAME_PARAM_NAME into the changes map when a real change occurs, and
leave the existing transferLimit comparison logic unchanged. Ensure you import
java.util.Objects if not already imported.
In
`@src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContext.java`:
- Around line 112-120: The reflection-based injection in setId(AppUser stub,
Long id) relies on the superclass having a field named "id", which is fragile;
update the setId method to include a clear comment above it explaining why
reflection is used, the dependency on the superclass field name "id", and the
risk if the superclass changes, and add a small unit test (targeting
SelfServiceCompatibleSecurityContext.setId behavior) that asserts the superclass
declares a field named "id" (failing fast if the name changes) so future
refactors surface the breakage early; reference the setId method, AppUser class,
and the id field in both the comment and the test.
In
`@src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java`:
- Line 38: The test class SelfBeneficiaryTPTIntegrationTest violates the
project's IT naming convention; rename the class
SelfBeneficiaryTPTIntegrationTest to SelfBeneficiaryTPTIT (update the class
declaration and the Java filename to match) and update any references/imports or
test suite entries that instantiate or refer to
SelfBeneficiaryTPTIntegrationTest to the new SelfBeneficiaryTPTIT name so the
build and test discovery continue to work.
- Around line 155-156: The test inserts a user by copying the password hash from
the existing m_appuser row for username 'mifos' (the SQL VALUES string in
SelfBeneficiaryTPTIntegrationTest) but later authenticates with the plaintext
"password", which implicitly relies on the default mifos password; add a concise
comment next to that SQL/insert (in SelfBeneficiaryTPTIntegrationTest) stating
this assumption (that the test depends on the default 'mifos' password being
"password") and, optionally, note that a more robust fix would be to set a known
password explicitly or derive it from the test seed.
- Around line 229-234: Replace the JUnit Assertions calls in
SelfBeneficiaryTPTIntegrationTest (the assertions on response.statusCode() and
resourceId) with AssertJ fluent assertions to match the project's style: assert
the status code with AssertJ's assertThat(response.statusCode()).isEqualTo(200)
(include a failure message via withFailMessage or as a description if needed)
and assert the resourceId with assertThat(resourceId).isNotNull(); locate the
two statements that use Assertions.assertEquals and Assertions.assertNotNull and
switch them to the corresponding AssertJ methods.
🪄 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: d5bb847b-d9cd-4eed-a22d-f722d1aa7d94
📒 Files selected for processing (8)
src/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/security/service/SelfServiceCompatibleSecurityContext.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/007-grant-selfservice-beneficiary-tpt-permissions.xmlsrc/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContextTest.javasrc/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java
DeathGun44
force-pushed
the
MX-233-fix-beneficiary-classcast-exception
branch
from
April 11, 2026 08:52
0ed2d9f to
722f376
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java (1)
43-208: Extract the seeding flow into smaller helpers.This setup method currently provisions client/product/account state, resolves the role, seeds JDBC rows, and verifies authentication in one block. Splitting it into focused helpers would make failures easier to localize and the test setup easier to maintain.
As per coding guidelines, "Keep methods concise (<20 lines)." and "Extract reusable logic to smaller, cohesive components."
🤖 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/SelfBeneficiaryTPTIntegrationTest.java` around lines 43 - 208, seedSelfServiceUserAndSavingsAccount is too long and does multiple responsibilities; extract the logic into small helper methods to improve readability and testability. Create helpers such as createClient (builds clientBody and POSTs to /api/v1/clients returning clientId), createSavingsProduct (builds savingsProductBody, posts to /api/v1/savingsproducts returning productId), createAndActivateSavingsAccount (takes clientId+productId, posts to /api/v1/savingsaccounts, approves and activates the account, and returns savingsId and accountNumber), resolveRoleId (wraps the /api/v1/roles call and extracts SelfServiceApiConstants.SELF_SERVICE_USER_ROLE id), seedDbUserAndRoles (moves all JDBC INSERT logic that inserts into m_appuser, m_appuser_role, m_appselfservice_user, m_appselfservice_user_role and m_selfservice_user_client_mapping and returns the username), and authenticateSelfServiceUser (posts to SELF_AUTH_PATH to verify login). Replace the big method by orchestrating these helpers in seedSelfServiceUserAndSavingsAccount to keep each helper <20 lines and focused on one responsibility.
🤖 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/SelfBeneficiaryTPTIntegrationTest.java`:
- Around line 134-143: The /api/v1/roles call can return non-200 which currently
leads to a misleading "Could not resolve role id..." error; before reading
rolesResponse.jsonPath() add an explicit check of the HTTP status (e.g., assert
or if-check that rolesResponse.statusCode() == 200 or use
rolesResponse.then().statusCode(200)) and if it is not 200 throw/Assert with a
message that includes the actual status and response body to surface the real
failure; update the block around rolesResponse, roleId and the
SelfServiceTestUtils.requestSpecWithAuth call and keep reference to
SelfServiceApiConstants.SELF_SERVICE_USER_ROLE when parsing the id.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java`:
- Around line 43-208: seedSelfServiceUserAndSavingsAccount is too long and does
multiple responsibilities; extract the logic into small helper methods to
improve readability and testability. Create helpers such as createClient (builds
clientBody and POSTs to /api/v1/clients returning clientId),
createSavingsProduct (builds savingsProductBody, posts to
/api/v1/savingsproducts returning productId), createAndActivateSavingsAccount
(takes clientId+productId, posts to /api/v1/savingsaccounts, approves and
activates the account, and returns savingsId and accountNumber), resolveRoleId
(wraps the /api/v1/roles call and extracts
SelfServiceApiConstants.SELF_SERVICE_USER_ROLE id), seedDbUserAndRoles (moves
all JDBC INSERT logic that inserts into m_appuser, m_appuser_role,
m_appselfservice_user, m_appselfservice_user_role and
m_selfservice_user_client_mapping and returns the username), and
authenticateSelfServiceUser (posts to SELF_AUTH_PATH to verify login). Replace
the big method by orchestrating these helpers in
seedSelfServiceUserAndSavingsAccount to keep each helper <20 lines and focused
on one responsibility.
🪄 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: f620fa08-f676-41cf-b56c-663c0730c1ae
📒 Files selected for processing (8)
src/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/security/service/SelfServiceCompatibleSecurityContext.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/007-grant-selfservice-beneficiary-tpt-permissions.xmlsrc/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContextTest.javasrc/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java
- src/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xml
- src/main/resources/db/changelog/tenant/module/selfservice/parts/007-grant-selfservice-beneficiary-tpt-permissions.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContext.java
- src/test/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContextTest.java
- src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
DeathGun44
force-pushed
the
MX-233-fix-beneficiary-classcast-exception
branch
3 times, most recently
from
April 11, 2026 09:16
a7e49f4 to
75188bb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.java (3)
258-283: Tighten the update and delete assertions.These tests currently stop at the final
200status. Please fail fast on a missingresourceIdfrom the setup POST, and add a follow-up assertion that verifies the beneficiary was actually updated or removed. That will make these regressions much more diagnostic.As per coding guidelines, Verify both happy path and edge cases.
Also applies to: 301-321
🤖 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/SelfBeneficiaryTPTIntegrationTest.java` around lines 258 - 283, Ensure the POST that creates the beneficiary actually returned a resourceId and fail fast if missing (check the extracted beneficiaryId from the POST to BENIFICIARIES_PATH and assert it's non-null/positive), then after the PUT to BENEFICIARIES_PATH/{beneficiaryId} validate the update by fetching the beneficiary (GET BENEFICIARIES_PATH/{beneficiaryId}) or by asserting returned/updateResponse body contains the updated fields (e.g., name == "Updated Name" and transferLimit == 1000); similarly, for delete tests assert the DELETE returned success and then attempt a GET to verify the beneficiary is gone (expect 404 or appropriate not-found response).
35-38: Rename the integration test class to match the project convention.
SelfBeneficiaryTPTIntegrationTestdoes not follow the repository naming rule for integration tests. Please rename it toSelfBeneficiaryTPTITorSelfBeneficiaryTPTTestfor consistency.As per coding guidelines, Name tests
ClassNameTestorClassNameIT(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/SelfBeneficiaryTPTIntegrationTest.java` around lines 35 - 38, The test class SelfBeneficiaryTPTIntegrationTest should be renamed to follow project conventions; change the class declaration SelfBeneficiaryTPTIntegrationTest to either SelfBeneficiaryTPTIT or SelfBeneficiaryTPTTest, update the Java file name to match the new class name, and update any references/imports/usages (e.g., in test suites, build configs, or other classes) to the new symbol so compilation and test discovery continue to work.
43-208: Split the seeding helper into smaller steps.
seedSelfServiceUserAndSavingsAccount()is doing provisioning, role lookup, direct JDBC seeding, and authentication in one block. Extracting those into focused helpers will make failures easier to diagnose and keep future test changes localized.As per coding guidelines, Keep methods concise (<20 lines).
🤖 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/SelfBeneficiaryTPTIntegrationTest.java` around lines 43 - 208, seedSelfServiceUserAndSavingsAccount() is too large—split it into focused helpers: extract the client/product/savings provisioning into a method like seedClientAndSavingsAccount() that returns clientId, productId, savingsId and accountNumber; extract the JDBC user creation block into createSelfServiceDbUser(username) which inserts into m_appuser/m_appuser_role and returns appUserId; extract self-service-specific inserts into assignSelfServiceRoleAndMappings(appUserId, roleId, clientId); and extract the auth POST into authenticateSelfServiceUser(username). Replace the big method with a high-level flow that calls seedClientAndSavingsAccount(), resolves roleId, calls createSelfServiceDbUser(), assignSelfServiceRoleAndMappings(), then authenticateSelfServiceUser(); keep each new helper focused (<20 lines), reuse existing SQL/REST logic (refer to seedSelfServiceUserAndSavingsAccount, getFineractPort, SelfServiceTestUtils.SELF_AUTH_PATH, and role lookup code) and preserve existing exception handling.
🤖 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/account/api/SelfBeneficiaryTPTIntegrationTest.java`:
- Around line 258-283: Ensure the POST that creates the beneficiary actually
returned a resourceId and fail fast if missing (check the extracted
beneficiaryId from the POST to BENIFICIARIES_PATH and assert it's
non-null/positive), then after the PUT to BENEFICIARIES_PATH/{beneficiaryId}
validate the update by fetching the beneficiary (GET
BENEFICIARIES_PATH/{beneficiaryId}) or by asserting returned/updateResponse body
contains the updated fields (e.g., name == "Updated Name" and transferLimit ==
1000); similarly, for delete tests assert the DELETE returned success and then
attempt a GET to verify the beneficiary is gone (expect 404 or appropriate
not-found response).
- Around line 35-38: The test class SelfBeneficiaryTPTIntegrationTest should be
renamed to follow project conventions; change the class declaration
SelfBeneficiaryTPTIntegrationTest to either SelfBeneficiaryTPTIT or
SelfBeneficiaryTPTTest, update the Java file name to match the new class name,
and update any references/imports/usages (e.g., in test suites, build configs,
or other classes) to the new symbol so compilation and test discovery continue
to work.
- Around line 43-208: seedSelfServiceUserAndSavingsAccount() is too large—split
it into focused helpers: extract the client/product/savings provisioning into a
method like seedClientAndSavingsAccount() that returns clientId, productId,
savingsId and accountNumber; extract the JDBC user creation block into
createSelfServiceDbUser(username) which inserts into m_appuser/m_appuser_role
and returns appUserId; extract self-service-specific inserts into
assignSelfServiceRoleAndMappings(appUserId, roleId, clientId); and extract the
auth POST into authenticateSelfServiceUser(username). Replace the big method
with a high-level flow that calls seedClientAndSavingsAccount(), resolves
roleId, calls createSelfServiceDbUser(), assignSelfServiceRoleAndMappings(),
then authenticateSelfServiceUser(); keep each new helper focused (<20 lines),
reuse existing SQL/REST logic (refer to seedSelfServiceUserAndSavingsAccount,
getFineractPort, SelfServiceTestUtils.SELF_AUTH_PATH, and role lookup code) and
preserve existing exception handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 414fd8df-9681-4090-b7bc-1bfb3daef128
📒 Files selected for processing (8)
src/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/security/service/SelfServiceCompatibleSecurityContext.javasrc/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xmlsrc/main/resources/db/changelog/tenant/module/selfservice/parts/007-grant-selfservice-beneficiary-tpt-permissions.xmlsrc/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/security/service/SelfServiceCompatibleSecurityContextTest.javasrc/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/db/changelog/tenant/module/selfservice/module-changelog-master.xml
- src/main/resources/db/changelog/tenant/module/selfservice/parts/007-grant-selfservice-beneficiary-tpt-permissions.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
- src/main/java/org/apache/fineract/selfservice/account/service/SelfBeneficiariesTPTWritePlatformServiceImpl.java
DeathGun44
force-pushed
the
MX-233-fix-beneficiary-classcast-exception
branch
from
April 11, 2026 09:28
75188bb to
a63b578
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three bugs that together made the TPT beneficiary feature completely
unusable for self-service users.
authenticatedUser(CommandWrapper)overrideto
SelfServiceCompatibleSecurityContext; preventsAppSelfServiceUserfromfalling through to the base-class hard cast.
SelfBeneficiariesTPTasAbstractPersistableCustom<Long>; eliminatesString → Longcast on persist.007grantsCREATE/UPDATE/DELETE_SSBENEFICIARYTPTto the Self Service User role; previously 403 on every write.
Verified by 3 new E2E tests (POST / PUT / DELETE) against a real Fineract +
PostgreSQL 15 stack via Testcontainers. All 23 tests pass.
Summary by CodeRabbit
New Features
Security
Database
Tests