Patch to follow that adds 3 new system preferences to control the parameters for the longoverdue script. New prefs are DefaultLongOverdueChargeValue, DefaultLongOverdueLostValue and DefaultLongOverdueDays. These preferences are used when you call the longoverdue.pl script without their parameters.
Sponsored by the koha-CCSR community.
Created attachment 10569 [details] [review] patch to add longoverdue syspref
Created attachment 11389 [details] [review] new patch without our po
This still applies to master, so switched to needs signoff.
The latest patch doesn't appear to be valid. Git reports "Patch format detection failed."
Created attachment 16687 [details] [review] Bug 8337 System preferences for longoverdue cron Patch to follow that adds 3 new system preferences to control the parameters for the longoverdue script. New prefs are DefaultLongOverdueChargeValue, DefaultLongOverdueLostValue and DefaultLongOverdueDays. These preferences are used when you call the longoverdue.pl script without their parameters. Sponsored-By: CCSR
Patch tested with a sandbox, by Leila and Fridos help <koha.aixmarseille@gmail.com>
Created attachment 16810 [details] [review] Bug 8337 System preferences for longoverdue cron Patch to follow that adds 3 new system preferences to control the parameters for the longoverdue script. New prefs are DefaultLongOverdueChargeValue, DefaultLongOverdueLostValue and DefaultLongOverdueDays. These preferences are used when you call the longoverdue.pl script without their parameters. Sponsored-By: CCSR Signed-off-by: Leila and Fridos help <koha.aixmarseille@gmail.com>
Tested but someone should test it again... Syspref : DefaultLongOverdueChargeValue 123 DefaultLongOverdueLostValue 1 DefaultLongOverdueDays 2 days No finds & charges on a patron with long overdues...
Created attachment 17393 [details] [review] Bug 8337 System preferences for longoverdue cron Patch to follow that adds 3 new system preferences to control the parameters for the longoverdue script. New prefs are DefaultLongOverdueChargeValue, DefaultLongOverdueLostValue and DefaultLongOverdueDays. These preferences are used when you call the longoverdue.pl script without their parameters. Sponsored-By: CCSR Signed-off-by: Leila and Fridos help <koha.aixmarseille@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA comment: Didn't test the patch, but the 3 syspref are not added to updatedatabase.pl and sysprefs.sql Marked as Failed QA.
Created attachment 30639 [details] [review] Bug 8337 - Added the sysprefs to the database scripts. modified: installer/data/mysql/sysprefs.sql modified: installer/data/mysql/updatedatabase.pl
Starting with a code review: 1) (trivial) The QA script has some complaints about those patches: FAIL installer/data/mysql/sysprefs.sql FAIL sysprefs_order Not blocker: Sysprefs defaultlongoverduechargevalue is bad placed (see bug 10610) FAIL installer/data/mysql/updatedatabase.pl OK critic FAIL pod *** WARNING: =head2 without preceding higher level in file installer/data/mysql/updatedatabase.pl OK valid OK forbidden patterns 2) (trivial) Database update should include the bug number for later reference. 3) (normal) The database update and sysprefs.sql use all lowercaps system preference names, but the pref file and the longoerdue.pl use camel case. 4) (just a question) I took a look at our crontab example file and the cronjob is commented out with parameters and in debian/koha-common.cron.daily the cronjob is missing. Should we adapt those? Please provide a follow-up and switch back to Signed-off.
Created attachment 30944 [details] [review] Bug 8337 - Follow-up to Comment 13 Fixed QA script issues. Syspref names are now camelcased. Bug number added in updatedatabase.pl . If the cronjob is deactivated by default, shouldn't we leave it like that? Bear in mind that I am not an expert. modified: installer/data/mysql/sysprefs.sql modified: installer/data/mysql/updatedatabase.pl
I don't understand the usefulness of these 3 sysprefs. I know everybody loves sysprefs but do you have another argument? The longoverdue script is a cronjob and if you want to call this script with "default parameters" you just have to set them in the crontab.
These prefs are the whole objective of this patch: allow the librarian to set things up without having to rely on IT people.
(In reply to Blou from comment #16) > These prefs are the whole objective of this patch: allow the librarian to > set things up without having to rely on IT people. If we increase the number of sysprefs, we may end up relying even more on IT people :) Looking at your argument, I would agree with Jonathan: You only have to put the correct arguments once (or very incidentally) in your crontab. No need for extra sysprefs.
(In reply to Blou from comment #16) > These prefs are the whole objective of this patch: allow the librarian to > set things up without having to rely on IT people. 1 new pref for every argument of each script in misc/cronjob, outch :)
I jump in the conversation since we are the schools consortia that initially create the ticket. In a context of school library systems such preferences are likely to change on average 2-3 times per year. Unfortulalty, many library does not have the money available to make requests for changes to IT company. Unlike Jonathan, I beleive that each cronjob parameter useful for the end user should be in the sys pref table. This allows the librarian to make his own choix and not always having to go through a process of change by the IT provider. I understand that right now there is a lot of parameters but this is also one the of biggest strenght of Koha ! Hard to start but once you understand how every sys pref work you can do anything you want.
Adding Jesse as I think he had some ideas on how to make the cronjobs configurable in the staff interface.
You should sent an email on the mailing list to get other points of view. In my opinion, this should never be pushed like that. One pref per script parameter is not conceivable.
You extrapolate, here. We're not talking about every script, nor about every argument of every script. Technicaly, there are no constraint on the number of prefs we can handle. There were 10, then 200 and there'll be 2000. Having the right nomenclature to identify them could be a big plus, but that is true for every pref already in the DB, imo. Now, if we go back to the need of the user, "configurable" cron job is a need. Specifically for this cronjob. And this seems like something other users would benefit from. With no impact. (THere are alternatives, but they do have impact, and it's outside this small scope)
I have some ideas related to this, though implementation is a bit stalled at the moment. My plans were for a configuration system outside of the system preferences, which could potentially replace all daily tasks in the crontab with: 0 1 * * * $KOHA_CRON_PATH/misc/cronjobs/run_daily_tasks.pl and a matching database structure to hold information on enabled/disabled cronjobs and all of their selected options (including metadata, somewhere, on what options were available for each and what values they took). This cronjob would load up all enabled cronjobs, figure out what dependency order they had to be executed in, then run them in series. There was also some thought for a built-in output logging facility. Note that I don't have any plans to modify the cronjobs in question; they would still be run as external scripts, just with options built up from the configuration in the database.
(In reply to Anthony Laquerre from comment #19) > In a context of school library systems such preferences are likely to change > on average 2-3 times per year. > Unfortulalty, many library does not have the money available to make > requests for changes to IT company. > > Unlike Jonathan, I beleive that each cronjob parameter useful for the end > user should be in the sys pref table. > This allows the librarian to make his own choix and not always having to go > through a process of change by the IT provider. In some command-line scripts preferences are used too (e.g. look at finesMode in fines.pl). So, an exception can always be made. Still, the principle is that cronjobs get their parameters on the command line. A lot of people may be able to access the preferences and change them by incident, the crontab file needs root privileges. So further away, but safe. If someone occassionally changes this pref, he may not even realize that a cron job will later use the value. The fact that you add them as fallback, makes it somewhat better. If you put them in the crontab, you should not fear the pref values anymore ;) Will look if the description of the prefs still needs some tweaking..
Created attachment 32288 [details] [review] Bug 8337 - Added the sysprefs to the database scripts. modified: installer/data/mysql/sysprefs.sql modified: installer/data/mysql/updatedatabase.pl
Created attachment 32289 [details] [review] Bug 8337 System preferences for longoverdue cron Patch to follow that adds 3 new system preferences to control the parameters for the longoverdue script. New prefs are DefaultLongOverdueChargeValue, DefaultLongOverdueLostValue and DefaultLongOverdueDays. These preferences are used when you call the longoverdue.pl script without their parameters. Sponsored-By: CCSR Signed-off-by: Leila and Fridos help <koha.aixmarseille@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 32290 [details] [review] Bug 8337 - Added prefs to sysprefs and updatedatabase Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 32291 [details] [review] Bug 8337 - Follow-up to Comment 13 Fixed QA script issues. Syspref names are now camelcased and sorted correctly in sysprefs.sql. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 32292 [details] [review] Bug 8337: Small typo in usage statement for longoverdue.pl The charge with the replacement price will only be made if the charge parameter matches the lost value. The given example is faulty (as well as some of the code, as it appears ;) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 33802 [details] [review] Bug 8337: (RM followup) make sysprefs match case of the .pref file Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Patches pushed to master. Thanks Frederik and Maxime!
Hi, wondering if I am missing something - but is it possible to add the longoverdues.pl to the cron files (normal and packages) and have it inactive until a library chooses to set the system preferences? It seems that the lost value is mandatory, so I have an error, when I try running the script without it being set (syspref empty, no param): ERROR: No --lost (-l) option defined at ./misc/cronjobs/longoverdue.pl line 119.