Skip to content

FINERACT-2455: added e2e test scenario for working capital discount fee amortization on excess repayment - #6265

Open
oleksii-novikov-onix wants to merge 2 commits into
apache:developfrom
openMF:FINERACT-2455/wc-discount-fee-amortization-excess-payment
Open

FINERACT-2455: added e2e test scenario for working capital discount fee amortization on excess repayment#6265
oleksii-novikov-onix wants to merge 2 commits into
apache:developfrom
openMF:FINERACT-2455/wc-discount-fee-amortization-excess-payment

Conversation

@oleksii-novikov-onix

Copy link
Copy Markdown
Contributor

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

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.
  • If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

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

@oleksii-novikov-onix
oleksii-novikov-onix marked this pull request as ready for review August 13, 2026 12:25

@galovics galovics left a comment

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.

The arithmetic in all four scenarios is genuinely correct - I checked the amortization amounts against the discount-fee service and they sum to the discount fee in every case, with the intermediate realized/unrealized checkpoints lining up with the schedule rows.

The problem is that two of the new scenarios don't actually assert what they claim to. The step The retrieved amortization schedule has payments with the following details for the listed payment numbers: only reads five known columns (date, expectedPaymentAmount, expectedBalance, expectedAmortizationAmount, expectedDiscountFeeBalance) - it has no branch for actualPaymentAmount, actualAmortizationAmount, actualBalance, or actualDiscountFeeBalance. Unknown table columns are just silently not read, so no assertion is produced for them. C98177's row-1 values (50.00 / 9.61 / 990.39, which is the entire point of the "repayment above the daily amount" scenario) are never checked, and C98178 loses 4 of its 8 columns the same way - only 3 of them actually get asserted. Either extend the step to handle the actual* columns, or make it fail fast on an unrecognized header so this can't happen silently again.

Also worth reworking: the new "consistency" invariant step (sum(actualAmortizationAmount) == realizedIncomeFromDiscountFee) only holds because all four scenarios happen to repay every single day - it isn't a general invariant, and as written it can't be reused on a scenario with a payment gap, a lump-sum payoff, or a reversal without going red for reasons unrelated to the thing under test. That's specifically the shape of coverage that would help catch the class of bug found on #6281 (final discount fee amortization on charge-off/reversal) - worth generalizing rather than leaving scoped to the daily-repayment case.

Smaller things: the PR currently shows as mergeable_state: dirty against develop with no CI checks reported - needs a rebase before it can be evaluated properly. Two commits from two different authors should probably be squashed per CONTRIBUTING. C98183 substantially duplicates C98177's setup (same product, same amounts, same day-1 repayment) just to add a journal-entry check - consider folding that into C98177 instead of booting a second identical loan.

Recommendation: CHANGES_REQUESTED

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