Bug 9634 - Allow for parameters re-use on SQL reports
Summary: Allow for parameters re-use on SQL reports
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2013-02-15 17:56 UTC by Tomás Cohen Arazi
Modified: 2018-05-14 19:27 UTC (History)
10 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:
This new feature allows parameters to be re-used in reports. When a report asks for two variables using the same name and type/authorised value they will be combined into a single input field on the form. i.e. SELECT * FROM items WHERE homebranch=<<Branchcode|branches>> AND holdingbranch=<<Branchcode|branches>> AND itype=<<Item type|itemtypes>> Will ask for only 2 parameters when run, Branchcode and Item type.


Attachments
Bug 9634: Allow for combining same paraneters in SQL reports (21.27 KB, patch)
2018-01-17 02:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9634 [QA Followup] - Add new column to kohastructure.sql (987 bytes, patch)
2018-01-18 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9634: Allow for combining same paraneters in SQL reports (21.32 KB, patch)
2018-01-19 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 9634 [QA Followup] - Add new column to kohastructure.sql (1.01 KB, patch)
2018-01-19 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 9634: Allow for combining same paraneters in SQL reports (7.04 KB, patch)
2018-04-03 10:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9634: Allow for combining same paraneters in SQL reports (6.81 KB, patch)
2018-04-03 10:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9634: Allow for combining same paraneters in SQL reports (6.90 KB, patch)
2018-04-05 13:58 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 9634: Allow for combining same paraneters in SQL reports (6.96 KB, patch)
2018-04-06 21:10 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 9634: Allow for combining same paraneters in SQL reports (7.01 KB, patch)
2018-04-07 08:02 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 Tomás Cohen Arazi 2013-02-15 17:56:33 UTC
If a parameter is used several times on a report, don't ask for him for each occurence. Detect duplicate name and re-use.
Comment 1 Tomás Cohen Arazi 2013-02-22 18:16:55 UTC
Parameters are passed through CGI in a way they are processed in order. A way of mapping parameter value to the pair (name,authval) (kind of using it as an ID) is needed for the task.
Comment 2 Nick Clemens 2018-01-17 02:41:26 UTC
Created attachment 70592 [details] [review]
Bug 9634: Allow for combining same paraneters in SQL reports

This patch set adds a new colum to saved_sql 'combine_params'

If set, parameters that have the same name and data type will only be
asked for once from the user

To test:
 1 - Create a report that takes multiple parameters, e.g.:
SELECT <<cat>> AS one, <<dog|branches>> AS two, <<dog|YES_NO>> as three,
       <<cat>> as four, <<dog|branches>> as five
 2 - Run this report, note you are asked for five parameters
 3 - Apply patch
 4 - Update database
 5 - Run report, note you are still asked for five parameters
 6 - Edit report, note the 'Combine params' options, say YES!!!!!
 7 - Re-run the report, you shoudl be asked for three parameters
 8 - Verify the results reflect the supplied parameters
 9 - Create a new report (perhaps something more substantive)
10 - Save with 'combine params' checke
11 - View in saved reports and see the new 'combine params' column
12 - Verify it is correctly set
13 - Run the report and verify correct results
14 - prove -v t/db_dependent/Reports/Guided.t
Comment 3 M. Tompsett 2018-01-17 23:00:26 UTC
Failed to include update for fresh install, only upgrade alter table.
Comment 4 Kyle M Hall 2018-01-18 11:56:27 UTC
Created attachment 70685 [details] [review]
Bug 9634 [QA Followup] - Add new column to kohastructure.sql
Comment 5 M. Tompsett 2018-01-19 04:03:37 UTC
Created attachment 70735 [details] [review]
Bug 9634: Allow for combining same paraneters in SQL reports

This patch set adds a new colum to saved_sql 'combine_params'

If set, parameters that have the same name and data type will only be
asked for once from the user

To test:
 1 - Create a report that takes multiple parameters, e.g.:
