MX-238: fix integration test auth failures - #140
Merged
IOhacker merged 1 commit intoApr 13, 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 44 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 (7)
📝 WalkthroughWalkthroughThe pull request adds transactional control to a password reset service method and refactors test infrastructure to replace direct JDBC operations with SQL execution via Postgres container utilities. Multiple test classes are updated to use the new SQL execution helpers instead of manual connection and statement management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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: 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/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.java (1)
101-157:⚠️ Potential issue | 🟠 MajorRollback intent is neutralized by swallowed delivery exceptions
@Transactional(rollbackFor = Exception.class)oncreateForgotPasswordRequestwill not roll back when token delivery fails, becausetrySendAuthorizationTokencatchesRuntimeExceptionand returns normally (Lines 299-304). This still allows persisted reset requests without successful delivery.Proposed fix
private void trySendAuthorizationToken(SelfServiceRegistration selfServiceRegistration, boolean isEmailAuthenticationMode) { try { sendAuthorizationToken(selfServiceRegistration, isEmailAuthenticationMode); } catch (RuntimeException e) { log.error("Failed to deliver self-service {} token for request {}", selfServiceRegistration.getRequestType(), selfServiceRegistration.getId(), e); - } + throw e; + } }🤖 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/SelfServiceForgotPasswordWritePlatformServiceImpl.java` around lines 101 - 157, The transaction currently persists the reset request before calling trySendAuthorizationToken which swallows delivery exceptions, so failures don't trigger rollback; change the flow in createForgotPasswordRequest to ensure delivery failures rollback by either (A) allowing trySendAuthorizationToken to throw instead of catching RuntimeException (remove or rethrow inside trySendAuthorizationToken), or (B) catch exceptions in createForgotPasswordRequest after calling trySendAuthorizationToken, call selfServiceRegistrationRepository.delete(request) (or deleteById) and rethrow the exception so the `@Transactional` on createForgotPasswordRequest can roll back; update references to trySendAuthorizationToken, createForgotPasswordRequest, selfServiceRegistrationRepository.saveAndFlush and selfServiceRegistrationRepository.delete accordingly.
🧹 Nitpick comments (1)
src/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java (1)
125-179: Consider extracting the seed templates into higher-level helpers.The
m_appselfservice_user/ role / client-mapping CTE now exists with small variations in five different test classes. A column change in that fixture will require touching multiple tests again, so this base class is a good place for helpers for the two patterns used in this PR: read-only self-service user seeding, and audited self-service user seeding with a pairedm_appuser.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/testing/support/SelfServiceIntegrationTestBase.java` around lines 125 - 179, The tests duplicate nearly identical SQL CTE fixtures; add two reusable helpers in SelfServiceIntegrationTestBase (e.g., methods seedReadOnlySelfServiceUser(...) and seedAuditedSelfServiceUser(...)) that build and execute the common SQL using existing helpers sqlLiteral and executeSqlInPostgres (or return the SQL string for callers to use with execPsql/querySingleValueInPostgres), parameterize differing columns/values, and replace the five inline fixtures in test classes with calls to these helpers so future schema/column changes are fixed in one place.
🤖 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/testing/support/SelfServiceIntegrationTestBase.java`:
- Around line 130-145: The helper executeSqlInPostgres currently passes
multi-statement SQL straight to execPsql which can leave partial state on
mid-script failures; wrap the supplied SQL in an explicit transaction by
prefixing "BEGIN;" and suffixing "COMMIT;" (i.e. execute "BEGIN; <your-sql>;
COMMIT;") so the entire script runs atomically via execPsql, and let the
existing error handling surface the failure (psql will abort the transaction on
error); update the executeSqlInPostgres implementation (referencing
executeSqlInPostgres and execPsql) to perform this wrapping so callers such as
SelfAccountTransferTPTIntegrationTest and SelfBeneficiaryTPTIntegrationTest need
no changes.
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.java`:
- Around line 101-157: The transaction currently persists the reset request
before calling trySendAuthorizationToken which swallows delivery exceptions, so
failures don't trigger rollback; change the flow in createForgotPasswordRequest
to ensure delivery failures rollback by either (A) allowing
trySendAuthorizationToken to throw instead of catching RuntimeException (remove
or rethrow inside trySendAuthorizationToken), or (B) catch exceptions in
createForgotPasswordRequest after calling trySendAuthorizationToken, call
selfServiceRegistrationRepository.delete(request) (or deleteById) and rethrow
the exception so the `@Transactional` on createForgotPasswordRequest can roll
back; update references to trySendAuthorizationToken,
createForgotPasswordRequest, selfServiceRegistrationRepository.saveAndFlush and
selfServiceRegistrationRepository.delete accordingly.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java`:
- Around line 125-179: The tests duplicate nearly identical SQL CTE fixtures;
add two reusable helpers in SelfServiceIntegrationTestBase (e.g., methods
seedReadOnlySelfServiceUser(...) and seedAuditedSelfServiceUser(...)) that build
and execute the common SQL using existing helpers sqlLiteral and
executeSqlInPostgres (or return the SQL string for callers to use with
execPsql/querySingleValueInPostgres), parameterize differing columns/values, and
replace the five inline fixtures in test classes with calls to these helpers so
future schema/column changes are fixed in one place.
🪄 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: df399e9b-e7a6-438f-a56c-71bac94e24ab
📒 Files selected for processing (7)
src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.javasrc/test/java/org/apache/fineract/selfservice/account/api/SelfAccountTransferTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/account/api/SelfBeneficiaryTPTIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/client/api/SelfClientsApiIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/runreport/SelfRunReportIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfServicePermissionEnforcementIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/testing/support/SelfServiceIntegrationTestBase.java
- Refactor all integration test DB seeding to use centralized executeSqlInPostgres() helpers in SelfServiceIntegrationTestBase - Beneficiary/Transfer tests: insert into both m_appuser and m_appselfservice_user (required for JPA audit entity resolution) - RunReport/PermissionEnforcement/Clients tests: insert only into m_appselfservice_user (read-only tests, no JPA audit needed) - Remove authenticateSelfUser() smoke calls; tests now use direct Basic Auth headers for consistency - Add @transactional(rollbackFor=Exception.class) to createForgotPasswordRequest to prevent orphaned records - Remove unused accountNo fetch in PermissionEnforcement test All 30 tests pass (0 failures, 0 errors).
DeathGun44
force-pushed
the
MX-238-fix-integration-test-auth
branch
from
April 13, 2026 17:46
d008a02 to
4203513
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.
Summary by CodeRabbit
Bug Fixes
Tests