oleksii-novikov-onix commented on PR #6205: URL: https://github.com/apache/fineract/pull/6205#issuecomment-5189680747
> AC3 is not met — 8 existing TestRail scenarios had their inputs changed to dodge the new validation, no? > > Breach C85273, C85274, C85275, C85280, C85588: business date 01 June 2026 → 15 May 2026 Breach C85282, C85283: business date 10 March 2019 → 15 January 2019 Delinquency C85583: reschedule frequency 15 DAYS → 30 DAYS, with a new comment stating outright it's so the recalculated end date doesn't fall before the business date > > Each of those originally exercised "reschedule deep inside a long period", which is now precisely the AC2 error case. Rather than moving the date, keep the original inputs and flip the assertion to the new validation error, then add a separate happy-path scenario at the earlier date. That both satisfies AC3 honestly and gives AC2 coverage across several product configs instead of one. Scenarios where only the expected tables changed (e.g. C74496, C74497, C85488, C85501) are legitimate consequences of AC1. > > The pause-drop isn't stable. It's action.getFrequency() != null "a frequency was supplied", not "the frequency changed". Re-sending the same frequency still re-dates the period and discards any pause extension, which the new "ignoring an overlapping pause" scenario locks in. It isn't durable: a later PAUSE/RESUME calls recalculatePeriodsForPauses, which rebuilds every period from period 1 using the effective frequency and re-applies all recorded pauses: resurrecting the extension that was just dropped. Same loan, different boundaries depending on action order. > > The validator and the service compute the same date two different ways. The validator uses action.getFrequency()/getFrequencyType(); the service uses params.frequency() from resolveEffectiveRescheduleParams. They agree only because the action is saved before the service runs and is the latest with a frequency group. Both also do their own findCurrentOpenPeriod lookup. Extract one helper both call, so the check can't drift from the mutation it's guarding. > > findCurrentOpenPeriod returning Optional is stricter than the loop it replaces. The old code tolerated multiple matches (last one won); the derived query throws IncorrectResultSizeDataAccessException if two open periods ever cover the business date. findTop…OrderByPeriodNumberDesc would preserve the old tolerance. > > The delinquency queries add AND s.reset = false, which the replaced loops did not have. A behaviour change bundled into a refactor. Unreachable today (resetPeriods only touches periods with toDate < resetDate), but if a reset period ever landed in the future window, updateFuturePeriods renumbers sequentially and would skip it → duplicate period numbers. Also note the breach queries have no equivalent filter, so the two schedules now diverge. > > Three different "period expired" predicates coexist in WorkingCapitalLoanBreachScheduleServiceImpl: evaluateBreachOnDate uses businessDate >= toDate, evaluateExpiredPeriods uses toDate < businessDate, recomputeBreach uses businessDate > toDate. Pre-existing, but this PR makes the boundary reachable by design, since the validation deliberately permits a re-dated period ending exactly on the business date. The "equals the current business date" scenario closes the loan immediately and never runs another COB, so what that period does at the next evaluation is unasserted. Add a COB step and pin it. > > Minor > > The breach validator now mixes two error mechanisms (failWithCodeNoParameterAddedToErrorCode and the new failGeneralValidation) and hardcodes "workingCapitalLoanBreachAction" in two places. The delinquency validator has a VALIDATION_RESOURCE constant — mirror it. > > No unit tests. calculateToDate and both validateFrequencyDoesNotEndBeforeBusinessDate methods are cheap to cover directly, especially the == businessDate / -1 day boundary that currently only has slow e2e coverage. 1. AC3 - eight scenarios had their inputs adjusted to stay valid under the new rule. Not changed. Those scenarios were written to cover reschedule inheritance and repeated reschedules, not the end date rule. Moving the business date keeps each of them testing what it was written for, and the new rule has its own dedicated scenarios on both the breach and the delinquency side. 2. Re-dating drops a pause extension, and a later pause resurrects it. Fixed. 3. The end date is computed in two places. Fixed. 4. findCurrentOpenPeriod returning Optional is stricter than the loop it replaced. Fixed. 5. AND s.reset = false in the delinquency query. Fixed. 6. Three predicates for "period expired", and the boundary is not asserted. The boundary part is fixed: the scenario now runs COB on the day after the recalculated end date and pins the resulting schedule, so the behaviour at that boundary is locked in. On the predicates, there are two rules, not three. evaluateExpiredPeriods and recomputeBreach are the same exclusive condition written two ways. The repository query and evaluateBreachOnDate are the same inclusive condition, and the query is just a prefilter for that check. The exclusive rule belongs to the reprocess path and the inclusive one to the COB evaluation path, which runs as of the previous business date. Unifying them would change breach timing logic. 7. Minor - mixed error mechanisms in the validator. Fixed. 8. Minor - no unit tests for the new date arithmetic. Not added as unit tests, extended current e2e test scenarios. -- 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]
