budaidev commented on PR #6283: URL: https://github.com/apache/fineract/pull/6283#issuecomment-5545555184
> The undo path is well covered - unit tests on both the resolver and the schedule methods, plus six integration tests including the pause-extended and payment-reassignment cases. That's usually the part I have to ask for, so nice work there. > > My concern is with how the undo figures out what to revert. `restoreSplitPeriod` reverse-engineers the pre-split state from the schedule as it stands now plus the current breach config, using three guard clauses to decide whether the split it's looking at is actually the one this undo owns. Two of those guards fall through to a `log.warn` and a `false`, and the caller then just recalculates past due and returns 200 - so the user gets a success response for an undo that only lifted a flag and left the split in place. > > Concretely: a RESCHEDULE recorded between the reset and the undo changes `resolveEffectiveRescheduleParams`, so the restore recomputes a boundary that never existed. And any PAUSE/RESUME between reset and undo rewrites every period contiguously from the schedule's first `fromDate` without knowing about the split, so the split disappears from the schedule entirely while the RESET row still says `restartPeriodFromResetDate = true` - the undo then silently falls into the flag-only branch. > > Wouldn't it be simpler and deterministic to record on the RESET action row what the reset actually changed (the pre-split `toDate`, maybe the period id), and have the undo just write that back? Then there's nothing to guess and nothing that can silently degrade. The same reconstruction problem shows up in `findLatestFlaggedPeriod`'s "last flagged period by period number" fallback in the reset service - it assumes period-number order tracks reset-date order, which is exactly the assumption the boundary rewrites can break. > > Two smaller things while I was in there: > > * `splitPeriodAtReset` only guards `resetDate == fromDate`; `resetDate < fromDate` falls through and can persist a period with `toDate < fromDate` / negative `numberOfDays`. > * The RESET-undo replay in `WorkingCapitalLoanActiveBreachResetResolver` is ordered by `startDate, id` - since reset/undo dates are always "the current business date," if the business date is ever moved backwards between two actions the replay order diverges from creation order, and the `break`-on-id can stop before the reset it's meant to cancel. > > This is a draft so nothing's blocking yet, but I'd like to settle the design question (store vs. reconstruct) before it comes out of draft - it affects several of the methods above at once. > > Recommendation: COMMENT Thanks for the review, and sorry for the late answer here: the design question was settled with Adam and the implementation you reviewed has been replaced, so the concerns below no longer apply to the current head. -- 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]
