From cdc5e11159c6c3dba8ae371094633feb5e64b57d Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 19 Feb 2013 15:58:53 -0300 Subject: [PATCH] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. Use cases are: - The user creates a report from SQL a) Uses bad authorised values b) Clicks 'Save Report' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Save Report' - The user edits an already saved report (Update SQL) a) Uses bad authorised values b) Clicks 'Update SQL' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Update SQL' - The user tries to run a saved report that contains bad authorised values, Koha advertises the problem and provides the user with a button 'Edit SQL' to fix things. To test, just create a report from SQL using invalid authorised values like this (misspelled 'branch'): SELECT * FROM itemtypes WHERE hola=<> AND hola2=<> Regards To+ Notes: - I added several comments on the code. - Fixed an annoying warning of uninitialised variable also (refactored some tiny bits to do it). - Added the following methods - C4::Reports::Guided::GetReservedAuthorisedValues - C4::Reports::Guided::GetParametersFromSQL - C4::Reports::Guided::IsAuthorisedValueValid - C4::Reports::Guided::ValidateSQLParameters - C4::Koha::IsAuthorisedValueCategory - Those methods could have been used to refactor this guided reports code as its *a bit messy*. I chose to do it in a new bug of course :-D. - Fixed some trivial perlcritic -5 errors - Removed some debugging stuff left by mistake - Fixed some POD problems - Optimal SQL-driven IsAuthorisedValueCategory method - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Bernardo Gonzalez Kriegel Comment: Work as described. No koha-qa errors. Test: Tried with examples (from help and test plan) reports, correctly identifies invalid authorized values, and no problem with authorized ones. NOTE: Online help for this does not states that partial values need to be between '%' in a SQLish way. Perhaps this could be addressed inserting % in values or adding a checkbox (partial|exact). Or changing help. Signed-off-by: Katrin Fischer Passes all tests and QA script. Signed-off-by: Jared Camins-Esakov --- C4/Koha.pm | 23 +++ C4/Reports/Guided.pm | 127 +++++++++++-- .../modules/reports/guided_reports_start.tt | 87 +++++++-- reports/guided_reports.pl | 170 +++++++++++++----- 4 files changed, 328 insertions(+), 79 deletions(-) diff --git a/C4/Koha.pm b/C4/Koha.pm index 6869ab0778..3648dccd61 100644 --- a/C4/Koha.pm +++ b/C4/Koha.pm @@ -57,6 +57,7 @@ BEGIN { &getitemtypeimagelocation &GetAuthorisedValues &GetAuthorisedValueCategories + &IsAuthorisedValueCategory &GetKohaAuthorisedValues &GetKohaAuthorisedValuesFromField &GetKohaAuthorisedValueLib @@ -1105,6 +1106,28 @@ sub GetAuthorisedValueCategories { return \@results; } +=head2 IsAuthorisedValueCategory + + $is_auth_val_category = IsAuthorisedValueCategory($category); + +Returns whether a given category name is a valid one + +=cut + +sub IsAuthorisedValueCategory { + my $category = shift; + my $query = ' + SELECT category + FROM authorised_values + WHERE BINARY category=? + LIMIT 1 + '; + my $sth = C4::Context->dbh->prepare($query); + $sth->execute($category); + $sth->fetchrow ? return 1 + : return 0; +} + =head2 GetAuthorisedValueByCode $authhorised_value = GetAuthorisedValueByCode( $category, $authvalcode ); diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index 279ac57d89..f0db7a3719 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -45,6 +45,10 @@ BEGIN { get_column_type get_distinct_values save_dictionary get_from_dictionary delete_definition delete_report format_results get_sql nb_rows update_sql build_authorised_value_list + GetReservedAuthorisedValues + GetParametersFromSQL + IsAuthorisedValueValid + ValidateSQLParameters ); } @@ -62,9 +66,7 @@ C4::Reports::Guided - Module for generating guided reports =head1 METHODS -=over 2 - -=item get_report_areas() +=head2 get_report_areas This will return a list of all the available report areas @@ -129,7 +131,7 @@ if ( C4::Context->preference('item-level_itypes') ) { unshift @{ $criteria{'CAT'} }, 'biblioitems.itemtype'; } -=item get_report_types() +=head2 get_report_types This will return a list of all the available report types @@ -151,7 +153,7 @@ sub get_report_types { } -=item get_report_groups() +=head2 get_report_groups This will return a list of all the available report areas with groups @@ -180,7 +182,7 @@ sub get_report_groups { return \%groups_with_subgroups } -=item get_all_tables() +=head2 get_all_tables This will return a list of all tables in the database @@ -200,7 +202,7 @@ sub get_all_tables { } -=item get_columns($area) +=head2 get_columns($area) This will return a list of all columns for a report area @@ -244,7 +246,7 @@ sub _get_columns { return (@columns); } -=item build_query($columns,$criteria,$orderby,$area) +=head2 build_query($columns,$criteria,$orderby,$area) This will build the sql needed to return the results asked for, $columns is expected to be of the format tablename.columnname. @@ -316,7 +318,7 @@ sub _build_query { return ($query); } -=item get_criteria($area,$cgi); +=head2 get_criteria($area,$cgi); Returns an arraref to hashrefs suitable for using in a tmpl_loop. With the criteria and available values. @@ -388,7 +390,7 @@ sub get_criteria { return ( \@criteria_array ); } -sub nb_rows($) { +sub nb_rows { my $sql = shift or return; my $sth = C4::Context->dbh->prepare($sql); $sth->execute(); @@ -396,7 +398,7 @@ sub nb_rows($) { return scalar (@$rows); } -=item execute_query +=head2 execute_query ($results, $error) = execute_query($sql, $offset, $limit) @@ -422,7 +424,7 @@ the user in a user-supplied SQL query WILL apply in any case. # ~ remove any LIMIT clause # ~ repace SELECT clause w/ SELECT count(*) -sub select_2_select_count ($) { +sub select_2_select_count { # Modify the query passed in to create a count query... (I think this covers all cases -crn) my ($sql) = strip_limit(shift) or return; $sql =~ s/\bSELECT\W+(?:\w+\W+){1,}?FROM\b|\bSELECT\W\*\WFROM\b/SELECT count(*) FROM /ig; @@ -512,7 +514,7 @@ sub execute_query ($;$$$) { # store_results($id,$xml); } -=item save_report($sql,$name,$type,$notes) +=head2 save_report($sql,$name,$type,$notes) Given some sql and a name this will saved it so that it can reused Returns id of the newly created report @@ -700,7 +702,7 @@ sub get_saved_report { return $dbh->selectrow_hashref($query, undef, $report_arg); } -=item create_compound($masterID,$subreportID) +=head2 create_compound($masterID,$subreportID) This will take 2 reports and create a compound report using both of them @@ -730,7 +732,7 @@ sub create_compound { return ( $mastertables, $subtables ); } -=item get_column_type($column) +=head2 get_column_type($column) This takes a column name of the format table.column and will return what type it is (free text, set values, date) @@ -758,7 +760,7 @@ sub get_column_type { } } -=item get_distinct_values($column) +=head2 get_distinct_values($column) Given a column name, return an arrary ref of hashrefs suitable for use as a tmpl_loop with the distinct values of the column @@ -852,7 +854,7 @@ sub _get_column_defs { return \%columns; } -=item build_authorised_value_list($authorised_value) +=head2 build_authorised_value_list($authorised_value) Returns an arrayref - hashref pair. The hashref consists of various code => name lists depending on the $authorised_value. @@ -917,11 +919,98 @@ sub build_authorised_value_list { return (\@authorised_values, \%authorised_lib); } +=head2 GetReservedAuthorisedValues + + my %reserved_authorised_values = GetReservedAuthorisedValues(); + +Returns a hash containig all reserved words + +=cut + +sub GetReservedAuthorisedValues { + my %reserved_authorised_values = + map { $_ => 1 } ( 'date', + 'branches', + 'itemtypes', + 'cn_source', + 'categorycode' ); + + return \%reserved_authorised_values; +} + + +=head2 IsAuthorisedValueValid + + my $is_valid_ath_value = IsAuthorisedValueValid($authorised_value) + +Returns 1 if $authorised_value is on the reserved authorised values list or +in the authorised value categories defined in + +=cut + +sub IsAuthorisedValueValid { + + my $authorised_value = shift; + my $reserved_authorised_values = GetReservedAuthorisedValues(); + + if ( exists $reserved_authorised_values->{$authorised_value} || + IsAuthorisedValueCategory($authorised_value) ) { + return 1; + } + + return 0; +} + +=head2 GetParametersFromSQL + + my @sql_parameters = GetParametersFromSQL($sql) + +Returns an arrayref of hashes containing the keys name and authval + +=cut + +sub GetParametersFromSQL { + + my $sql = shift ; + my @split = split(/<<|>>/,$sql); + my @sql_parameters = (); + + for ( my $i = 0; $i < ($#split/2) ; $i++ ) { + my ($name,$authval) = split(/\|/,$split[$i*2+1]); + push @sql_parameters, { 'name' => $name, 'authval' => $authval }; + } + + return \@sql_parameters; +} + +=head2 ValidateSQLParameters + + my @problematic_parameters = ValidateSQLParameters($sql) + +Returns an arrayref of hashes containing the keys name and authval of +those SQL parameters that do not correspond to valid authorised names + +=cut + +sub ValidateSQLParameters { + + my $sql = shift; + my @problematic_parameters = (); + my $sql_parameters = GetParametersFromSQL($sql); + + foreach my $sql_parameter (@$sql_parameters) { + if ( defined $sql_parameter->{'authval'} ) { + push @problematic_parameters, $sql_parameter unless + IsAuthorisedValueValid($sql_parameter->{'authval'}); + } + } + + return \@problematic_parameters; +} + 1; __END__ -=back - =head1 AUTHOR Chris Cormack 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 330a7cda49..cf38cbf752 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 @@ -654,28 +654,93 @@ canned reports and writing custom SQL reports.

[% END %] +[% IF ( warn_authval_problem ) %] +
+ + + + + + + + + + + +
+

Errors found when processing parameters for report: [% name %]

+ [% FOREACH problematic_authval IN problematic_authvals %] +

+ [% problematic_authval.name %]: The authorised value category ([% problematic_authval.authval %]) + you selected does not exist. +

+ [% END %] + + + + + + + + + + + + + [% IF ( phase_update) %] + + + [% ELSIF ( phase_save) %] + + + + [% END %] + + +
+ +
+
+ +[% END %] + [% IF ( enter_params ) %]
- + [% IF ( auth_val_error ) %] + +
+

Errors found when processing parameters for report: [% name %]

+ [% FOREACH auth_val_error IN auth_val_errors %] +

+ [% auth_val_error.entry %]: The authorised value category ([% auth_val_error.auth_val %]) + you selected does not exist. +

+ [% END %] +
+
+ [% ELSE %] +

Enter parameters for report [% name %]:

[% IF ( notes ) %]

[% notes %]

[% END %]
    - [% FOREACH sql_param IN sql_params %] - [% IF sql_param.input == 'date' %] -
  1. - -
    2. - [% ELSIF ( sql_param.input == 'text' ) %] -
    3. - [% ELSE %] -
  3. [% sql_param.input %]
    4. + [% FOREACH sql_param IN sql_params %] + [% IF sql_param.input == 'date' %] +
  4. + +
    5. + [% ELSIF ( sql_param.input == 'text' ) %] +
    6. + [% ELSE %] +
  6. [% sql_param.input %]
    7. + [% END %] [% END %] - [% END %]
+ [% END %]
[% END %] diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index c25767a2fa..52539f5c6a 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -27,6 +27,7 @@ use C4::Output; use C4::Dates qw/format_date/; use C4::Debug; use C4::Branch; # XXX subfield_is_koha_internal_p +use C4::Koha qw/IsAuthorisedValueCategory/; =head1 NAME @@ -131,7 +132,6 @@ elsif ( $phase eq 'Show SQL'){ } elsif ( $phase eq 'Edit SQL'){ - my $id = $input->param('reports'); my $report = get_saved_report($id); my $group = $report->{report_group}; @@ -159,6 +159,7 @@ elsif ( $phase eq 'Update SQL'){ my $cache_expiry = $input->param('cache_expiry'); my $cache_expiry_units = $input->param('cache_expiry_units'); my $public = $input->param('public'); + my $save_anyway = $input->param('save_anyway'); my @errors; @@ -185,26 +186,52 @@ elsif ( $phase eq 'Update SQL'){ elsif ($sql !~ /^(SELECT)/i) { push @errors, {queryerr => 1}; } + if (@errors) { $template->param( 'errors' => \@errors, 'sql' => $sql, ); } else { - update_sql( $id, { - sql => $sql, - name => $reportname, - group => $group, - subgroup => $subgroup, - notes => $notes, - cache_expiry => $cache_expiry, - public => $public, - } ); - $template->param( - 'save_successful' => 1, - 'reportname' => $reportname, - 'id' => $id, - ); + + # Check defined SQL parameters for authorised value validity + my $problematic_authvals = ValidateSQLParameters($sql); + + if ( scalar @$problematic_authvals > 0 && not $save_anyway ) { + # There's at least one problematic parameter, report to the + # GUI and provide all user input for further actions + $template->param( + 'id' => $id, + 'sql' => $sql, + 'reportname' => $reportname, + 'group' => $group, + 'subgroup' => $subgroup, + 'notes' => $notes, + 'cache_expiry' => $cache_expiry, + 'cache_expiry_units' => $cache_expiry_units, + 'public' => $public, + 'problematic_authvals' => $problematic_authvals, + 'warn_authval_problem' => 1, + 'phase_update' => 1 + ); + + } else { + # No params problem found or asked to save anyway + update_sql( $id, { + sql => $sql, + name => $reportname, + group => $group, + subgroup => $subgroup, + notes => $notes, + cache_expiry => $cache_expiry, + public => $public, + } ); + $template->param( + 'save_successful' => 1, + 'reportname' => $reportname, + 'id' => $id, + ); + } } } @@ -467,6 +494,7 @@ elsif ( $phase eq 'Save Report' ) { my $cache_expiry = $input->param('cache_expiry'); my $cache_expiry_units = $input->param('cache_expiry_units'); my $public = $input->param('public'); + my $save_anyway = $input->param('save_anyway'); # if we have the units, then we came from creating a report from SQL and thus need to handle converting units @@ -493,6 +521,7 @@ elsif ( $phase eq 'Save Report' ) { elsif ($sql !~ /^(SELECT)/i) { push @errors, {queryerr => "No SELECT"}; } + if (@errors) { $template->param( 'errors' => \@errors, @@ -503,25 +532,48 @@ elsif ( $phase eq 'Save Report' ) { 'cache_expiry' => $cache_expiry, 'public' => $public, ); - } - else { - my $id = save_report( { - borrowernumber => $borrowernumber, - sql => $sql, - name => $name, - area => $area, - group => $group, - subgroup => $subgroup, - type => $type, - notes => $notes, - cache_expiry => $cache_expiry, - public => $public, - } ); - $template->param( - 'save_successful' => 1, - 'reportname' => $name, - 'id' => $id, - ); + } else { + # Check defined SQL parameters for authorised value validity + my $problematic_authvals = ValidateSQLParameters($sql); + + if ( scalar @$problematic_authvals > 0 && not $save_anyway ) { + # There's at least one problematic parameter, report to the + # GUI and provide all user input for further actions + $template->param( + 'area' => $area, + 'group' => $group, + 'subgroup' => $subgroup, + 'sql' => $sql, + 'reportname' => $name, + 'type' => $type, + 'notes' => $notes, + 'cache_expiry' => $cache_expiry, + 'cache_expiry_units' => $cache_expiry_units, + 'public' => $public, + 'problematic_authvals' => $problematic_authvals, + 'warn_authval_problem' => 1, + 'phase_save' => 1 + ); + } else { + # No params problem found or asked to save anyway + my $id = save_report( { + borrowernumber => $borrowernumber, + sql => $sql, + name => $name, + area => $area, + group => $group, + subgroup => $subgroup, + type => $type, + notes => $notes, + cache_expiry => $cache_expiry, + public => $public, + } ); + $template->param( + 'save_successful' => 1, + 'reportname' => $name, + 'id' => $id, + ); + } } } @@ -553,14 +605,19 @@ elsif ($phase eq 'Run this report'){ # split on ??. Each odd (2,4,6,...) entry should be a parameter to fill my @split = split /<<|>>/,$sql; my @tmpl_parameters; + my @authval_errors; for(my $i=0;$i<($#split/2);$i++) { my ($text,$authorised_value) = split /\|/,$split[$i*2+1]; my $input; my $labelid; - if ($authorised_value eq "date") { - $input = 'date'; - } - elsif ($authorised_value) { + if ( not defined $authorised_value ) { + # no authorised value input, provide a text box + $input = "text"; + } elsif ( $authorised_value eq "date" ) { + # require a date, provide a date picker + $input = 'date'; + } else { + # defined $authorised_value, and not 'date' my $dbh=C4::Context->dbh; my @authorised_values; my %authorised_lib; @@ -601,15 +658,30 @@ elsif ($phase eq 'Run this report'){ #---- "true" authorised value } else { - my $authorised_values_sth = $dbh->prepare("SELECT authorised_value,lib FROM authorised_values WHERE category=? ORDER BY lib"); - - $authorised_values_sth->execute( $authorised_value); - - while ( my ( $value, $lib ) = $authorised_values_sth->fetchrow_array ) { - push @authorised_values, $value; - $authorised_lib{$value} = $lib; - # For item location, we show the code and the libelle - $authorised_lib{$value} = $lib; + if ( IsAuthorisedValueCategory($authorised_value) ) { + my $query = ' + SELECT authorised_value,lib + FROM authorised_values + WHERE category=? + ORDER BY lib + '; + my $authorised_values_sth = $dbh->prepare($query); + $authorised_values_sth->execute( $authorised_value); + + while ( my ( $value, $lib ) = $authorised_values_sth->fetchrow_array ) { + push @authorised_values, $value; + $authorised_lib{$value} = $lib; + # For item location, we show the code and the libelle + $authorised_lib{$value} = $lib; + } + } else { + # not exists $authorised_value_categories{$authorised_value}) + push @authval_errors, {'entry' => $text, + 'auth_val' => $authorised_value }; + # tell the template there's an error + $template->param( auth_val_error => 1 ); + # skip scrolling list creation and params push + next; } } $labelid = $text; @@ -625,14 +697,14 @@ elsif ($phase eq 'Run this report'){ -multiple => 0, -tabindex => 1, ); - } else { - $input = "text"; } + push @tmpl_parameters, {'entry' => $text, 'input' => $input, 'labelid' => $labelid }; } $template->param('sql' => $sql, 'name' => $name, 'sql_params' => \@tmpl_parameters, + 'auth_val_errors' => \@authval_errors, 'enter_params' => 1, 'reports' => $report_id, ); -- 2.39.5