https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20125 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA QA Contact|alex.arnaud@biblibre.com | --- Comment #32 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Arthur, I this this would be a great addition, but I noticed some smaller and some bigger things in testing. I hope the comments make sense: 1) Documentation a) letter-code option is undocumented. b) We have max-claim, why not min-days instead of claimed-for? I think this could improve usability and make it a little easier to understand intuitively. c) bookseller should be vendor +Default is the booseller's delivery time or 0. + Koha::Exception->throw("Bookseller not found for booksellerid: $booksellerid"); d) Small typos: +Print a brief help message and exits. Should be either "Prints" or "exit" I believe. +This script get all late orders (depending on --delay parameter, +group them by basket and put an entry in message_queue table. ... gets ... groups... puts... +Number of days from which an orders is considered as late. ... order ... e) Rephrase suggestion (not blocker) +ordersClaim.pl - cron script that put late orders in message queue grouped by basket. Cron script to generate late order claim notices and add them to the message queue. 2) Script name (not blocker) Currently the name of the script is ordersClaim.pl Looking at the other script, I see more cases of verb_subject.pl. (merge_authorities, remove_..., process_message_queue.pl, gather_print_notices.pl, delete_... etc.) Maybe change to: claim_orders.pl ? I know we have no coding guideline there, just trying for consistency whenever possible. :) 3) Testing functionality a) Run without parameters When the script is run without any parameters it still "does" something. I think it would be better, if it printed the help and required a confirmation option to actually change data. Otherwise you could easily run a quite expensive operation on a big database sending out a lot of emails. b) Not logging to action_logs (blocker) The script is not logging start and end to the action_logs (probably since it predates those changes). As this is a coding guideline now (CMD1), I think it's a blocker. c) Exception error when encountering a vendor with no contact When the script encounters a vendor with no contact information, it prints: Exception 'Koha::Exception' thrown 'No contact found for booksellerid: 1 I think this could clutter logs pretty fast and probably should be tied to a -v verbose option. A vendor without contact is by itself not a problem in Koha and might just indicate the library doesn't want to send them emails. Also: the error is misleading. In my case the vendor had a contact, but it was not set up to send late orders. d) Message is created for vendors without email address. (blocker) I believe we should require email address, primary acquisition contact and contact about late orders as minimal requirements (that should match the GUI behavior) e) Generated email text The generated email text in the message_queue contains <order> tags. f) There is no -v option There is no way to see a success message or summary of the results on command line. X) General questions (not blocker) a) Why grouping by basked instead of grouping by vendor? -- You are receiving this mail because: You are watching all bug changes.