Skip to content

FINERACT-2620: Add type-safe Feign-based helpers and integration tests for the Savings domain#5907

Open
DeathGun44 wants to merge 1 commit into
apache:developfrom
DeathGun44:FINERACT-2620/feign-savings-helper-and-tests
Open

FINERACT-2620: Add type-safe Feign-based helpers and integration tests for the Savings domain#5907
DeathGun44 wants to merge 1 commit into
apache:developfrom
DeathGun44:FINERACT-2620/feign-savings-helper-and-tests

Conversation

@DeathGun44
Copy link
Copy Markdown
Contributor

@DeathGun44 DeathGun44 commented May 28, 2026

Description

Adds Feign-based helpers and integration tests for the Savings domain — mirrors the existing Loan test infrastructure.

New files

File Purpose
FeignTestConstants Shared date format and locale constants across test domains
FeignSavingsProductHelper Create and retrieve savings products
FeignSavingsHelper Account lifecycle: submit, approve, activate, close, query, delete
FeignSavingsTransactionHelper Deposit and withdrawal operations
SavingsTestData Savings-specific constants (interest types, accounting rules, statuses)
SavingsRequestBuilders Typed request object factories
FeignSavingsTestBase Abstract base wiring all helpers together
FeignSavingsLifecycleExtension JUnit 5 cleanup extension with idempotent teardown
FeignSavingsLifecycleTest 8 end-to-end tests covering the full savings lifecycle

Modified files

File Change
LoanTestData Delegates DATETIME_PATTERN and LOCALE to FeignTestConstants (source-compatible)

Notes

  • Pure Feign throughout — no RestAssured or Retrofit imports
  • Cleanup extension catches all exceptions so pre-existing accounts on CI don't break test runs
  • No changes to existing legacy helpers or tests

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per our guidelines
  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
  • Create/update unit or integration tests for verifying the changes made.
  • Follow our coding conventions.
  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

@DeathGun44 DeathGun44 force-pushed the FINERACT-2620/feign-savings-helper-and-tests branch 2 times, most recently from 9c695a2 to 87078ea Compare May 29, 2026 07:22

public static final String DATETIME_PATTERN = "dd MMMM yyyy";
public static final String LOCALE = "en";
public static final String DATE_FORMAT = DATETIME_PATTERN;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we use this anywhere? What's the role of this field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it. each domain's request builders already use DATETIME_PATTERN via their own *TestData class, so the alias was unnecessary. thank you!


public static PostSavingsAccountsAccountIdRequest rejectSavings(String rejectedOnDate) {
return new PostSavingsAccountsAccountIdRequest()//
.closedOnDate(rejectedOnDate)//
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange that we use closedOnDate here, but as I checked it is probably a swagger model problem. Anyway, I don't think it's the right way to send rejectedOnDate on other parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a limitation of the generated Swagger model. PostSavingsAccountsAccountIdRequest doesn't expose a rejectedOnDate field (only activatedOnDate, approvedOnDate, closedOnDate). the legacy helper works around this by constructing raw JSON with map.put("rejectedOnDate", ...), but that defeats the purpose of type-safe migration. the server accepts closedOnDate for the reject command, and our tests pass. this is a Swagger spec gap worth filing separately so we can improve our tests .thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final JsonElement element = this.fromApiJsonHelper.parse(json);
        final LocalDate rejectedOnDate = this.fromApiJsonHelper.extractLocalDateNamed("rejectedOnDate", element);
        baseDataValidator.reset().parameter("rejectedOnDate").value(rejectedOnDate).notNull();

Are you sure server accepts it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you were right it doesn't. I traced through SavingsAccountApplicationTransitionApiJsonValidator.validateRejection() and confirmed it rejects closedOnDate as unsupported. My tests only passed because rejectSavings is called in cleanup code wrapped in a try/catch that silently logs the failure. I've filed #5934 to fix the Swagger spec and updated this branch to use .rejectedOnDate() directly. that will be the proper fix!

@DeathGun44 DeathGun44 force-pushed the FINERACT-2620/feign-savings-helper-and-tests branch from 87078ea to 50887b7 Compare May 30, 2026 05:49
this.fineractClient = fineractClient;
}

public Long submitApplication(PostSavingsAccountsRequest request) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets return the response directly, not just the savingsId.
That would improve the reusability and the data is anyway fetched and returned, we can use it ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

this.fineractClient = fineractClient;
}

public Long createSavingsProduct(PostSavingsProductsRequest request) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets return the response object, not just the resourceId.
That would help reusability and the data is fetched anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Copy Markdown
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review my concerns.

@DeathGun44 DeathGun44 force-pushed the FINERACT-2620/feign-savings-helper-and-tests branch from 50887b7 to 11b4015 Compare June 3, 2026 18:57
…s for the Savings domain

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44 DeathGun44 force-pushed the FINERACT-2620/feign-savings-helper-and-tests branch from 11b4015 to d2b2de4 Compare June 3, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants