Bug 30947 - Simplify date handling in CanbookBeIssued
Summary: Simplify date handling in CanbookBeIssued
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-11 10:53 UTC by Joonas Kylmälä
Modified: 2022-07-29 15:31 UTC (History)
5 users (show)

See Also:
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 (2.20 KB, patch)
2022-06-11 11:02 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30947: Simplify CanBookBeIssued date handling (5.72 KB, patch)
2022-06-11 11:02 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30947: Add test for Illrequest checkout with custom due date (2.26 KB, patch)
2022-06-15 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30947: Simplify CanBookBeIssued date handling (5.78 KB, patch)
2022-06-15 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30947: Add test for Illrequest checkout with custom due date (2.32 KB, patch)
2022-06-16 12:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30947: Simplify CanBookBeIssued date handling (5.84 KB, patch)
2022-06-16 12:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30947: (follow-up) Remove unnecessary object cloning (1.27 KB, patch)
2022-06-19 10:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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