[Eyad-Sharkawy]

Description

Resolves FINERACT-848.

Adds the ability to override product-level interest rate charts with a custom nominal annual interest rate for Fixed Deposit (FD) and Recurring Deposit (RD) accounts:

• Added isRateChartOverridden boolean flag to deposit account terms and data models.
• Updated domain interest calculation and validation in FixedDepositAccount and RecurringDepositAccount so that when isRateChartOverridden is enabled, custom nominal annual interest rate takes precedence and bypasses product chart slab lookups.
• Added API request/response parameters, Swagger documentation annotations, and Liquibase database migration.
• Added comprehensive unit tests in DepositAccountRateChartOverriddenTest.

Checklist

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green")
• Create/update unit or integration tests for verifying the changes made
• Follow our coding conventions
• Add required Swagger annotation and update API documentation
• This PR must not be a "code dump"
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately

[Eyad-Sharkawy]

Hi @Aman-Mittal / reviewers, all 86 test and quality checks passed, but the regression-safety check was cancelled on the baseline step (Instance #1: The operation was canceled). Could someone with write permissions please re-run that failed job? Thank you

[Eyad-Sharkawy]

I saw the announcement regarding FINERACT-2621, so I went ahead and rebased this PR to ensure full compatibility with the new Spring Boot 4.1 upgrade.

[Eyad-Sharkawy]

Hi @adamsaghy / @Aman-Mittal, I know the core team has been laser-focused on shipping the Working Capital epic recently! I just resolved a few minor Swagger conflicts to keep this branch perfectly up-to-date with develop. Since this is a larger domain feature (FINERACT-848), let me know if there is anything I can do to make the review process easier once the team has bandwidth to look at the general queue again.

[adamsaghy]

Hi @adamsaghy / @Aman-Mittal, I know the core team has been laser-focused on shipping the Working Capital epic recently! I just resolved a few minor Swagger conflicts to keep this branch perfectly up-to-date with develop. Since this is a larger domain feature (FINERACT-848), let me know if there is anything I can do to make the review process easier once the team has bandwidth to look at the general queue again.

Sorry for being slow on PR review. I am facing an increased workload at the moment, but will try to review your PR ASAP.

[adamsaghy]

@Eyad-Sharkawy Can you please review the below concerns or mine?

Gap?
The penalty keeps being subtracted from the stored rate

To handle the override, the PR moved the premature-closure penalty outside the chart != null block. It still ends with this.nominalAnnualInterestRate = applicableInterestRate; (FixedDepositAccount.java:193, RecurringDepositAccount.java:245).

• Before this PR: the rate was re-read from the chart on every call, so writing the penalised rate back did no harm.
• With the override on: the stored rate is the only source of the rate, so each call lowers it by the penalty again.

This happens in a real API flow:

1. POST /fixeddepositaccounts/{id}?command=calculatePrematureAmount (the preview) runs DepositAccountPreMatureCalculationPlatformServiceImpl.calculatePreMatureAmount. That method has a plain read-write @transactional.
2. The call goes through calculatePreMatureInterest → calculateInterestPayable → getEffectiveInterestRateAsFraction(…, true). That changes the rate on a live database entity, and the change is saved when the transaction commits.
3. Each preview lowers the stored rate by the penalty: with a 7.5% custom rate and a 1% penalty, it goes 7.5 → 6.5 → 5.5 and so on, until it reaches 0.
4. The real prematureClose then subtracts the penalty once more from whatever is left, so the interest posted is lower than it should be.

The RD endpoint works the same way. I traced this through the code but didn't run it. It's easy to demonstrate: calling the method from testFixedDepositEffectiveInterestRateWithOverriddenRateAndPrematureClosurePenalty a second time returns 0.055 instead of 0.065.

Suggested fix?

Don't write the penalised rate back to nominalAnnualInterestRate when the override is on. Alternatively, calculate the penalised rate in a local variable and leave the stored custom rate alone. -> Is this correct?

Change not mentioned in the description

The same move means FD accounts with no chart at all now get the premature-closure penalty. That happens when the product had no applicable chart (DepositAccountAssembler.java:308-313); before this PR, those accounts had no penalty. These accounts also get the drift bug above. This may be intended, but the description only talks about overridden accounts, so the author should either confirm it or keep the old behaviour.

[Eyad-Sharkawy]

Thank you @adamsaghy for catching this! You are completely right on both counts:

Premature Closure Rate Drift

When isRateChartOverridden is true, overwriting this.nominalAnnualInterestRate with applicableInterestRate after applying applyPreMaturePenalty caused each preview/calculation call (calculatePrematureAmount) to permanently mutate and lower the stored contract rate.

Fix Applied

• In both FixedDepositAccount.java and RecurringDepositAccount.java, writing the penalised rate back to this.nominalAnnualInterestRate is now explicitly guarded:

  if (!this.accountTermAndPreClosure.isRateChartOverridden()) {
      this.nominalAnnualInterestRate = applicableInterestRate;
  }

• For overridden accounts (isRateChartOverridden == true): The premature penalty is calculated locally and returned as the effective rate fraction, keeping the persistent contract rate completely untouched and eliminating rate drift.
• For standard accounts (isRateChartOverridden == false): The exact legacy behavior is preserved without regression.

Unit Tests

• Extended DepositAccountRateChartOverriddenTest to execute getEffectiveInterestRateAsFraction(..., isPreMatureClosure=true) multiple times sequentially, verifying that nominalAnnualInterestRate remains unaltered and the effective rate calculation is strictly idempotent.

[Eyad-Sharkawy]

Hi @adamsaghy, just a quick update: all 90 CI checks are now passing, and all previous review feedback has been addressed. Ready for your final review whenever you have a chance. Thank you!

[galovics]

Hi @Eyad-Sharkawy, thanks for the PR. The direction is fine and the unit tests are good, but the FD refactor changes behaviour for accounts that do not use the feature.

• FixedDepositAccount.getEffectiveInterestRateAsFraction: the pre-closure penalty block and the this.nominalAnnualInterestRate = applicableInterestRate assignment used to live inside if (this.chart != null). Now the penalty runs when chart == null too, so a non-overridden FD with no chart and a premature closure gets rate minus penalty, and the lowered rate is written back onto the entity, so calling it again lowers it a second time. Your idempotency test covers only the overridden case. Why not keep the original structure and just add && !isRateChartOverridden() to the existing condition? If the new structure is intended, pls add a test for no-chart + not-overridden + premature closure.
• DepositAccountDataValidator (~580 and ~620): the two blocks are identical, extract a helper. On update, isRateChartOverridden=true requires nominalAnnualInterestRate in the same payload, so toggling the flag on an account that already has a rate is rejected - fall back to the stored value? zeroOrPositiveAmount() lets 0 through and validateDomainRules rejects it later from another layer.
• Toggling the flag back to false leaves the custom rate in nominalAnnualInterestRate until the next recalculation. Intended? Please cover it with a test.
• Boolean isRateChartOverridden = false + parameterExists in the assembler is one call with booleanPrimitiveValueOfParameterNamed, and createNew / the constructor should take a primitive. The flag is threaded through 8+ instance / withTemplate factories just to default to false - do the data classes need it at all?
• Revert the reformat of DEPOSIT_ACCOUNT_REQUEST_DATA_PARAMETERS (hides the one-line change) and check the response-data sets and template include the new param.
• No integration test hits the API: create an FD/RD with isRateChartOverridden=true, check interest and the GET, toggle on update. The DB column, mapper SQL and JSON parsing are not exercised. The 406-line test repeats its setup.

[Eyad-Sharkawy]

Hi @galovics, thank you for the detailed and constructive review! I have addressed all points:

1. Idempotency & calculation drift:

   • Restored the original calculation structure in FixedDepositAccount and RecurringDepositAccount so chart lookups, premature closure penalties, and this.nominalAnnualInterestRate assignments only run inside if (this.chart != null && !this.accountTermAndPreClosure.isRateChartOverridden()). Overridden accounts return the custom nominal rate directly without entity mutation.
   • Added unit tests covering the non-overridden no-chart scenario with premature closure to verify rate idempotency.

2. Validation deduplication & behavior:

   • Extracted shared helper method validateRateChartOverride(...) in DepositAccountDataValidator.
   • Changed .zeroOrPositiveAmount() to .positiveAmount() to reject 0 upfront.
   • Allowed updates where isRateChartOverridden = true without re-submitting nominalAnnualInterestRate (falling back to existing rate).

3. Domain model & Assembler:

   • Simplified reading of isRateChartOverridden using command.booleanPrimitiveValueOfParameterNamed(isRateChartOverriddenParamName).
   • Changed isRateChartOverridden parameter in DepositAccountTermAndPreClosure constructor and createNew(...) factory to primitive boolean.

4. DepositsApiConstants & Swagger:

   • Preserved original multiline formatting of DEPOSIT_ACCOUNT_REQUEST_DATA_PARAMETERS in DepositsApiConstants.
   • Added Swagger annotations for isRateChartOverridden and nominalAnnualInterestRate on PUT requests for both FD and RD resources.

5. Unit test refactoring:

   • Refactored DepositAccountRateChartOverriddenTest to eliminate repetitive account/term setup across tests.
   • Added tests covering toggling isRateChartOverridden back to false, no-chart premature closure idempotency, and zero-rate rejection.

6. Integration test:

   • Added an end-to-end integration test testFixedDepositAccountWithRateChartOverriddenLifecycle() in FixedDepositTest exercising account creation with isRateChartOverridden = true, verification via GET, and updating to toggle the flag back to false.