SELECT <<cat>> AS one, <<dog|branches>> AS two, <<dog|YES_NO>> as three,
       <<cat>> as four, <<dog|branches>> as five
 2 - Run this report, note you are asked for five parameters
 3 - Apply patch
 4 - Update database
 5 - Run report, note you are still asked for five parameters
 6 - Edit report, note the 'Combine params' options, say YES!!!!!
 7 - Re-run the report, you shoudl be asked for three parameters
 8 - Verify the results reflect the supplied parameters
 9 - Create a new report (perhaps something more substantive)
10 - Save with 'combine params' checke
11 - View in saved reports and see the new 'combine params' column
12 - Verify it is correctly set
13 - Run the report and verify correct results
14 - prove -v t/db_dependent/Reports/Guided.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 M. Tompsett 2018-01-19 04:03:41 UTC
Created attachment 70736 [details] [review]
Bug 9634 [QA Followup] - Add new column to kohastructure.sql

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Julian Maurice 2018-03-02 13:04:26 UTC
Is the new option really necessary ?
I can't imagine a use case where someone would like to not use this feature. And if users really want to repeat themselves they can just rename one of the parameters.
Comment 8 Nick Clemens 2018-03-02 13:37:06 UTC
I added it only for old reports, if a library has been using a report with duplicated inputs this will be a change in behavior and may be unexpected/unwanted.

