budaidev commented on PR #6283: URL: https://github.com/apache/fineract/pull/6283#issuecomment-5579135300
> Thanks @budaidev, this addresses everything I raised in the draft review. The LIFO replay now goes by recording order (not business-date order) with the reasoning documented directly on the method, the negative-period guard is in, and the reschedule-after-reset case is handled by filtering to reschedules in force at the time of the reset. Test coverage went from thin to substantial - unit tests on the resolver, the flag derivation, and the split/restore methods, plus a large Feign integration suite and new e2e scenarios. > > One thing I want called out explicitly rather than left implicit: two of your own new integration tests demonstrate that a split can still be silently lost. `pauseAfterRestartReset_keepsTheFlagOnThePeriodHoldingTheResetDate` shows a pause recorded after a restart-reset re-expanding the truncated boundary back to its natural length - the reset flag survives but the split itself is gone, so the later undo has nothing to restore. And `undoAfterBackwardsBusinessDateMove_flagsTheRegeneratedPeriodOfTheStillActiveReset` shows a second restart-reset deleting an earlier, still-active reset's boundary permanently, even though that earlier reset stays on the active stack. > > Both of these read like accepted/intentional behavior in the tests as written, and I don't think either needs to block this PR - the acute failure modes (heuristic drift, stale reschedule params, "success" on a no-op undo) are genuinely closed. But if the split is meant to be durable across a pause or a stacked reset, these two cases say otherwise, and I'd rather that be a documented decision (or a follow-up ticket to persist the pre-split boundary on the reset action itself, which would close both at once) than something a future bug report rediscovers. > > Recommendation: COMMENT (would be APPROVE if the two cases above get an explicit answer either in this PR's description or as a tracked follow-up) Since your review, one more fix from Adam's last comment went in: the pause re-dating, the initial period and the undo restore now compute the period end through one shared helper, so period 1's grace days can no longer drift between them; unit and Feign scenarios cover it. -- 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]
