Bug 9915 - Allow opac/svc/report to be passed sql parameters
Summary: Allow opac/svc/report to be passed sql parameters
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Chris Cormack
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 13219
  Show dependency treegraph
 
Reported: 2013-03-24 07:36 UTC by Chris Cormack
Modified: 2015-06-04 23:33 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 9915 : Allow sql_params to be passed to a report using the svc interface (3.74 KB, patch)
2013-03-24 07:39 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9915 : Allow sql_params to be passed to a report using the svc interface (3.77 KB, patch)
2014-03-06 22:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9915 : Allow sql_params to be passed to a report using the svc interface (3.82 KB, patch)
2014-03-07 19:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
0001-PASSED-QA-Bug-9915-Allow-sql_params-to-be-passed-to-.patch (4.72 KB, patch)
2014-04-20 08:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9915: Allow sql_params to be passed to a report using the OPAC svc/report interface (4.77 KB, patch)
2014-04-20 16:08 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9915: (follow-up) use SQL placeholders (5.72 KB, patch)
2014-04-20 16:08 UTC, Galen Charlton
Details | Diff | Splinter Review
[PASSED QA] Bug 9915: (follow-up) use SQL placeholders (5.83 KB, patch)
2014-04-20 20:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12114: Fix encoding problem with reports JSON web service (opac) (1.58 KB, patch)
2014-04-20 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12114: Fix encoding problem with reports JSON web service (staff) (1.23 KB, patch)
2014-04-20 22:10 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2013-03-24 07:36:52 UTC
To allow for things like cgi-bin/koha/svc/report?id=1&sql_params=chris&sql_params=chris2
Comment 1 Chris Cormack 2013-03-24 07:39:34 UTC Comment hidden (obsolete)
Comment 2 Julien Sicot 2013-04-03 21:50:21 UTC
I tested the patch following your test plan but json output remains empty (just []) when i call report requiring parameters.
By cons, if i call a report without parameters, i get a json response.

I tested with the following sql query :

SELECT COUNT(s.datetime) as "Total" FROM statistics AS s JOIN borrowers AS b ON s.borrowernumber=b.borrowernumber WHERE (year(s.datetime)=<<Année>> AND s.type = "issue") AND s.branch = <<Bibliothèque|branches>>

Tested on master (3.11.00.201)
Comment 3 Katrin Fischer 2013-04-04 05:35:16 UTC
Did you specify the parameters in the URL when calling the report? Maybe you can share the URL?
Comment 4 Julien Sicot 2013-04-04 06:30:32 UTC
Yes i specified paramesters in URL like below (my report ID is 49) :
cgi-bin/koha/svc/report?id=49&sql_params=2012&sql_params=BU

