Bug 16219

Summary: Runreport.pl should allow SQL parameters to be passed on the command line
Product: Koha Reporter: Barton Chittenden <barton>
Component: Command-line UtilitiesAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: arthur.suzuki, cslone, fridolin.somers, katrin.fischer, lucas, m.de.rooy, martin.renvoize, nick, robin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.04
Attachments: Bug 16219: Add 'params' parameter to runreport.pl
Bug 16219: Add 'params' parameter to runreport.pl
Bug 16219: (follow-up) Nicer message if params missing and note that they are not combined
Bug 16219: Add 'params' parameter to runreport.pl
Bug 16219: (follow-up) Nicer message if params missing and note that they are not combined
Bug 16219: (QA follow-up) Tiny regex change

Description Barton Chittenden 2016-04-06 20:51:56 UTC
Runreport.pl has a major limitation in that it is not capable of running reports that use sql parameters:

Runreport will run this:

SELECT p.surname, p.firstname, p.email, p.cardnumber, 
       h.waitingdate AS 'hold date', i.barcode, b.title
FROM reserves h 
LEFT JOIN borrowers p USING (borrowernumber) 
LEFT JOIN items i USING (itemnumber) 
LEFT JOIN biblio b ON (i.biblionumber=b.biblionumber) 
WHERE h.waitingdate IS NOT NULL AND h.branchcode='ABC'


but if your report looks like this:

SELECT p.surname, p.firstname, p.email, p.cardnumber, 
       h.waitingdate AS 'hold date', i.barcode, b.title
FROM reserves h 
LEFT JOIN borrowers p USING (borrowernumber) 
LEFT JOIN items i USING (itemnumber) 
LEFT JOIN biblio b ON (i.biblionumber=b.biblionumber) 
WHERE h.waitingdate IS NOT NULL AND h.branchcode=<<Waiting at|branches>>

We should be able to supply a --sql_params argument one or more times to provide the sql parameters.

runreport.pl --to="someone@someplace.com" --subject="Patrons with Holds Waiting at Library" --format=csv --sql_params="ABC" 1
Comment 1 Nick Clemens 2018-09-05 11:21:20 UTC
Created attachment 78434 [details] [review]
Bug 16219: Add 'params' parameter to runreport.pl

To test:
 1 - Apply patch
 2 - Write a report that takes no variables
 3 - Run this using runreport.pl
    perl misc/cronjobs/runreport.pl 1
 4 - Confirm results as expected
 5 - Write a report that takes one or more variables
 6 - Run using runreport.pl, but pass no params
 7 - You should get a SQL error
 8 - Now run passing the correct number of params
    perl misc/cronjobs/runreport.pl --param first --param=2nd 2
 9 - Verify results as expected
10 - call runreport with no parameters
    perl misc/cronjobs/runreport.pl
11 - Verify documentation is correct and helpful
Comment 2 Claire Gravely 2019-06-06 12:31:54 UTC
Created attachment 90363 [details] [review]
Bug 16219: Add 'params' parameter to runreport.pl

To test:
 1 - Apply patch
 2 - Write a report that takes no variables
 3 - Run this using runreport.pl
    perl misc/cronjobs/runreport.pl 1
 4 - Confirm results as expected
 5 - Write a report that takes one or more variables
 6 - Run using runreport.pl, but pass no params
 7 - You should get a SQL error
 8 - Now run passing the correct number of params
    perl misc/cronjobs/runreport.pl --param first --param=2nd 2
 9 - Verify results as expected
10 - call runreport with no parameters
    perl misc/cronjobs/runreport.pl
11 - Verify documentation is correct and helpful

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 3 Katrin Fischer 2019-06-09 20:54:10 UTC
This works, but feels a little hacky:
- When the report expects params, the SQL message is output. We could possibly print a nicer error message instead with the number of params to expected.
- When the report is built with reusable variables, you need to enter each of those on command line. If not easy to fix, hinting at it in the help might be good.

