Bug 30518

Summary: StockRotationItems crossing DST boundary throw invalid local time exception
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: ToolsAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, dcook, kyle, nick, tomascohen, victor, wainuiwitikapark
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.06,21.05.15,20.11.19
Attachments: Bug 30518: Unit test
Bug 30518: Correct DateTime maths for needs_advancing
Bug 30518: Unit test
Bug 30518: Correct DateTime maths for needs_advancing

Description Martin Renvoize 2022-04-12 20:03:34 UTC
The 'needs_advance' method can fall foul of DateTime math errors when crossing a DST boundary which can in turn the cron script failing.
Comment 1 Martin Renvoize 2022-04-13 09:51:54 UTC
Created attachment 133233 [details] [review]
Bug 30518: Unit test

This patch adds tests for checking the needs_advancing method against
a timezone and datetime that would cause an Invalid local time failure

To test:
1. Apply this patch
2. Run:
   $ kshell
   k$ prove t/db_dependent/StockRotationItems.t
=> FAIL: The feature is not implemented, tests fail to complete
Comment 2 Martin Renvoize 2022-04-13 09:51:59 UTC
Created attachment 133234 [details] [review]
Bug 30518: Correct DateTime maths for needs_advancing

The needs_advancing method prior to this patch used basic DateTime
arithmatic, adding a DateTime::Duration in Days to the Arrival date of
the item and then comparing that to today. This, however, can cause bugs
when the arrival + duration date coincides with a DST boundary and as
such may result in an invalid local date.  See
https://metacpan.org/pod/DateTime#Making-Things-Simple for further
details.

This patch updates the code to use the DST safe delta_days method to
count the days between arrival and now instead and then compares this
integer to the defined duration of the stage.

To test:
1. Re-run the unit tests, they should now pass.
Comment 3 Nick Clemens 2022-04-14 10:44:30 UTC
Created attachment 133295 [details] [review]
Bug 30518: Unit test

This patch adds tests for checking the needs_advancing method against
a timezone and datetime that would cause an Invalid local time failure

To test:
1. Apply this patch
2. Run:
   $ kshell
   k$ prove t/db_dependent/StockRotationItems.t
=> FAIL: The feature is not implemented, tests fail to complete

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens 2022-04-14 10:44:34 UTC
Created attachment 133296 [details] [review]
Bug 30518: Correct DateTime maths for needs_advancing

The needs_advancing method prior to this patch used basic DateTime
arithmatic, adding a DateTime::Duration in Days to the Arrival date of
the item and then comparing that to today. This, however, can cause bugs
when the arrival + duration date coincides with a DST boundary and as
such may result in an invalid local date.  See
https://metacpan.org/pod/DateTime#Making-Things-Simple for further
details.

This patch updates the code to use the DST safe delta_days method to
count the days between arrival and now instead and then compares this
integer to the defined duration of the stage.

To test:
1. Re-run the unit tests, they should now pass.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2022-04-14 10:44:58 UTC
Trivial and good change, covered by tests, passing QA
Comment 6 Fridolin Somers 2022-04-20 07:24:24 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 7 Kyle M Hall 2022-04-29 15:16:58 UTC
Pushed to 21.11.x for 21.11.06
Comment 8 Andrew Fuerste-Henry 2022-05-03 19:54:12 UTC
Pushed to 21.05.x for 21.05.15
Comment 9 Victor Grousset/tuxayo 2022-05-09 21:49:36 UTC
Trying to backport to 20.11.x

Tests don't actually cover the changes. They pass without the implementation commit applied. So how should this be tested?

I'm still trusting the test suite to cover for general regressions, so backported anyway.
Backported: Pushed to 20.11.x branch for 20.11.19
Comment 10 wainuiwitikapark 2022-05-16 04:21:45 UTC
Does this need to be backported to 19.11.x?