Bug 30666 - Holds reminder cronjob (holds_reminder.pl) uses DateTime::subtract wrong
Summary: Holds reminder cronjob (holds_reminder.pl) uses DateTime::subtract wrong
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: 21.05
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords: rel_21_11_candidate
Depends on: 30667
Blocks:
  Show dependency treegraph
 
Reported: 2022-05-03 10:27 UTC by Kyle M Hall
Modified: 2022-10-25 11:00 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong (2.23 KB, patch)
2022-05-03 10:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong (2.20 KB, patch)
2022-05-03 11:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong (2.25 KB, patch)
2022-05-04 02:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong (2.34 KB, patch)
2022-05-13 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2022-05-03 10:27:57 UTC
In holds_reminder.pl, the script loops over all available branchcodes. For each iteration of the loop, if not using the calendar, the script subtracts the days parameter from the current date to get the waiting date threshold. The problem is that this method alters the DateTime object in $date_to_run, so for each iteration of the loop, the waiting date becomes farther and farther in the past, when it should always be the same!

The solution is to either clone the "date to run" for each call to subtract, or to move it out of the loop since it doesn't need to be recalculated each time.
Comment 1 Kyle M Hall 2022-05-03 10:33:43 UTC
Created attachment 134494 [details] [review]
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong

In holds_reminder.pl, the script loops over all available branchcodes. For each iteration of the loop, if not using the calendar, the script subtracts the days parameter from the current date to get the waiting date threshold. The problem is that this method alters the DateTime object in $date_to_run, so for each iteration of the loop, the waiting date becomes farther and farther in the past, when it should always be the same!

The solution is to either clone the "date to run" for each call to subtract, or to move it out of the loop since it doesn't need to be recalculated each time.

Test Plan:
1) Become the koha user using koha-shell
2) Run DBIC_TRACE=1 /usr/share/koha/bin/cronjobs/holds/holds_reminder.pl --days 7
3) Note in the queries that for each loop, the waiting date is different
4) Apply this patch
5) Run the command in step 2 again
6) Note the queries all now have the same waiting date threshold!
Comment 2 Kyle M Hall 2022-05-03 11:00:30 UTC
Created attachment 134505 [details] [review]
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong

In holds_reminder.pl, the script loops over all available branchcodes. For each iteration of the loop, if not using the calendar, the script subtracts the days parameter from the current date to get the waiting date threshold. The problem is that this method alters the DateTime object in $date_to_run, so for each iteration of the loop, the waiting date becomes farther and farther in the past, when it should always be the same!

The solution is to either clone the "date to run" for each call to subtract, or to move it out of the loop since it doesn't need to be recalculated each time.

Test Plan:
1) Become the koha user using koha-shell
2) Run DBIC_TRACE=1 misc/cronjobs/holds/holds_reminder.pl --days 7
3) Note in the queries that for each loop, the waiting date is different
4) Apply this patch
5) Run the command in step 2 again
6) Note the queries all now have the same waiting date threshold!
Comment 3 David Nind 2022-05-04 02:51:50 UTC
Created attachment 134565 [details] [review]
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong

In holds_reminder.pl, the script loops over all available branchcodes. For each iteration of the loop, if not using the calendar, the script subtracts the days parameter from the current date to get the waiting date threshold. The problem is that this method alters the DateTime object in $date_to_run, so for each iteration of the loop, the waiting date becomes farther and farther in the past, when it should always be the same!

The solution is to either clone the "date to run" for each call to subtract, or to move it out of the loop since it doesn't need to be recalculated each time.

Test Plan:
1) Become the koha user using koha-shell
2) Run DBIC_TRACE=1 misc/cronjobs/holds/holds_reminder.pl --days 7
3) Note in the queries that for each loop, the waiting date is different
4) Apply this patch
5) Run the command in step 2 again
6) Note the queries all now have the same waiting date threshold!

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Marcel de Rooy 2022-05-13 09:48:08 UTC
QA: looking here
Comment 5 Marcel de Rooy 2022-05-13 09:54:39 UTC
Created attachment 134959 [details] [review]
Bug 30666: Holds reminder cronjob (holds_reminder.pl) uses DataTime::subtract wrong

In holds_reminder.pl, the script loops over all available branchcodes. For each iteration of the loop, if not using the calendar, the script subtracts the days parameter from the current date to get the waiting date threshold. The problem is that this method alters the DateTime object in $date_to_run, so for each iteration of the loop, the waiting date becomes farther and farther in the past, when it should always be the same!

The solution is to either clone the "date to run" for each call to subtract, or to move it out of the loop since it doesn't need to be recalculated each time.

Test Plan:
1) Become the koha user using koha-shell
2) Run DBIC_TRACE=1 misc/cronjobs/holds/holds_reminder.pl --days 7
3) Note in the queries that for each loop, the waiting date is different
4) Apply this patch
5) Run the command in step 2 again
6) Note the queries all now have the same waiting date threshold!

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Fridolin Somers 2022-05-14 01:42:57 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