My example query was:
select * from items where holdingbranch = <<b|branches>> and homebranch = <<b|branches>> and itype= <<i>>

So b will be shown as 1 input field, but will be required twice in the command:

./misc/cronjobs/runreport.pl 1 --param=CPL --param=BK
DBD::mysql::st execute failed: called with 4 bind variables when 5 are needed [for Statement "select * from items where holdingbranch = ? and homebranch = ? and itype= ? LIMIT ?, ?" with ParamValues: 0=Null!, 1=Null!, 2=Null!, 3=Null!, 4=Null!] at /home/vagrant/kohaclone/C4/Reports/Guided.pm line 544.
NO OUTPUT: 0 results from execute_query

./misc/cronjobs/runreport.pl 1 --param=CPL --param=CPL --param=BK
Works.

Pushing back to Nick for comment.
Comment 4 Nick Clemens 2019-07-19 19:43:32 UTC
Created attachment 91668 [details] [review]
Bug 16219: (follow-up) Nicer message if params missing and note that they are not combined
Comment 5 Nick Clemens 2019-07-19 19:45:36 UTC
(In reply to Katrin Fischer from comment #3)
> This works, but feels a little hacky:
> - When the report expects params, the SQL message is output. We could
> possibly print a nicer error message instead with the number of params to
> expected.

Ok, done

> - When the report is built with reusable variables, you need to enter each
> of those on command line. If not easy to fix, hinting at it in the help
> might be good.

Noted in the POD, really reports needs an update and move to objects, this is a helpful patch mostly for sysadmins (and to prevent libraries having to have hardcoded cron version reports and staff side version with params)
Comment 6 Marcel de Rooy 2019-08-09 10:06:16 UTC
QA: Looking here
Comment 7 Marcel de Rooy 2019-08-09 10:22:32 UTC
Created attachment 92113 [details] [review]
Bug 16219: Add 'params' parameter to runreport.pl

To test:
 1 - Apply patch
 2 - Write a report that takes no variables
 3 - Run this using runreport.pl
    perl misc/cronjobs/runreport.pl 1
 4 - Confirm results as expected
 5 - Write a report that takes one or more variables
 6 - Run using runreport.pl, but pass no params
 7 - You should get a SQL error
 8 - Now run passing the correct number of params
    perl misc/cronjobs/runreport.pl --param first --param=2nd 2
 9 - Verify results as expected
10 - call runreport with no parameters
    perl misc/cronjobs/runreport.pl
11 - Verify documentation is correct and helpful

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2019-08-09 10:22:36 UTC
Created attachment 92114 [details] [review]
Bug 16219: (follow-up) Nicer message if params missing and note that they are not combined

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2019-08-09 10:22:41 UTC
Created attachment 92115 [details] [review]
Bug 16219: (QA follow-up) Tiny regex change

The parameter <<>> does not work. There should be text inside.
The construction .*? does not look good completely.
Replacing it by the stricter [^>]+ So, no greater than's and at least one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Nick Clemens 2019-08-09 10:53:22 UTC
(In reply to Marcel de Rooy from comment #9)
> Created attachment 92115 [details] [review] [review]
> Bug 16219: (QA follow-up) Tiny regex change
> 
> The parameter <<>> does not work. There should be text inside.
> The construction .*? does not look good completely.
> Replacing it by the stricter [^>]+ So, no greater than's and at least one.
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Thanks Marcel!
Comment 11 Martin Renvoize 2019-08-13 13:37:33 UTC
Nice work!

Pushed to master for 19.11.00
Comment 12 Fridolin Somers 2019-09-02 14:22:06 UTC
Small but powerfull enhancement, I choose to backport.

Pushed to 19.05.x for 19.05.04
Comment 13 Lucas Gass 2019-09-17 20:15:24 UTC
enhancement, not backporting to 18.11.x