I can't share url because i tested from a local install. I will try to test it from a live install.
Comment 5 Julien Sicot 2013-04-04 07:27:31 UTC
I tested on live install, same behavior :-(
Below are the URLs :

with parameters :
http://pro-kohapreprod.bu.uhb.fr/cgi-bin/koha/svc/report?id=108&sql_params=2012&sql_params=BU

without :
http://pro-kohapreprod.bu.uhb.fr/cgi-bin/koha/svc/report?id=109

I precise that reports are public.
Comment 6 Chris Cormack 2013-04-04 07:37:26 UTC
And it works in the report interface with those values?
Comment 7 Julien Sicot 2013-04-04 07:46:59 UTC
yes, normally the response of report #108 should be the same as report #109.
Comment 8 Katrin Fischer 2013-04-07 11:25:08 UTC
Hm I tested it too and it does look like the params are not replaced in the SQL:

[Sun Apr 07 13:21:18 2013] [error] [client 127.0.0.1] [Sun Apr  7 13:21:18 2013] report: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '<<holding|branches>> and homebranch = <<home|branches>> LIMIT 0, 10' at line 1 at /home/katrin/kohaclone/C4/Reports/Guided.pm line 507.
[Sun Apr 07 13:21:18 2013] [error] [client 127.0.0.1] [Sun Apr  7 13:21:18 2013] report: DBD::mysql::st fetchall_arrayref failed: fetch() without execute() at /home/katrin/kohaclone/svc/report line 60.


My report is:
select * from items where holdingbranch = <<holding|branches>> and homebranch = <<home|branches>>

Used URL:
http://localhost:8080/cgi-bin/koha/svc/report?id=2&sql_params=MPL&sql_params=MPL
Comment 9 Katrin Fischer 2013-08-11 08:53:35 UTC
Chris, could you maybe take another look at this? I think it would be a really nice and helpful feature.
Comment 10 Chris Cormack 2013-08-11 20:01:37 UTC
(In reply to Katrin Fischer from comment #9)
> Chris, could you maybe take another look at this? I think it would be a
> really nice and helpful feature.

Maybe, it's just something I was doing on my own time, and I don't have much of that at the moment.
Comment 11 Chris Cormack 2014-01-24 01:11:39 UTC
Try it without the <<something|something>> construct.
Because that is for making a dropdown, which we obviously can't do
Comment 12 Marc Véron 2014-03-06 16:07:10 UTC
Merge conflict in opac/svc/report
Comment 13 Chris Cormack 2014-03-06 22:00:22 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-03-07 19:26:04 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-03-07 19:27:27 UTC
From a QA perspective, should this be using placeholders rather than quote? I know I've had a finger wagged at be once or twice for using quote and/or quote_identifier.
Comment 16 Chris Cormack 2014-03-07 19:32:57 UTC
(In reply to Kyle M Hall from comment #15)
> From a QA perspective, should this be using placeholders rather than quote?
> I know I've had a finger wagged at be once or twice for using quote and/or
> quote_identifier.

Perhaps, but it is using the exact same code as the reports engine on the staff side, so that it gives the same behaviour. I don't like to mix refactoring and new features on the same bug.
Comment 17 Martin Renvoize 2014-03-12 11:57:04 UTC
I'm afraid I can't get this to work for more than one parameter?

I have the below report to grab 'New Items added between two dates', running through the normal report interface I can varify I do get responses for the query, but running through svc interface I get zero results (i.e [] is the response)

URLs and Report detailed below:

svc URL used:
http://koha-staff.koha-ptfs.co.uk/cgi-bin/koha/svc/report?id=69&sql_params=2010-03-19&sql_params=2010-11-18

report URL used:
http://koha-staff.koha-ptfs.co.uk/cgi-bin/koha/reports/guided_reports.pl?reports=69&phase=Run+this+report&sql_params=2010-03-19&sql_params=2010-11-18

report used:
SELECT  biblio.title,biblio.author,biblio.datecreated,biblioitems.isbn,items.barcode,items.location FROM items LEFT JOIN biblioitems on (items.biblioitemnumber=biblioitems.biblioitemnumber) LEFT JOIN biblio on (biblioitems.biblionumber=biblio.biblionumber)   WHERE biblio.datecreated >= <<Enter Start Date|date>>  AND biblio.datecreated <= <<Enter End Date|date>> ORDER BY biblio.author asc

If I remove the second parameter from the report and hard code it, I can use both the reports and svc interface without issue.

Could you look into this Chris?
Comment 18 Chris Cormack 2014-04-18 04:00:28 UTC
(In reply to Martin Renvoize from comment #17)
> I'm afraid I can't get this to work for more than one parameter?
> 
> I have the below report to grab 'New Items added between two dates', running
> through the normal report interface I can varify I do get responses for the
> query, but running through svc interface I get zero results (i.e [] is the
> response)
> 
> URLs and Report detailed below:
> 
> svc URL used:
> http://koha-staff.koha-ptfs.co.uk/cgi-bin/koha/svc/
> report?id=69&sql_params=2010-03-19&sql_params=2010-11-18
> 
> report URL used:
> http://koha-staff.koha-ptfs.co.uk/cgi-bin/koha/reports/guided_reports.
> pl?reports=69&phase=Run+this+report&sql_params=2010-03-19&sql_params=2010-11-
> 18
> 
> report used:
> SELECT 
> biblio.title,biblio.author,biblio.datecreated,biblioitems.isbn,items.barcode,
> items.location FROM items LEFT JOIN biblioitems on
> (items.biblioitemnumber=biblioitems.biblioitemnumber) LEFT JOIN biblio on
> (biblioitems.biblionumber=biblio.biblionumber)   WHERE biblio.datecreated >=
> <<Enter Start Date|date>>  AND biblio.datecreated <= <<Enter End Date|date>>
> ORDER BY biblio.author asc
> 
> If I remove the second parameter from the report and hard code it, I can use
> both the reports and svc interface without issue.
> 
> Could you look into this Chris?

I cant replicate it failing, I did one with 3 parameters and it worked fine
http://rorohiko/cgi-bin/koha/svc/report?id=196&sql_params=2014-01-01&sql_params=AS&sql_params=ABS
Comment 19 Katrin Fischer 2014-04-20 08:05:29 UTC
I think I found the problem - staff URLs (non-public reports) don't accept the params, but it works nicely for public reports. It would be nice if the same feature could be made available for non-public reports as well, but this is still a very nice addition as it is.
Comment 20 Katrin Fischer 2014-04-20 08:05:57 UTC Comment hidden (obsolete)
Comment 21 Galen Charlton 2014-04-20 16:08:21 UTC
Created attachment 27327 [details] [review]
Bug 9915: Allow sql_params to be passed to a report using the OPAC svc/report interface

Test plan:

1/ Make a public report that contains SQL parameters
2/ Test it from the staff side
3/ Hit the url like
cgi-bin/koha/svc/report?id=1&sql_params=chris&sql_params=chris2
4/ Test you get JSON

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

Tested with a public report:
select * from items
  - cgi-bin/koha/svc/report?id=6
  - URL shows JSON and all items
select * from items where itype = <<itype>>
  - cgi-bin/koha/svc/report?id=6&sql_params=BK
  - URL shows JSON and only BK items
select * from items where itype = <<itype>> and homebranch = <<homebranch>>
  - cgi-bin/koha/svc/report?id=6&sql_params=BK&sql_params=MPL
  - URL shows JSON and only BK items in MPL
select * from items where itype = <<itype|itemtypes>> and homebranch = <<homebranch|branches>>
  - same as above
  - URL shows JSON and only BK items in MPL
select * from items where dateaccessioned = 2013-12-15
  - cgi-bin/koha/svc/report?id=6&sql_params=BK&sql_params=MPL&sql_params=2013-12-15
  - URL shows JSON and correct items

Note: This will currently only work for public reports!

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Comment 22 Galen Charlton 2014-04-20 16:08:30 UTC Comment hidden (obsolete)
Comment 23 Galen Charlton 2014-04-20 16:09:49 UTC
I've added a follow-up that replaces the use of ->quote() with placeholders, and am kicking this back for QA review of the follow-up.
Comment 24 Katrin Fischer 2014-04-20 20:55:30 UTC
Created attachment 27330 [details] [review]
[PASSED QA] Bug 9915: (follow-up) use SQL placeholders

This patch teaches C4::Reports::Guided::execute_query()
how to accept a list of query parameter values.  It then
follows-up on the main patch by simplifying how it converts
report parameters to a complete SQL query, and removes the
use of DBI->quote() and complicated regexes.

To test:

[1] Verify that using the OPAC svc/report service with
    sql_params continues to work.
[2] Verify that there are no regressions with running
    reports from the staff interface, both via the web
    service and the reports interface.
[3] Verify that prove -v /db_dependent/Reports_Guided.t passes.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
No regressions found.
Comment 25 Katrin Fischer 2014-04-20 22:09:46 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2014-04-20 22:10:04 UTC Comment hidden (obsolete)
Comment 27 Galen Charlton 2014-04-20 22:54:48 UTC
Pushed to master.  Thanks, Chris!