Am happy to reconsider or explore alternate options, this seemed the simplest at the time
Comment 9 Julian Maurice 2018-03-02 14:03:02 UTC
(In reply to Nick Clemens from comment #8)
> I added it only for old reports, if a library has been using a report with
> duplicated inputs this will be a change in behavior and may be
> unexpected/unwanted.
> 
> Am happy to reconsider or explore alternate options, this seemed the
> simplest at the time

I hadn't thought about old reports. It makes sense... but what do you think about renaming all parameters (appending a number for instance) of existing reports in an updatedatabase script ?
Comment 10 Nick Clemens 2018-03-02 15:07:27 UTC
That works for me, Jonathan or Katrin, any input? If it sounds good I will try to work on that or you can feel free Julian
Comment 11 Julian Maurice 2018-03-09 13:00:18 UTC
(In reply to Nick Clemens from comment #10)
> That works for me, Jonathan or Katrin, any input? If it sounds good I will
> try to work on that or you can feel free Julian

Changing status to ASSIGNED. Feel free to change it back directly to Signed off once the patch is ready.
Comment 12 Katrin Fischer 2018-04-01 12:17:38 UTC
What I think is:

Don't make the option, because the chance of messing up existing reports is very very slim. 

They will remain working as they work now if parameters are named differently. If there exist parameters who already are named the exactly same, the patch will take effect, but then we can also assume that this is indeed the wanted behaviour.

Of course we should add a nice note to the release notes to highlight the new feature!
Comment 13 Nick Clemens 2018-04-03 10:37:27 UTC Comment hidden (obsolete)
Comment 14 Nick Clemens 2018-04-03 10:39:26 UTC Comment hidden (obsolete)
Comment 15 Nick Clemens 2018-04-03 10:42:13 UTC Comment hidden (obsolete)
Comment 16 Nick Clemens 2018-04-03 10:43:42 UTC
Created attachment 73549 [details] [review]
Bug 9634: Allow for combining same paraneters in SQL reports

This patch combines report paramters with the same name and data type
(authorised value)

This presereves the past ability to provide all parameters as
'sql_params' in order to preserve existing public report links

To test:
 1 - Create a report that takes multiple parameters, e.g.:
SELECT <<cat>> AS one, <<dog|branches>> AS two, <<dog|YES_NO>> as three,
       <<cat>> as four, <<dog|branches>> as five
 2 - Run this report, note you are asked for five parameters
 3 - Apply patch
 4 - Update database
 5 - Run report, note you are asked for three params
 6 - Verify the results reflect the supplied parameters
 7 - Export the report and verify results
 8 - Alter the URL and provide 5 params as sql_params and no param_name
 variables i.e.
 http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run+this+report&sql_params=Banana&sql_params=CPL&sql_params=0&sql_params=Orange&sql_params=LPL
 9 - Verify results come out as expected
Comment 17 Victor Grousset/tuxayo 2018-04-05 13:58:53 UTC
Created attachment 73691 [details] [review]
Bug 9634: Allow for combining same paraneters in SQL reports

This patch combines report paramters with the same name and data type
(authorised value)

This presereves the past ability to provide all parameters as
'sql_params' in order to preserve existing public report links

To test:
 1 - Create a report that takes multiple parameters, e.g.:
SELECT <<cat>> AS one, <<dog|branches>> AS two, <<dog|YES_NO>> as three,
       <<cat>> as four, <<dog|branches>> as five
 2 - Run this report, note you are asked for five parameters
 3 - Apply patch
 4 - Update database
 5 - Run report, note you are asked for three params
 6 - Verify the results reflect the supplied parameters
 7 - Export the report and verify results
 8 - Alter the URL and provide 5 params as sql_params and no param_name
 variables i.e.
 http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run+this+report&sql_params=Banana&sql_params=CPL&sql_params=0&sql_params=Orange&sql_params=LPL
 9 - Verify results come out as expected. i.e. 5 different values that
       are those of the URL.

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 18 Charles Farmer 2018-04-06 21:10:20 UTC
Created attachment 73802 [details] [review]
Bug 9634: Allow for combining same paraneters in SQL reports

This patch combines report paramters with the same name and data type
(authorised value)

This presereves the past ability to provide all parameters as
'sql_params' in order to preserve existing public report links

To test:
 1 - Create a report that takes multiple parameters, e.g.:
SELECT <<cat>> AS one, <<dog|branches>> AS two, <<dog|YES_NO>> as three,
       <<cat>> as four, <<dog|branches>> as five
 2 - Run this report, note you are asked for five parameters
 3 - Apply patch
 4 - Update database
 5 - Run report, note you are asked for three params
 6 - Verify the results reflect the supplied parameters
 7 - Export the report and verify results
 8 - Alter the URL and provide 5 params as sql_params and no param_name
 variables i.e.
 http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run+this+report&sql_params=Banana&sql_params=CPL&sql_params=0&sql_params=Orange&sql_params=LPL
 9 - Verify results come out as expected. i.e. 5 different values that
       are those of the URL.

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 19 Katrin Fischer 2018-04-07 08:02:10 UTC
Stole this one from you Julian, hope it's ok!
Comment 20 Katrin Fischer 2018-04-07 08:02:51 UTC
Created attachment 73807 [details] [review]
Bug 9634: Allow for combining same paraneters in SQL reports

This patch combines report paramters with the same name and data type
(authorised value)

This presereves the past ability to provide all parameters as
'sql_params' in order to preserve existing public report links

To test:
 1 - Create a report that takes multiple parameters, e.g.:
SELECT <<cat>> AS one, <<dog|branches>> AS two, <<dog|YES_NO>> as three,
       <<cat>> as four, <<dog|branches>> as five
 2 - Run this report, note you are asked for five parameters
 3 - Apply patch
 4 - Update database
 5 - Run report, note you are asked for three params
 6 - Verify the results reflect the supplied parameters
 7 - Export the report and verify results
 8 - Alter the URL and provide 5 params as sql_params and no param_name
 variables i.e.
 http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run+this+report&sql_params=Banana&sql_params=CPL&sql_params=0&sql_params=Orange&sql_params=LPL
 9 - Verify results come out as expected. i.e. 5 different values that
       are those of the URL.

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Jonathan Druart 2018-04-11 19:55:05 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 22 Nick Clemens 2018-05-14 19:27:44 UTC
Enhancement, not backported for 17.11.x