adamsaghy commented on PR #6205: URL: https://github.com/apache/fineract/pull/6205#issuecomment-5168053246
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. -- 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]
