Bug 30518 - StockRotationItems crossing DST boundary throw invalid local time exception
Summary: StockRotationItems crossing DST boundary throw invalid local time exception
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-12 20:03 UTC by Martin Renvoize
Modified: 2023-12-28 20:42 UTC (History)
7 users (show)

See Also:
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 (2.11 KB, patch)
2022-04-13 09:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30518: Correct DateTime maths for needs_advancing (2.74 KB, patch)
2022-04-13 09:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30518: Unit test (2.17 KB, patch)
2022-04-14 10:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30518: Correct DateTime maths for needs_advancing (2.79 KB, patch)
2022-04-14 10:44 UTC, Nick Clemens
Details | Diff | Splinter Review

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