Eyad-Sharkawy commented on PR #6405:
URL: https://github.com/apache/fineract/pull/6405#issuecomment-6272525435
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`](file:///c:/Users/eyadd/Documents/opensource/fineract/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/FixedDepositAccount.java)
and
[`RecurringDepositAccount`](file:///c:/Users/eyadd/Documents/opensource/fineract/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/RecurringDepositAccount.java)
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`](file:///c:/Users/eyadd/Documents/opensource/fineract/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/DepositAccountDataValidator.java).
- 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`](file:///c:/Users/eyadd/Documents/opensource/fineract/fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/domain/DepositAccountTermAndPreClosure.java)
constructor and `createNew(...)` factory to primitive `boolean`.
4. **DepositsApiConstants & Swagger**:
- Preserved original multiline formatting of
`DEPOSIT_ACCOUNT_REQUEST_DATA_PARAMETERS` in
[`DepositsApiConstants`](file:///c:/Users/eyadd/Documents/opensource/fineract/fineract-core/src/main/java/org/apache/fineract/portfolio/savings/DepositsApiConstants.java).
- Added Swagger annotations for `isRateChartOverridden` and
`nominalAnnualInterestRate` on PUT requests for both FD and RD resources.
5. **Unit test refactoring**:
- Refactored
[`DepositAccountRateChartOverriddenTest`](file:///c:/Users/eyadd/Documents/opensource/fineract/fineract-provider/src/test/java/org/apache/fineract/portfolio/savings/domain/DepositAccountRateChartOverriddenTest.java)
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`](file:///c:/Users/eyadd/Documents/opensource/fineract/integration-tests/src/test/java/org/apache/fineract/integrationtests/FixedDepositTest.java)
exercising account creation with `isRateChartOverridden = true`, verification
via GET, and updating to toggle the flag back to false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
