Bug 38857 - Cronjobs should log their start and command line parameters before processing options
Summary: Cronjobs should log their start and command line parameters before processing...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-09 19:59 UTC by Nick Clemens (kidclamp)
Modified: 2025-01-11 17:33 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes all cronjobs so that if they fail because of bad parameters, information is now available in the log viewer to help with troubleshooting (when the CronjobLog system preference is enabled). Notes: - This changed all the cronjobs in misc/cronjobs that had the 'cronlogaction'. - It also changed misc/maintenance/fix_invalid_dates.pl (not a cronjob, but now only logs if confirmed - similar to misc/import_patrons.pl). - For misc/cronjobs/purge_suggestions.pl, a verbose option was added for consistency.
Version(s) released in:
Circulation function:


Attachments
Bug 38857: Update cronjobs to log run before processing parameters (28.04 KB, patch)
2025-01-09 20:53 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38857: Update cronjobs to log run before processing parameters (28.03 KB, patch)
2025-01-10 16:05 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 38857: Update cronjobs to log run before processing parameters (28.16 KB, patch)
2025-01-10 16:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38857: (QA follow-up) Fix perltidy errors (4.98 KB, patch)
2025-01-10 16:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-01-09 19:59:46 UTC
When a cronjob is passed bad parameters it may fail. If it does so before logging its start, there is no trace that it was even attempted

We should move the logging to before the options are fetched to ease troubleshooting

To test:
1 - Enable CronjobLog
2 - On command line:
    perl misc/cronjobs/cleanup_database.pl --old_reservers 550
3 - Error on command line
4 - In staff client, go to Tools->Log viewer, check 'Cronjobs' and submit
5 - No entries
Comment 1 Nick Clemens (kidclamp) 2025-01-09 20:53:51 UTC
Created attachment 176304 [details] [review]
Bug 38857: Update cronjobs to log run before processing parameters

A few notes:
- I grabbed this list by looking for things in misc/ that had cronlogaction
- I didn't touch scripts that aren't in the cronjobs directory, except
  fix_invalid_dates where I follow import_patrons and only log if confirmed
  since they aren't crons, I figure this seems reasonable
- purge_suggestions I moved the logggin up and changed the addition of
  effective days to a verbose message - this seems more consistent

To test:
 1 - Enable CronjobLog
 2 - On command line:
     perl misc/cronjobs/cleanup_database.pl --old_reservers 550
 3 - Error on command line
 4 - In staff client, go to Tools->Log viewer, check 'Cronjobs' and submit
 5 - No entries
 6 - Apply patch
 7 - Repeat and see run is logged with wrong parameter
 8 - Fix parameter and confirm
 9 - Spot check other files until you are satisfied
10 - Sign it off!
Comment 2 William Lavoie 2025-01-10 16:05:11 UTC
Created attachment 176369 [details] [review]
Bug 38857: Update cronjobs to log run before processing parameters

A few notes:
- I grabbed this list by looking for things in misc/ that had cronlogaction
- I didn't touch scripts that aren't in the cronjobs directory, except
  fix_invalid_dates where I follow import_patrons and only log if confirmed
  since they aren't crons, I figure this seems reasonable
- purge_suggestions I moved the logggin up and changed the addition of
  effective days to a verbose message - this seems more consistent

To test:
 1 - Enable CronjobLog
 2 - On command line:
     perl misc/cronjobs/cleanup_database.pl --old_reservers 550
 3 - Error on command line
 4 - In staff client, go to Tools->Log viewer, check 'Cronjobs' and submit
 5 - No entries
 6 - Apply patch
 7 - Repeat and see run is logged with wrong parameter
 8 - Fix parameter and confirm
 9 - Spot check other files until you are satisfied
10 - Sign it off!

Signed-off-by: William Lavoie <william.lavoie@inLibro.com>
Comment 3 Martin Renvoize (ashimema) 2025-01-10 16:36:27 UTC
Created attachment 176370 [details] [review]
Bug 38857: Update cronjobs to log run before processing parameters

A few notes:
- I grabbed this list by looking for things in misc/ that had cronlogaction
- I didn't touch scripts that aren't in the cronjobs directory, except
  fix_invalid_dates where I follow import_patrons and only log if confirmed
  since they aren't crons, I figure this seems reasonable
- purge_suggestions I moved the logggin up and changed the addition of
  effective days to a verbose message - this seems more consistent

To test:
 1 - Enable CronjobLog
 2 - On command line:
     perl misc/cronjobs/cleanup_database.pl --old_reservers 550
 3 - Error on command line
 4 - In staff client, go to Tools->Log viewer, check 'Cronjobs' and submit
 5 - No entries
 6 - Apply patch
 7 - Repeat and see run is logged with wrong parameter
 8 - Fix parameter and confirm
 9 - Spot check other files until you are satisfied
10 - Sign it off!

Signed-off-by: William Lavoie <william.lavoie@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize (ashimema) 2025-01-10 16:36:30 UTC
Created attachment 176371 [details] [review]
Bug 38857: (QA follow-up) Fix perltidy errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Katrin Fischer 2025-01-10 18:21:33 UTC
Pushed for 25.05!

Well done everyone, thank you!