Bug 8337 - System preferences for longoverdue cron
Summary: System preferences for longoverdue cron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: simith.doliveira
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 13589
  Show dependency treegraph
 
Reported: 2012-06-29 19:12 UTC by Maxime Pelletier
Modified: 2016-12-05 21:23 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
patch to add longoverdue syspref (6.02 KB, patch)
2012-06-29 19:27 UTC, Maxime Pelletier
Details | Diff | Splinter Review
new patch without our po (2.74 KB, patch)
2012-08-06 17:26 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 8337 System preferences for longoverdue cron (3.59 KB, patch)
2013-03-21 21:53 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 8337 System preferences for longoverdue cron (3.66 KB, patch)
2013-03-22 16:26 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8337 System preferences for longoverdue cron (3.71 KB, patch)
2013-04-12 15:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8337 - Added the sysprefs to the database scripts. modified: installer/data/mysql/sysprefs.sql modified: installer/data/mysql/updatedatabase.pl (3.02 KB, patch)
2014-08-08 18:24 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 8337 - Follow-up to Comment 13 (6.40 KB, patch)
2014-08-18 15:13 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 8337 - Added the sysprefs to the database scripts. modified: installer/data/mysql/sysprefs.sql modified: installer/data/mysql/updatedatabase.pl (3.05 KB, patch)
2014-10-13 12:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8337 System preferences for longoverdue cron (3.81 KB, patch)
2014-10-13 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8337 - Added prefs to sysprefs and updatedatabase (3.01 KB, patch)
2014-10-13 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8337 - Follow-up to Comment 13 (4.44 KB, patch)
2014-10-13 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8337: Small typo in usage statement for longoverdue.pl (1.32 KB, patch)
2014-10-13 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8337: (RM followup) make sysprefs match case of the .pref file (6.03 KB, patch)
2014-11-21 22:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Pelletier 2012-06-29 19:12:13 UTC
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.
Comment 1 Maxime Pelletier 2012-06-29 19:13:56 UTC
Sponsored by the koha-CCSR community.
Comment 2 Maxime Pelletier 2012-06-29 19:27:18 UTC Comment hidden (obsolete)
Comment 3 Maxime Pelletier 2012-08-06 17:26:22 UTC Comment hidden (obsolete)
Comment 4 Maxime Pelletier 2013-03-13 16:32:31 UTC
This still applies to master, so switched to needs signoff.
Comment 5 Owen Leonard 2013-03-18 16:43:42 UTC
The latest patch doesn't appear to be valid. Git reports "Patch format detection failed."
Comment 6 Maxime Pelletier 2013-03-21 21:53:09 UTC Comment hidden (obsolete)
Comment 7 Paul Poulain 2013-03-22 16:26:04 UTC
Patch tested with a sandbox, by Leila and Fridos help <koha.aixmarseille@gmail.com>
Comment 8 Paul Poulain 2013-03-22 16:26:20 UTC Comment hidden (obsolete)
Comment 9 Koha Team AMU 2013-03-22 16:37:53 UTC
Tested but someone should test it again...

Syspref : DefaultLongOverdueChargeValue 123
 DefaultLongOverdueLostValue 1
DefaultLongOverdueDays 2 days
No finds & charges on a patron with long overdues...
Comment 10 Kyle M Hall 2013-04-12 15:03:10 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-05-30 15:01:02 UTC
QA comment:

Didn't test the patch, but the 3 syspref are not added to updatedatabase.pl and sysprefs.sql

Marked as Failed QA.
Comment 12 Maxime Beaulieu 2014-08-08 18:24:45 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2014-08-17 20:21:24 UTC
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.
Comment 14 Maxime Beaulieu 2014-08-18 15:13:06 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-08-22 14:57:10 UTC
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.
Comment 16 Blou 2014-08-22 15:04:07 UTC
These prefs are the whole objective of this patch: allow the librarian to set things up without having to rely on IT people.
Comment 17 Marcel de Rooy 2014-08-22 15:16:59 UTC
(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.
Comment 18 Jonathan Druart 2014-08-22 15:21:04 UTC
(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 :)
Comment 19 Anthony Laquerre 2014-08-22 15:35:32 UTC
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.
Comment 20 Katrin Fischer 2014-08-23 13:43:04 UTC
Adding Jesse as I think he had some ideas on how to make the cronjobs configurable in the staff interface.
Comment 21 Jonathan Druart 2014-09-10 15:16:04 UTC
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.
Comment 22 Blou 2014-09-10 15:41:42 UTC
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)
Comment 23 Jesse Weaver 2014-09-16 23:36:51 UTC
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.
Comment 24 Marcel de Rooy 2014-10-13 12:41:25 UTC
(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..
Comment 25 Marcel de Rooy 2014-10-13 12:51:30 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2014-10-13 14:30:51 UTC
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>
Comment 27 Marcel de Rooy 2014-10-13 14:30:57 UTC
Created attachment 32290 [details] [review]
Bug 8337 - Added prefs to sysprefs and updatedatabase

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2014-10-13 14:31:03 UTC
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>
Comment 29 Marcel de Rooy 2014-10-13 14:31:08 UTC
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>
Comment 30 Tomás Cohen Arazi 2014-11-21 22:41:25 UTC
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>
Comment 31 Tomás Cohen Arazi 2014-11-21 22:58:00 UTC
Patches pushed to master.

Thanks Frederik and Maxime!
Comment 32 Katrin Fischer 2016-04-05 08:08:20 UTC
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.