Bugzilla – Attachment 70592 Details for
Bug 9634
Allow for parameters re-use on SQL reports
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 9634: Allow for combining same paraneters in SQL reports
Bug-9634-Allow-for-combining-same-paraneters-in-SQ.patch (text/plain), 21.27 KB, created by
Nick Clemens
on 2018-01-17 02:41:26 UTC
(
hide
)
Description:
Bug 9634: Allow for combining same paraneters in SQL reports
Filename:
MIME Type:
Creator:
Nick Clemens
Created:
2018-01-17 02:41:26 UTC
Size:
21.27 KB
patch
obsolete
>From 7bc6d2fa54e98d259d8566b8b353a4bed654b3e5 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Mon, 15 Jan 2018 14:11:29 +0000 >Subject: [PATCH] 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 >--- > C4/Reports/Guided.pm | 10 +++--- > ...g9634_add_combine_params_option_to_reports.perl | 7 +++++ > .../en/modules/reports/guided_reports_start.tt | 27 ++++++++++++++++ > reports/guided_reports.pl | 36 ++++++++++++++++++++-- > t/db_dependent/Reports/Guided.t | 32 ++++++++++++++++++- > 5 files changed, 105 insertions(+), 7 deletions(-) > create mode 100644 installer/data/mysql/atomicupdate/bug9634_add_combine_params_option_to_reports.perl > >diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm >index d61b282..51d6795 100644 >--- a/C4/Reports/Guided.pm >+++ b/C4/Reports/Guided.pm >@@ -561,11 +561,12 @@ sub save_report { > my $subgroup = $fields->{subgroup}; > my $cache_expiry = $fields->{cache_expiry} || 300; > my $public = $fields->{public}; >+ my $combine_params = $fields->{combine_params} || 0; > > my $dbh = C4::Context->dbh(); > $sql =~ s/(\s*\;\s*)$//; # removes trailing whitespace and /;/ >- my $query = "INSERT INTO saved_sql (borrowernumber,date_created,last_modified,savedsql,report_name,report_area,report_group,report_subgroup,type,notes,cache_expiry,public) VALUES (?,now(),now(),?,?,?,?,?,?,?,?,?)"; >- $dbh->do($query, undef, $borrowernumber, $sql, $name, $area, $group, $subgroup, $type, $notes, $cache_expiry, $public); >+ my $query = "INSERT INTO saved_sql (borrowernumber,date_created,last_modified,savedsql,report_name,report_area,report_group,report_subgroup,type,notes,cache_expiry,public,combine_params) VALUES (?,now(),now(),?,?,?,?,?,?,?,?,?,?)"; >+ $dbh->do($query, undef, $borrowernumber, $sql, $name, $area, $group, $subgroup, $type, $notes, $cache_expiry, $public,$combine_params); > > my $id = $dbh->selectrow_array("SELECT max(id) FROM saved_sql WHERE borrowernumber=? AND report_name=?", undef, > $borrowernumber, $name); >@@ -582,6 +583,7 @@ sub update_sql { > my $subgroup = $fields->{subgroup}; > my $cache_expiry = $fields->{cache_expiry}; > my $public = $fields->{public}; >+ my $combine_params = $fields->{combine_params}; > > if( $cache_expiry >= 2592000 ){ > die "Please specify a cache expiry less than 30 days\n"; >@@ -589,8 +591,8 @@ sub update_sql { > > my $dbh = C4::Context->dbh(); > $sql =~ s/(\s*\;\s*)$//; # removes trailing whitespace and /;/ >- my $query = "UPDATE saved_sql SET savedsql = ?, last_modified = now(), report_name = ?, report_group = ?, report_subgroup = ?, notes = ?, cache_expiry = ?, public = ? WHERE id = ? "; >- $dbh->do($query, undef, $sql, $name, $group, $subgroup, $notes, $cache_expiry, $public, $id ); >+ my $query = "UPDATE saved_sql SET savedsql = ?, last_modified = now(), report_name = ?, report_group = ?, report_subgroup = ?, notes = ?, cache_expiry = ?, public = ?, combine_params = ? WHERE id = ? "; >+ $dbh->do($query, undef, $sql, $name, $group, $subgroup, $notes, $cache_expiry, $public, $combine_params, $id ); > } > > sub store_results { >diff --git a/installer/data/mysql/atomicupdate/bug9634_add_combine_params_option_to_reports.perl b/installer/data/mysql/atomicupdate/bug9634_add_combine_params_option_to_reports.perl >new file mode 100644 >index 0000000..ac56c79 >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/bug9634_add_combine_params_option_to_reports.perl >@@ -0,0 +1,7 @@ >+$DBversion = 'XXX'; # will be replaced by the RM >+if( CheckVersion( $DBversion ) ) { >+ $dbh->do( "ALTER TABLE saved_sql ADD COLUMN combine_params tinyint(1) NOT NULL DEFAULT 0" ); >+ >+ SetVersion( $DBversion ); >+ print "Upgrade to $DBversion done (Bug XXXXX - add combine params option to reports)\n"; >+} >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt >index 1d5e9f4..84a4efb 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt >@@ -165,6 +165,7 @@ canned reports and writing custom SQL reports.</p> > <th class="title-string">Last run</th> > <th class="report_public">Public</th> > <th class="report_json_url">JSON URL</th> >+ <th class="combine_params">Combine parameters</th> > [% IF (usecache) %] <th>Cache expiry (seconds)</th> [% END %] > <th>Saved results</th> > [% IF has_obsolete_reports %]<th>Update</th>[% END %] >@@ -209,6 +210,13 @@ canned reports and writing custom SQL reports.</p> > <a href="/cgi-bin/koha/svc/report?id=[% savedreport.id | uri %]">[% Koha.Preference('staffClientBaseURL') %]/cgi-bin/koha/svc/report?id=[% savedreport.id | html %]</a> > [% END %] > </td> >+ <td class="combine_params"> >+ [% IF (savedreport.combine_params) %] >+ Yes >+ [% ELSE %] >+ No >+ [% END %] >+ </td> > [% IF (usecache) %] <td>[% savedreport.cache_expiry %]</td> [% END %] > <td> > [% FOR result IN savedreport.results %] >@@ -316,6 +324,11 @@ canned reports and writing custom SQL reports.</p> > [% ELSE %] > <li><label for="public">Report is public:</label><select id="public" name="public"> <option value="0" selected="selected">No (default)</option> <option value="1">Yes</option> </select></li> > [% END %] >+[% IF (combine_params) %] >+ <li><label for="combine_params">Combine parameters:</label><select id="combine_params" name="combine_params"> <option value="0">No (default)</option> <option value="1" selected="selected">Yes</option> </select></li> >+[% ELSE %] >+ <li><label for="combine_params">Combine parameters:</label><select id="combine_params" name="combine_params"> <option value="0" selected="selected">No (default)</option> <option value="1">Yes</option> </select></li> >+[% END %] > [% IF (usecache) %] <li> > <label for="cache_expiry">Cache expiry:</label><input type="text" id="cache_expiry" name="cache_expiry" value="[% cache_expiry %]"></input> > <select id="cache_expiry_units" name="cache_expiry_units"> >@@ -668,6 +681,10 @@ canned reports and writing custom SQL reports.</p> > </li> > [% END %] > [% END %] >+ [% FOREACH key IN uniq_params.keys %] >+ <input type="hidden" name="param_name" value="[% key %]"> >+ <input type="hidden" name="param_value" value="[% uniq_params.$key %]"> >+ [% END %] > </ol> > </fieldset> > <fieldset class="action"><input type="submit" value="Run the report" /></fieldset> >@@ -737,6 +754,11 @@ $(document).ready(function() { > [% ELSE %] > <li><label for="public">Report is public:</label><select id="public" name="public"> <option value="0" selected="selected">No (default)</option> <option value="1">Yes</option> </select></li> > [% END %] >+[% IF (combine_params) %] >+ <li><label for="combine_params">Combine parameters:</label><select id="combine_params" name="combine_params"> <option value="0">No (default)</option> <option value="1" selected="selected">Yes</option> </select></li> >+[% ELSE %] >+ <li><label for="combine_params">Combine parameters:</label><select id="combine_params" name="combine_params"> <option value="0" selected="selected">No (default)</option> <option value="1">Yes</option> </select></li> >+[% END %] > [% IF (usecache) %] <li> > <label for="cache_expiry">Cache expiry:</label><input type="text" id="cache_expiry" name="cache_expiry" value="[% cache_expiry %]"></input> > <select id="cache_expiry_units" name="cache_expiry_units"> >@@ -826,6 +848,11 @@ Sub report:<select name="subreport"> > [% ELSE %] > <li><label for="public">Report is public:</label><select id="public" name="public"> <option value="0" selected="selected">No (default)</option> <option value="1">Yes</option> </select></li> > [% END %] >+[% IF (combine_params) %] >+ <li><label for="combine_params">Combine parameters:</label><select id="combine_params" name="combine_params"> <option value="0">No (default)</option> <option value="1" selected="selected">Yes</option> </select></li> >+[% ELSE %] >+ <li><label for="combine_params">Combine parameters:</label><select id="combine_params" name="combine_params"> <option value="0" selected="selected">No (default)</option> <option value="1">Yes</option> </select></li> >+[% END %] > [% IF (usecache) %] <li> > <label for="cache_expiry">Cache expiry:</label><input type="text" id="cache_expiry" name="cache_expiry" value="[% cache_expiry %]"></input> > <select id="cache_expiry_units" name="cache_expiry_units"> >diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl >index 106ddb5..32c0e09 100755 >--- a/reports/guided_reports.pl >+++ b/reports/guided_reports.pl >@@ -104,6 +104,7 @@ elsif ( $phase eq 'Build new' ) { > 'usecache' => $usecache, > 'cache_expiry' => 300, > 'public' => '0', >+ 'combine_params' => '0', > ); > } elsif ( $phase eq 'Use saved' ) { > >@@ -122,6 +123,7 @@ elsif ( $phase eq 'Build new' ) { > notes => $report->{notes}, > public => $report->{public}, > cache_expiry => $report->{cache_expiry}, >+ combine_params => $report->{combine_params}, > } > ); > $template->param( report_converted => $report->{report_name} ); >@@ -194,6 +196,7 @@ elsif ( $phase eq 'Edit SQL'){ > 'notes' => $report->{notes}, > 'id' => $id, > 'cache_expiry' => $report->{cache_expiry}, >+ 'combine_params' => $report->{combine_params}, > 'public' => $report->{public}, > 'usecache' => $usecache, > 'editsql' => 1, >@@ -208,6 +211,7 @@ elsif ( $phase eq 'Update SQL'){ > my $subgroup = $input->param('subgroup'); > my $notes = $input->param('notes'); > my $cache_expiry = $input->param('cache_expiry'); >+ my $combine_params = $input->param('combine_params'); > my $cache_expiry_units = $input->param('cache_expiry_units'); > my $public = $input->param('public'); > my $save_anyway = $input->param('save_anyway'); >@@ -259,6 +263,7 @@ elsif ( $phase eq 'Update SQL'){ > 'subgroup' => $subgroup, > 'notes' => $notes, > 'public' => $public, >+ 'combine_params' => $combine_params, > 'problematic_authvals' => $problematic_authvals, > 'warn_authval_problem' => 1, > 'phase_update' => 1 >@@ -274,6 +279,7 @@ elsif ( $phase eq 'Update SQL'){ > notes => $notes, > public => $public, > cache_expiry => $cache_expiry, >+ combine_params => $combine_params, > } ); > $template->param( > 'save_successful' => 1, >@@ -305,6 +311,7 @@ elsif ($phase eq 'retrieve results') { > elsif ( $phase eq 'Report on this Area' ) { > my $cache_expiry_units = $input->param('cache_expiry_units'), > my $cache_expiry = $input->param('cache_expiry'); >+ my $combine_params = $input->param('combine_params'); > > # we need to handle converting units > if( $cache_expiry_units eq "minutes" ){ >@@ -322,6 +329,7 @@ elsif ( $phase eq 'Report on this Area' ) { > 'build1' => 1, > 'areas' => get_report_areas(), > 'cache_expiry' => $cache_expiry, >+ 'combine_params' => $combine_params, > 'usecache' => $usecache, > 'public' => scalar $input->param('public'), > ); >@@ -332,6 +340,7 @@ elsif ( $phase eq 'Report on this Area' ) { > 'area' => scalar $input->param('area'), > 'types' => get_report_types(), > 'cache_expiry' => $cache_expiry, >+ 'combine_params' => $combine_params, > 'public' => scalar $input->param('public'), > ); > } >@@ -348,6 +357,7 @@ elsif ( $phase eq 'Choose this type' ) { > 'type' => $type, > columns => get_columns($area,$input), > 'cache_expiry' => scalar $input->param('cache_expiry'), >+ 'combine_params' => scalar $input->param('combine_params'), > 'public' => scalar $input->param('public'), > ); > } >@@ -372,6 +382,7 @@ elsif ( $phase eq 'Choose these columns' ) { > if ( $usecache ) { > $template->param( > cache_expiry => scalar $input->param('cache_expiry'), >+ combine_params => scalar $input->param('combine_params'), > cache_expiry_units => scalar $input->param('cache_expiry_units'), > ); > } >@@ -433,6 +444,7 @@ elsif ( $phase eq 'Choose these criteria' ) { > 'definition' => $definition, > 'criteriastring' => $query_criteria, > 'public' => scalar $input->param('public'), >+ 'combine_params' => scalar $input->param('combine_params'), > ); > if ( $usecache ) { > $template->param( >@@ -480,6 +492,7 @@ elsif ( $phase eq 'Choose these operations' ) { > 'totals' => $totals, > 'definition' => $definition, > 'cache_expiry' => scalar $input->param('cache_expiry'), >+ 'combine_params' => scalar $input->param('combine_params'), > 'public' => scalar $input->param('public'), > ); > >@@ -533,6 +546,7 @@ elsif ( $phase eq 'Build report' ) { > 'sql' => $sql, > 'type' => $type, > 'cache_expiry' => scalar $input->param('cache_expiry'), >+ 'combine_params' => scalar $input->param('combine_params'), > 'public' => scalar $input->param('public'), > ); > } >@@ -548,6 +562,7 @@ elsif ( $phase eq 'Save' ) { > 'sql' => $sql, > 'type' => $type, > 'cache_expiry' => scalar $input->param('cache_expiry'), >+ 'combine_params' => scalar $input->param('combine_params'), > 'public' => scalar $input->param('public'), > 'groups_with_subgroups' => groups_with_subgroups($area), # in case we have a report group that matches area > ); >@@ -563,6 +578,7 @@ elsif ( $phase eq 'Save Report' ) { > my $type = $input->param('types'); > my $notes = $input->param('notes'); > my $cache_expiry = $input->param('cache_expiry'); >+ my $combine_params = $input->param('combine_params'), > my $cache_expiry_units = $input->param('cache_expiry_units'); > my $public = $input->param('public'); > my $save_anyway = $input->param('save_anyway'); >@@ -601,6 +617,7 @@ elsif ( $phase eq 'Save Report' ) { > 'type' => $type, > 'notes' => $notes, > 'cache_expiry' => $cache_expiry, >+ 'combine_params' => $combine_params, > 'public' => $public, > ); > } else { >@@ -619,6 +636,7 @@ elsif ( $phase eq 'Save Report' ) { > 'type' => $type, > 'notes' => $notes, > 'public' => $public, >+ 'combine_params' => $combine_params, > 'problematic_authvals' => $problematic_authvals, > 'warn_authval_problem' => 1, > 'phase_save' => 1 >@@ -641,6 +659,7 @@ elsif ( $phase eq 'Save Report' ) { > type => $type, > notes => $notes, > cache_expiry => $cache_expiry, >+ combine_params => $combine_params, > public => $public, > } ); > logaction( "REPORTS", "ADD", $id, "$name | $sql" ) if C4::Context->preference("ReportsLog"); >@@ -659,6 +678,11 @@ elsif ($phase eq 'Run this report'){ > my $offset = 0; > my $report_id = $input->param('reports'); > my @sql_params = $input->multi_param('sql_params'); >+ my @param_name = $input->multi_param('param_name'); >+ my @param_value = $input->multi_param('param_value'); >+ my %lookup; >+ @lookup{@param_name} = @param_value; >+ > # offset algorithm > if ($input->param('page')) { > $offset = ($input->param('page') - 1) * $limit; >@@ -669,11 +693,12 @@ elsif ($phase eq 'Run this report'){ > 'report_id' => $report_id, > ); > >- my ( $sql, $original_sql, $type, $name, $notes ); >+ my ( $sql, $original_sql, $type, $name, $notes, $combine_params ); > if (my $report = get_saved_report($report_id)) { > $sql = $original_sql = $report->{savedsql}; > $name = $report->{report_name}; > $notes = $report->{notes}; >+ $combine_params = $report->{combine_params}; > > my @rows = (); > # if we have at least 1 parameter, and it's not filled, then don't execute but ask for parameters >@@ -682,8 +707,12 @@ elsif ($phase eq 'Run this report'){ > my @split = split /<<|>>/,$sql; > my @tmpl_parameters; > my @authval_errors; >+ my %uniq_params; > for(my $i=0;$i<($#split/2);$i++) { > my ($text,$authorised_value) = split /\|/,$split[$i*2+1]; >+ if( defined $uniq_params{$text."|".$authorised_value} && $combine_params ){ >+ next; >+ } else { $uniq_params{$text."|".$authorised_value} = "$i"; } > my $input; > my $labelid; > if ( not defined $authorised_value ) { >@@ -783,6 +812,7 @@ elsif ($phase eq 'Run this report'){ > 'auth_val_errors' => \@authval_errors, > 'enter_params' => 1, > 'reports' => $report_id, >+ 'uniq_params' => \%uniq_params, > ); > } else { > # OK, we have parameters, or there are none, we run the report >@@ -791,7 +821,8 @@ elsif ($phase eq 'Run this report'){ > my @split = split /<<|>>/,$sql; > my @tmpl_parameters; > for(my $i=0;$i<$#split/2;$i++) { >- my $quoted = $sql_params[$i]; >+ my $use_param = $combine_params ? $lookup{ $split[$i*2+1] } : $i; >+ my $quoted = $sql_params[$use_param]; > # if there are special regexp chars, we must \ them > $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g; > if ($split[$i*2+1] =~ /\|\s*date\s*$/) { >@@ -956,6 +987,7 @@ elsif ( $phase eq 'Create report from SQL' ) { > 'groups_with_subgroups' => groups_with_subgroups($group, $subgroup), > 'public' => '0', > 'cache_expiry' => 300, >+ 'combine_params' => 0, > 'usecache' => $usecache, > ); > } >diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t >index 0a2c734..e779089 100644 >--- a/t/db_dependent/Reports/Guided.t >+++ b/t/db_dependent/Reports/Guided.t >@@ -18,7 +18,7 @@ > > use Modern::Perl; > >-use Test::More tests => 9; >+use Test::More tests => 10; > use Test::Warn; > > use t::lib::TestBuilder; >@@ -354,6 +354,36 @@ count(h.reservedate) AS 'holds' > is( C4::Reports::Guided::convert_sql( $sql ), $expected_converted_sql, "Query with multiple instances of marcxml and biblioitems should have them all replaced"); > }; > >+subtest 'add combine_params column' => sub { >+ plan tests => 3; >+ >+ my $id = $builder->build({ source => 'Borrower' })->{ borrowernumber }; >+ >+ my $new_report_params = { >+ borrowernumber => $id, >+ sql => "SELECT 'CAT'", >+ name => "Kitty cat", >+ type => 1, >+ public => 0, >+ }; >+ >+ my $new_report_id = save_report($new_report_params); >+ my $saved_report = get_saved_report($new_report_id); >+ is( $saved_report->{combine_params},0,'When no combine params supplied to new report default is 0'); >+ >+ $new_report_params->{combine_params} = 1; >+ $new_report_id = save_report($new_report_params); >+ $saved_report = get_saved_report($new_report_id); >+ is( $saved_report->{combine_params},1,'When combine params supplied to new report, it is saved'); >+ >+ $saved_report->{combine_params} = 0; >+ $saved_report->{sql} = $saved_report->{savedsql}; #because we don't get back the same column name >+ update_sql($new_report_id,$saved_report); >+ $saved_report = get_saved_report($new_report_id); >+ is( $saved_report->{combine_params},0,'When combine params updated it is correctly saved'); >+ >+}; >+ > $schema->storage->txn_rollback; > > sub trim { >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 9634
:
70592
|
70685
|
70735
|
70736
|
73548
|
73549
|
73691
|
73802
|
73807