Skip to content

FINERACT-2455: Fix e2e execution times of DelinquencyPause and GoodwillCreditPaymentAllocation features - #6497

Open
Cocoa-Puffs wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455-optimize-e2e-delinquency-pause-goodwill-credit-features
Open

Cocoa-Puffs wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455-optimize-e2e-delinquency-pause-goodwill-credit-features

Conversation

@Cocoa-Puffs

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.
  • I followed the AI Policy.

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

@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.

Test-only, and I recomputed the pause and goodwill schedules against the old logic - they match. Each delinquency pause scenario now creates its own 3-day bucket and product (both steps already exist and WorkingCapitalDelinquencyReschedule.feature uses the same pattern), and the period end is still start + 3 + the inclusive pause days that overlap it, with delinquent days still counted without subtracting pauses. The goodwill scenarios keep the transaction ordering that matters (UC19's goodwill is still before the charge due date so the in-advance buckets are exercised; UC17's is still backdated relative to the repayment). All dates are business dates, no wall-clock or timezone dependence, and the step defs are backward compatible. The remaining points are about coverage that got lost in the rescale.

UC6.4 tests less than before (WorkingCapitalDelinquencyPause.feature ~L281-305). The old scenario had an existing 15 Jan-25 Feb pause plus a backdated 05-10 Jan one, and rejected 05-14 Jan (partial overlap with only the backdated pause) and 02-14 Jan (full containment, ending before the first pause). Now the existing pause is 03-06 and the backdated one 01-02, so the first negative case is an exact duplicate of the 01-02 pause and the second (02-03) overlaps both pauses - neither still proves that a pause overlapping only the backdated one is rejected. Something like business date 03 Jan, first pause 05-08, backdated 01-02, then expect errors for 02-03 and 01-04 keeps both cases. UC9/UC6.3 still cover some partial overlap, so it's a small loss.

Smaller coverage changes: UC6.1's future pause used to start exactly on the first day of period 3 (04-18) and now starts a day into it (14 Jan vs a 01-13 period start; 13-15 Jan would keep the boundary), UC8's two pauses are now back to back with no gap, and UC5's pause now runs past period 2's end rather than sitting inside it. Also product.name was dropped from the account-data tables (random name), so nothing now checks the loan is linked to the new 3-day product beyond the schedule rows.

Merge order: this rewrites the same UC3 loan tables and the ~14 goodwill "full repayment" lines that #6446 and #6398 also change (to "all obligations met"), so whichever lands second needs a manual rebase - worth agreeing the order. The PR description has no timings for the speed-up.

Recommendation: COMMENT

@Cocoa-Puffs
Cocoa-Puffs force-pushed the FINERACT-2455-optimize-e2e-delinquency-pause-goodwill-credit-features branch from 7be699a to 695a763 Compare September 24, 2026 10:31
@Cocoa-Puffs
Cocoa-Puffs force-pushed the FINERACT-2455-optimize-e2e-delinquency-pause-goodwill-credit-features branch from 695a763 to de074c9 Compare September 24, 2026 11:31
@Cocoa-Puffs

Copy link
Copy Markdown
Contributor Author

All four coverage points are fair, and I've fixed them. The product.name one I'd push back on, reasoning at the bottom.

UC6.4 — restored both negative cases (the real loss)

Adopted the reviewer's layout: business date 03 Jan, first pause 05–08, backdated pause 01–02, errors expected for 02–03 and 01–04.

That gets the two distinct cases back:

candidate why it's rejected case it covers
02-03 shares only day 02 with the backdated pause partial overlap with only the backdated pause
01-04 contains 01-02, stops at 04 full containment, ending before the first pause

The thing that makes this clean rather than ambiguous is worth stating, since the file also has a scenario asserting "touching consecutive pauses are rejected". I checked the validator and overlap needs a shared day, not adjacency.

return !parsedPauseStart.isAfter(existingPauseEnd) && !parsedPauseEnd.isBefore(existingPauseStart);

So 01–04 against a pause at 05–08 is not an overlap, and its rejection can only come from containing the backdated pause. (The old scenario was actually weaker here — its 02–14 butted straight against a pause starting 15 Jan, so it had the ambiguity this version doesn't.)

I also moved the schedule assertion from after the first pause to after the backdated one, where it now shows something: period 1 goes 01–03 → 01–05, i.e. base 3 days plus the 2 inclusive pause days.

UC6.1 / UC8 / UC5

  change effect on assertions
UC6.1 future pause 14-16 → 13-15 none — period 3 is 13-18 either way (both are 3 inclusive days). Now starts exactly on period 3's first day, as the old 18 Apr pause did on its 18 Apr - 25 May period.
UC8 second pause 04-05 → 05-06, business date 04 → 05 none — period 1 still 01-03 → 01-05 → 01-07. Restores a clear gap day (04) between the two pauses.
UC5 pause 05-07 → 05-05 period 2 04-09 → 04-07

UC5 needs a caveat. With 3-day periods, period 2 is 04-06, so the only range strictly inside it is a single day. A one-day pause is legal (validateStartBeforeEnd rejects only start > end) and it does sit in the middle (04 < 05 < 06), so the scenario title still holds, but it is an unusual shape. If you'd rather have a multi-day interior pause, UC5 needs a bucket wider than the 3-day guideline. Happy to give it its own if you prefer that, however I believe that while the shape is not common, it nevertheless accurately tests the overlapping pause validations it's supposed to.

product.name — I think the linkage is already covered

Real concern, but I'd argue it's checked structurally rather than by name. If WCLP_DELINQUENCY resolved to the wrong product, the next assertion in every affected scenario fails:

periodNumber fromDate toDate
1 2026-01-01 2026-01-03

A 01-03 first period can only come from a 3-day bucket. The stock WCLP product gives 01-01 .. 01-30 — which is exactly what the pre-change baseline asserted — and a product with no bucket produces no schedule rows at all. So a mislinked loan can't reach the end of any of these scenarios.

The period boundaries are also a stronger check than the name was, because unlike the name they aren't random. That's the reason the column went: each scenario now builds its own bucket and product, so the name is a fresh DB-WCL-random every run and can't be asserted. This could be done by touching the shared stepdefs and creating a version which stores the created product name in the test context, but I would rather not touch them unless you feel strongly.

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