Bug 21215 - Saved SQL reports code improvements [Omnibus]
Summary: Saved SQL reports code improvements [Omnibus]
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: paxed
QA Contact: Testopia
URL:
Keywords:
Depends on: 24695
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-14 05:51 UTC by paxed
Modified: 2020-03-15 16:55 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21215: Saved SQL reports code improvements (23.89 KB, patch)
2018-08-14 05:56 UTC, paxed
Details | Diff | Splinter Review
Bug 21215: Saved SQL reports code improvements (27.42 KB, patch)
2018-08-14 12:04 UTC, paxed
Details | Diff | Splinter Review
Bug 21215: Saved SQL reports code improvements (27.44 KB, patch)
2018-09-18 20:27 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21215: Saved SQL reports code improvements (27.65 KB, patch)
2018-10-15 04:49 UTC, paxed
Details | Diff | Splinter Review
Bug 21215: Saved SQL reports code improvements (27.69 KB, patch)
2018-10-17 13:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21215: Saved SQL reports code improvements (27.74 KB, patch)
2018-10-17 13:58 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 :)