Bugzilla – Attachment 15596 Details for
Bug 9659
Undefined authorised value category yields empty dropdown menu on SQL reports
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports
Bug-9659---Better-handling-of-non-existent-authori.patch (text/plain), 22.96 KB, created by
Tomás Cohen Arazi (tcohen)
on 2013-02-21 18:53:04 UTC
(
hide
)
Description:
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2013-02-21 18:53:04 UTC
Size:
22.96 KB
patch
obsolete
>From aa70e38db809c50da4e4fd03d48ebb370762387f Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@gmail.com> >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. > >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. >- Thanks to Owen and Jared for their patience heh. > >Sponsored-by: Universidad Nacional de Córdoba >--- > C4/Koha.pm | 21 +++ > C4/Reports/Guided.pm | 95 ++++++++++- > .../en/modules/reports/guided_reports_start.tt | 87 ++++++++-- > reports/guided_reports.pl | 172 ++++++++++++++------ > 4 files changed, 314 insertions(+), 61 deletions(-) > >diff --git a/C4/Koha.pm b/C4/Koha.pm >index db71fe7..3c20acb 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..98a0e58 100644 >--- a/C4/Reports/Guided.pm >+++ b/C4/Reports/Guided.pm >@@ -32,7 +32,7 @@ use XML::Simple; > use XML::Dumper; > use C4::Debug; > # use Smart::Comments; >-# use Data::Dumper; >+use Data::Dumper; > > BEGIN { > # set the version for version checking >@@ -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 > ); > } > >@@ -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.</p> > </form> > [% END %] > >+[% IF ( warn_authval_problem ) %] >+ <form action='/cgi-bin/koha/reports/guided_reports.pl'> >+ <!--Every parameter the user issued is provided as a hidden field for recovery--> >+ <input type='hidden' name='id' value='[% id %]' /> >+ <input type='hidden' name='sql' value='[% sql %]' /> >+ <input type='hidden' name='reportname' value='[% reportname %]' /> >+ <input type='hidden' name='group' value='[% group %]' /> >+ <input type='hidden' name='subgroup' value='[% subgroup %]' /> >+ <input type='hidden' name='notes' value='[% notes %]' /> >+ <input type='hidden' name='cache_expiry' value='[% cache_expiry %]' /> >+ <input type='hidden' name='cache_expiry_units' value='[% cache_expiry_units %]' /> >+ <input type='hidden' name='public' value='[% public %]' /> >+ >+ <div class="dialog alert"> >+ <h3>Errors found when processing parameters for report: [% name %]</h3> >+ [% FOREACH problematic_authval IN problematic_authvals %] >+ <p> >+ <strong>[% problematic_authval.name %]:</strong> The authorised value category (<strong>[% problematic_authval.authval %]</strong>) >+ you selected does not exist. >+ </p> >+ [% END %] >+ <!-- Save Anyway Form --> >+ <form action='/cgi-bin/koha/reports/guided_reports.pl'> >+ <!--Every parameter the user issued is provided as a hidden field for recovery--> >+ <input type='hidden' name='id' value='[% id %]' /> >+ <input type='hidden' name='sql' value='[% sql %]' /> >+ <input type='hidden' name='reportname' value='[% reportname %]' /> >+ <input type='hidden' name='group' value='[% group %]' /> >+ <input type='hidden' name='subgroup' value='[% subgroup %]' /> >+ <input type='hidden' name='notes' value='[% notes %]' /> >+ <input type='hidden' name='cache_expiry' value='[% cache_expiry %]' /> >+ <input type='hidden' name='cache_expiry_units' value='[% cache_expiry_units %]' /> >+ <input type='hidden' name='public' value='[% public %]' /> >+ [% IF ( phase_update) %] >+ <input type='hidden' name='phase' value='Update SQL' /> >+ <input type="submit" name="save_anyway" class="approve" value="Save anyway" /> >+ [% ELSIF ( phase_save) %] >+ <input type='hidden' name='area' value='[% area %]' /> >+ <input type='hidden' name='phase' value='Save Report' /> >+ <input type="submit" name="save_anyway" class="approve" value="Save anyway" /> >+ [% END %] >+ </form> >+ <!-- Go back to editing --> >+ <form action='/cgi-bin/koha/reports/guided_reports.pl'> >+ <input type="button" name='back' class="deny" value="Edit SQL" >+ onclick="javascript:history.back()" /> >+ </form> >+ </div> >+ </form> >+[% END %] >+ > [% IF ( enter_params ) %] > <form action='/cgi-bin/koha/reports/guided_reports.pl'> >- <input type='hidden' name='phase' value='Run this report' /> > <input type='hidden' name='reports' value="[% reports %]" /> >+ [% IF ( auth_val_error ) %] >+ <input type='hidden' name='phase' value='Edit SQL' /> >+ <div class="dialog alert"> >+ <h3>Errors found when processing parameters for report: [% name %]</h3> >+ [% FOREACH auth_val_error IN auth_val_errors %] >+ <p> >+ <strong>[% auth_val_error.entry %]:</strong> The authorised value category (<strong>[% auth_val_error.auth_val %]</strong>) >+ you selected does not exist. >+ </p> >+ [% END %] >+ </div> >+ <fieldset class="action"><input type="submit" value="Edit SQL" /></fieldset> >+ [% ELSE %] >+ <input type='hidden' name='phase' value='Run this report' /> > <h1>Enter parameters for report [% name %]:</h1> > [% IF ( notes ) %]<p>[% notes %]</p>[% END %] > <fieldset class="rows"> > <ol> >- [% FOREACH sql_param IN sql_params %] >- [% IF sql_param.input == 'date' %] >- <li> >- <label for="date_[% sql_param_entry %][% loop.count %]">[% sql_param.entry %]:</label> <input id="date_[% sql_param_entry %][% loop.count %]" type="text" value="" size="10" name="sql_params" class="datepicker" /> >- </li> >- [% ELSIF ( sql_param.input == 'text' ) %] >- <li><label for="sql_params[% loop.count %]">[% sql_param.entry %]: </label><input id="sql_params[% loop.count %]" type="text" name="sql_params" /></li> >- [% ELSE %] >- <li><label for="sql_params_[% sql_param.labelid %]">[% sql_param.entry %]:</label> [% sql_param.input %]</li> >+ [% FOREACH sql_param IN sql_params %] >+ [% IF sql_param.input == 'date' %] >+ <li> >+ <label for="date_[% sql_param_entry %][% loop.count %]">[% sql_param.entry %]:</label> <input id="date_[% sql_param_entry %][% loop.count %]" type="text" value="" size="10" name="sql_params" class="datepicker" /> >+ </li> >+ [% ELSIF ( sql_param.input == 'text' ) %] >+ <li><label for="sql_params[% loop.count %]">[% sql_param.entry %]: </label><input id="sql_params[% loop.count %]" type="text" name="sql_params" /></li> >+ [% ELSE %] >+ <li><label for="sql_params_[% sql_param.labelid %]">[% sql_param.entry %]:</label> [% sql_param.input %]</li> >+ [% END %] > [% END %] >- [% END %] > </ol> > </fieldset> > <fieldset class="action"><input type="submit" value="Run the report" /></fieldset> >+ [% END %] > </form> > [% END %] > >diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl >index 23d85a9..328c07c 100755 >--- a/reports/guided_reports.pl >+++ b/reports/guided_reports.pl >@@ -29,6 +29,9 @@ 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/; >+use Carp; >+use Data::Dumper; > > =head1 NAME > >@@ -132,7 +135,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 +162,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 +187,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 +495,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 +519,7 @@ elsif ( $phase eq 'Save Report' ) { > elsif ($sql !~ /^(SELECT)/i) { > push @errors, {queryerr => "No SELECT"}; > } >+ > if (@errors) { > $template->param( > 'errors' => \@errors, >@@ -499,25 +530,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 +603,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 +656,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 +695,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
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 9659
:
15560
|
15596
|
15603
|
15618
|
15621
|
15626
|
17351
|
17652
|
17660
|
17661