Bug 21215

Summary: Saved SQL reports code improvements [Omnibus]
Product: Koha Reporter: paxed <pasi.kallinen>
Component: ReportsAssignee: paxed <pasi.kallinen>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21763
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 24695    
Bug Blocks:    
Attachments: Bug 21215: Saved SQL reports code improvements
Bug 21215: Saved SQL reports code improvements
Bug 21215: Saved SQL reports code improvements
Bug 21215: Saved SQL reports code improvements
Bug 21215: Saved SQL reports code improvements
Bug 21215: Saved SQL reports code improvements

Description paxed 2018-08-14 05:51:31 UTC
The code for the saved SQL reports is a bit warty - For example, there's the same code that splits the SQL and does the parameter parsing in multiple places, when it should be in the module.
Comment 1 paxed 2018-08-14 05:56:09 UTC
Created attachment 77776 [details] [review]
Bug 21215: Saved SQL reports code improvements

The saved SQL reports code is a bit warty, so improve it:

Instead of using the same code in multiple places to split the SQL
move that code into the Guided.pm module

Add several tests for parameter substitution

Make it much easier to add more parameter types for substitution.

Test plan:

1) Run your saved SQL reports and check that they work correctly
2) Create new SQL reports and check that the parameters are queried
   correctly.
3) Prove the t/db_dependent/Reports/Guided.t
Comment 2 paxed 2018-08-14 12:04:05 UTC
Created attachment 77781 [details] [review]
Bug 21215: Saved SQL reports code improvements

The saved SQL reports code is a bit warty, so improve it:

Instead of using the same code in multiple places to split the SQL
move that code into the Guided.pm module.

Add several tests for parameter substitution and SQL validation.

Make it much easier to add more parameter types for substitution.

Test plan:

1) Run your saved SQL reports and check that they work correctly
2) Create new SQL reports and check that the parameters are queried
   correctly.
3) Prove the t/db_dependent/Reports/Guided.t
Comment 3 Michal Denar 2018-09-18 20:27:42 UTC
Created attachment 79060 [details] [review]
Bug 21215: Saved SQL reports code improvements

The saved SQL reports code is a bit warty, so improve it:

Instead of using the same code in multiple places to split the SQL
move that code into the Guided.pm module.

Add several tests for parameter substitution and SQL validation.

Make it much easier to add more parameter types for substitution.

Test plan:

1) Run your saved SQL reports and check that they work correctly
2) Create new SQL reports and check that the parameters are queried
   correctly.
3) Prove the t/db_dependent/Reports/Guided.t

Signed-off-by: Pasi Kallinen <pasi.kallinen@joensuu.fi>
Signed-off-by:  Michal Denar <black23@gmail.coml>
Comment 4 Martin Renvoize 2018-10-12 14:54:17 UTC
Lots of tabs introduced so we're failing the QA scripts I'm afraid..

Otherwise I like it.. please fix the tabs

Failed QA
Comment 5 paxed 2018-10-15 04:49:09 UTC
(In reply to Martin Renvoize from comment #4)
> Lots of tabs introduced so we're failing the QA scripts I'm afraid..
> 
> Otherwise I like it.. please fix the tabs
> 
> Failed QA

Whoops. A misconfigured text editor in the koha container was the culprit. Will fix.
Comment 6 paxed 2018-10-15 04:49:43 UTC
Created attachment 80585 [details] [review]
Bug 21215: Saved SQL reports code improvements


The saved SQL reports code is a bit warty, so improve it:

Instead of using the same code in multiple places to split the SQL
move that code into the Guided.pm module.

Add several tests for parameter substitution and SQL validation.

Make it much easier to add more parameter types for substitution.

Test plan:

1) Run your saved SQL reports and check that they work correctly
2) Create new SQL reports and check that the parameters are queried
   correctly.
3) Prove the t/db_dependent/Reports/Guided.t
Comment 7 Martin Renvoize 2018-10-17 13:54:27 UTC
Created attachment 80710 [details] [review]
Bug 21215: Saved SQL reports code improvements

The saved SQL reports code is a bit warty, so improve it:

Instead of using the same code in multiple places to split the SQL
move that code into the Guided.pm module.

Add several tests for parameter substitution and SQL validation.

Make it much easier to add more parameter types for substitution.

Test plan:

1) Run your saved SQL reports and check that they work correctly
2) Create new SQL reports and check that the parameters are queried
   correctly.
3) Prove the t/db_dependent/Reports/Guided.t

Signed-off-by: Pasi Kallinen <pasi.kallinen@joensuu.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2018-10-17 13:58:13 UTC
Created attachment 80711 [details] [review]
Bug 21215: Saved SQL reports code improvements

The saved SQL reports code is a bit warty, so improve it:

Instead of using the same code in multiple places to split the SQL
move that code into the Guided.pm module.

Add several tests for parameter substitution and SQL validation.

Make it much easier to add more parameter types for substitution.

Test plan:

1) Run your saved SQL reports and check that they work correctly
2) Create new SQL reports and check that the parameters are queried
   correctly.
3) Prove the t/db_dependent/Reports/Guided.t

Signed-off-by: Pasi Kallinen <pasi.kallinen@joensuu.fi>
Signed-off-by: Michal Denar <black23@gmail.coml>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2018-10-17 13:59:05 UTC
Thanks for the quick followup.

Works well, passes all tests and no regressions found.

Passing QA
Comment 10 Nick Clemens (kidclamp) 2018-11-13 14:58:42 UTC
This is not working for me with reports with parameters

SELECT itemcallnumber, barcode
FROM items
WHERE barcode LIKE <<Barcode>>

If i enter "%9" I get results before the patch, but none  after

SELECT <<cat>>,<<dog>>,<<cat>>

I get two input boxes, enter "this", "that"

I get NULL, NULL NULL
Comment 11 Nick Clemens (kidclamp) 2018-11-13 15:01:20 UTC
I also added a see also to bug 21763 - I would strongly prefer this new code to go into Koha::Report(s) and not the C4 namespace
Comment 12 paxed 2020-02-20 09:43:46 UTC
Splitting this up into multiple parts. One is bug 24695
Comment 13 Katrin Fischer 2020-03-15 16:55:44 UTC
(In reply to paxed from comment #12)
> Splitting this up into multiple parts. One is bug 24695

Splitting is a good idea - always a bit scared of 'enhancements to...' bugs :)