From 38dfafa98150490005c1832d02d2f73019f7b60b 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=UTF-8 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 - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba --- C4/Koha.pm | 21 +++ C4/Reports/Guided.pm | 97 ++++++++++- .../en/modules/reports/guided_reports_start.tt | 87 ++++++++-- reports/guided_reports.pl | 170 ++++++++++++++------ 4 files changed, 313 insertions(+), 62 deletions(-) diff --git a/C4/Koha.pm b/C4/Koha.pm index db71fe7..4c027ee 100644 --- a/C4/Koha.pm +++ b/C4/Koha.pm @@ -57,6 +57,7 @@ BEGIN { &getitemtypeimagelocation &GetAuthorisedValues &GetAuthorisedValueCategories + &IsAuthorisedValueCategory &GetKohaAuthorisedValues &GetKohaAuthorisedValuesFromField &GetKohaAuthorisedValueLib @@ -1104,6 +1105,26 @@ 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; + # Build a hash of authorised value categories for quick search + my %authorised_value_categories = map { $_ => 1 } (GetAuthorisedValueCategories()); + + if ( exists $authorised_value_categories{$category}) { + return 1; + } + + return 0; +} + =head2 GetAuthorisedValueByCode $authhorised_value = GetAuthorisedValueByCode( $category, $authvalcode ); diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index 279ac57..c6eeb14 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 ); } @@ -388,7 +392,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(); @@ -422,7 +426,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; @@ -917,6 +921,95 @@ 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__ 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 7f109d9..52d23d7 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 @@ -549,28 +549,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 %] -
  4. [% sql_param.input %]
  5. + [% FOREACH sql_param IN sql_params %] + [% IF sql_param.input == 'date' %] +
  6. + +
  7. + [% ELSIF ( sql_param.input == 'text' ) %] +
  8. + [% ELSE %] +
  9. [% sql_param.input %]
  10. + [% END %] [% END %] - [% END %]
+ [% END %]
[% END %] diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index 23d85a9..38b4f30 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -29,6 +29,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 @@ -132,7 +133,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}; @@ -160,6 +160,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; @@ -184,26 +185,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, + ); + } } } @@ -466,6 +493,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 @@ -489,6 +517,7 @@ elsif ( $phase eq 'Save Report' ) { elsif ($sql !~ /^(SELECT)/i) { push @errors, {queryerr => "No SELECT"}; } + if (@errors) { $template->param( 'errors' => \@errors, @@ -499,25 +528,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, + ); + } } } @@ -549,14 +601,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; @@ -597,15 +654,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; @@ -621,14 +693,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, ); -- 1.7.10.4