Aarti-panchal01 commented on PR #6660:
URL: https://github.com/apache/fineract/pull/6660#issuecomment-6297275798
Thanks for the careful review @galovics. I amended the single commit; point
by point:
1. Added 4 tests for `validateOnLoanDisbursal` in
`CashierTransactionDataValidatorTest`: full day cashier on duty, part day
within its hours, part day outside its hours (13:01 for a 09:00-13:00 shift),
and nobody on duty. The first two assert the amount is checked against that
cashier's cash, the last two assert the cash is never looked up.
2. Agreed. On update the overlap and date checks now only run when the staff
member, the dates, `isFullDay` or the hours changed. I went with that instead
of `!changes.isEmpty()` for two reasons: a description-only edit of a legacy
overlapping record would still have failed with `!changes.isEmpty()`, and a
staff change never shows up in `changes` (the service calls `setStaff` before
`update`), so it would have been skipped. Added unit tests in
`TellerWritePlatformServiceJpaImplTest` for description only, no change, and
staff change.
3. Fixed. One `DateUtils.getOffsetDateTimeOfTenant()` call, and the date and
time of day both come from it.
4. I could not find anything in Fineract that supports shifts past midnight.
The old disbursal SQL (`TIME(now) BETWEEN start AND end`) and the old overlap
SQL both assumed start <= end, so a 22:00-02:00 cashier was never on duty
anyway. So a part-day shift where the end is not after the start (or a time
that can't be read, like 25:00) is now rejected with a 400,
`validation.msg.teller.hourEndTime.must.be.after.start.time`. Unit tests for
22:00-02:00, 09:00-09:00 and 25:00, plus one in `PartDayCashierTest`.
5. A stored time that can't be read still counts as an overlap (so a bad row
can't let a second assignment through), but it now logs a WARN with the cashier
id and the stored value, so the row can be found and fixed.
6. Done. `ExistingShift` and `parseTime` are gone. The query maps rows to
`Cashier` and the times are read with the `Cashier` getters
(`getHourFromStartTime()` etc). I kept it as plain SQL through
`NamedParameterJdbcTemplate` instead of a JPQL repository query, because in the
update and disbursal paths a JPQL query would auto-flush the pending changes of
the cashier or the half-built loan first. The `contains("TIME(")` and
SQL-string assertions are replaced with behavioural tests (self-exclusion on
update, adjacent shifts, params passed).
7. Downgraded to `LOG.debug`. A user who isn't on duty as a cashier is the
normal case, not an error.
One question where your call changes the code: rejecting end <= start also
applies on update when only the staff member changes, so a legacy overnight
cashier can't be moved to another staff member without fixing its hours. Is
that OK, or would you rather I only run the end-after-start check when the
hours themselves change?
--
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]
