Bug 30947

Summary: Simplify date handling in CanbookBeIssued
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: CirculationAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, kyle.m.hall, lucas, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Attachments: Bug 30947: Add test for Illrequest checkout with custom due date
Bug 30947: Simplify CanBookBeIssued date handling
Bug 30947: Add test for Illrequest checkout with custom due date
Bug 30947: Simplify CanBookBeIssued date handling
Bug 30947: Add test for Illrequest checkout with custom due date
Bug 30947: Simplify CanBookBeIssued date handling
Bug 30947: (follow-up) Remove unnecessary object cloning

Description Joonas Kylmälä 2022-06-11 10:53:06 UTC
C4::Circulation::CanbookBeIssued does some unnecessary stuff with dates, opening bug to get rid of that extra code and make it simpler and more efficient :)
Comment 1 Joonas Kylmälä 2022-06-11 11:02:27 UTC
Created attachment 135959 [details] [review]
Bug 30947: Add test for Illrequest checkout with custom due date

To test:
 1) prove t/db_dependent/Illrequests.t
Comment 2 Joonas Kylmälä 2022-06-11 11:02:31 UTC
Created attachment 135960 [details] [review]
Bug 30947: Simplify CanBookBeIssued date handling

1) This removes support for passing string dates to CanBookBeIssued. The
function didn't publicly even document support for string dates, only
DateTime objects.

2) We get a $duedate always at least from CalcDateDue so having

 $issuingimpossible{INVALID_DATE} = output_pref($duedate);

was unneccesary and thus removed.

3) The check "duedate cannot be before now" was needlessly complex: if
the due date really cannot be before now we should check seconds too
and warn the librarian! Thus the truncation to minutes can be dropped
safely.

To test:
 1) prove t/db_dependent/Circulation.t
 2) prove t/db_dependent/Illrequests.t
 3) Enable OnSiteCheckouts and disable SpecifyDueDate syspref. Create
 on-site checkout for any patron and verify the due date is your
 current date at 23:59, you can check the exact minute with sql:
  > select * from issues
Comment 3 Nick Clemens (kidclamp) 2022-06-15 12:25:15 UTC
Created attachment 136092 [details] [review]
Bug 30947: Add test for Illrequest checkout with custom due date

To test:
 1) prove t/db_dependent/Illrequests.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens (kidclamp) 2022-06-15 12:25:19 UTC
Created attachment 136093 [details] [review]
Bug 30947: Simplify CanBookBeIssued date handling

1) This removes support for passing string dates to CanBookBeIssued. The
function didn't publicly even document support for string dates, only
DateTime objects.

2) We get a $duedate always at least from CalcDateDue so having

 $issuingimpossible{INVALID_DATE} = output_pref($duedate);

was unneccesary and thus removed.

3) The check "duedate cannot be before now" was needlessly complex: if
the due date really cannot be before now we should check seconds too
and warn the librarian! Thus the truncation to minutes can be dropped
safely.

To test:
 1) prove t/db_dependent/Circulation.t
 2) prove t/db_dependent/Illrequests.t
 3) Enable OnSiteCheckouts and disable SpecifyDueDate syspref. Create
 on-site checkout for any patron and verify the due date is your
 current date at 23:59, you can check the exact minute with sql:
  > select * from issues

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Martin Renvoize 2022-06-16 12:30:46 UTC
Created attachment 136158 [details] [review]
Bug 30947: Add test for Illrequest checkout with custom due date

To test:
 1) prove t/db_dependent/Illrequests.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-06-16 12:30:50 UTC
Created attachment 136159 [details] [review]
Bug 30947: Simplify CanBookBeIssued date handling

1) This removes support for passing string dates to CanBookBeIssued. The
function didn't publicly even document support for string dates, only
DateTime objects.

2) We get a $duedate always at least from CalcDateDue so having

 $issuingimpossible{INVALID_DATE} = output_pref($duedate);

was unneccesary and thus removed.

3) The check "duedate cannot be before now" was needlessly complex: if
the due date really cannot be before now we should check seconds too
and warn the librarian! Thus the truncation to minutes can be dropped
safely.

To test:
 1) prove t/db_dependent/Circulation.t
 2) prove t/db_dependent/Illrequests.t
 3) Enable OnSiteCheckouts and disable SpecifyDueDate syspref. Create
 on-site checkout for any patron and verify the due date is your
 current date at 23:59, you can check the exact minute with sql:
  > select * from issues

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-06-16 12:33:02 UTC
QA scripts are happy and tests all pass.

This is a nice simplification and improvement.. great work.

Passing QA
Comment 8 Joonas Kylmälä 2022-06-19 10:29:42 UTC
Created attachment 136329 [details] [review]
Bug 30947: (follow-up) Remove unnecessary object cloning

CalcDateDue() works on its own copy of the $startdate parameter
so the cloning in the calling end is not necessary.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 9 Joonas Kylmälä 2022-06-19 10:31:05 UTC
We were doing a copy of the $now DateTime object unnecessarily so added a trivial follow-up for that.
Comment 10 Tomás Cohen Arazi 2022-06-23 14:41:52 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 11 Lucas Gass 2022-07-29 15:31:25 UTC
Not going to backport this enhancement to 22.05.x unless requested