From c9c0d82f2295e26963075511b74ce8f41b511798 Mon Sep 17 00:00:00 2001 From: Fridolin Somers Date: Thu, 14 Jan 2021 16:46:25 +0100 Subject: [PATCH] Bug 26669: Last Run if report not always updated The last run of a report is updated only if method execute_query() is called with report_id. This whas missing for : - when report is run publicly - when report is sent by email - when report is exported Patch changes the method signature to use a hash of params, in order to easily avoid some params. Test plan : 1) Create a report. 2) Run report. 3) Check the report listing. Confirm that the last run info on the report is updated. 4) Make report public. 5) Run report via public url. 6) Check the report listing. Confirm that the last run info on the report IS NOT updated. 7) Schedule the report to run at a given time and e-mailed to an address. 8) After the report runs at the scheduled time, check the report listing. Confirm that the last run info on the report IS NOT updated. 9) Run report. 10) Export results. 11) Check the report listing. Confirm that the last run info on the report IS NOT updated AT THE TIME OF THE EXPORT. Questionable (I don't know if this is addressed): 12) Run report on backend through a cron job and send results via e-mail. 13) Check the report listing. Confirm that the last run info on the report IS NOT updated. 14) Apply patch. 15) Rerun steps 2-13. Confirm that steps 3, 6, 8, 11, and 13 DO UPDATE the last run info. --- C4/Reports/Guided.pm | 14 +++++++----- misc/cronjobs/runreport.pl | 8 ++++++- opac/svc/report | 12 +++++++--- reports/guided_reports.pl | 13 ++++++++--- svc/report | 11 ++++++++-- t/db_dependent/Reports/Guided.t | 39 +++++++++++++++++++++++---------- 6 files changed, 70 insertions(+), 27 deletions(-) diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index b625443a7a..30366a960a 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -539,17 +539,18 @@ sub strip_limit { sub execute_query { - my ( $sql, $offset, $limit, $sql_params, $report_id ) = @_; - - $sql_params = [] unless defined $sql_params; + my $params = shift; + my $sql = $params->{sql}; + my $offset = $params->{offset} || 0; + my $limit = $params->{limit} || 999999; + my $sql_params = defined $params->{sql_params} ? $params->{sql_params} : []; + my $report_id = $params->{report_id}; # check parameters unless ($sql) { carp "execute_query() called without SQL argument"; return; } - $offset = 0 unless $offset; - $limit = 999999 unless $limit; $debug and print STDERR "execute_query($sql, $offset, $limit)\n"; if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) { return (undef, { sqlerr => $1} ); @@ -1033,7 +1034,8 @@ sub EmailReport { my $sql = $report->savedsql; return ( { FATAL => "NO_REPORT" } ) unless $sql; - my ( $sth, $errors ) = execute_query( $sql ); #don't pass offset or limit, hardcoded limit of 999,999 will be used + #don't pass offset or limit, hardcoded limit of 999,999 will be used + my ( $sth, $errors ) = execute_query( { sql => $sql, report_id => $report_id } ); return ( undef, [{ FATAL => "REPORT_FAIL" }] ) if $errors; my $counter = 1; diff --git a/misc/cronjobs/runreport.pl b/misc/cronjobs/runreport.pl index 3e81e08a97..6a1b0b4024 100755 --- a/misc/cronjobs/runreport.pl +++ b/misc/cronjobs/runreport.pl @@ -269,7 +269,13 @@ foreach my $report_id (@ARGV) { my $params_needed = ( $sql =~ s/(<<[^>]+>>)/\?/g ); die("You supplied ". scalar @params . " parameter(s) and $params_needed are required by the report") if scalar @params != $params_needed; - my ($sth) = execute_query( $sql, undef, undef, \@params, $report_id ); + my ($sth) = execute_query( + { + sql => $sql, + sql_params => \@params, + report_id => $report_id, + } + ); my $count = scalar($sth->rows); unless ($count) { print "NO OUTPUT: 0 results from execute_query\n"; diff --git a/opac/svc/report b/opac/svc/report index ef07334573..6bae8aac2e 100755 --- a/opac/svc/report +++ b/opac/svc/report @@ -56,7 +56,6 @@ if ($cache_active) { } unless ($json_text) { - my $offset = 0; my $limit = C4::Context->preference("SvcMaxReportRows") || 10; my $sql = $report_rec->savedsql; @@ -64,8 +63,15 @@ unless ($json_text) { $sql =~ s/(<<.*?>>)/\?/g; $sql =~ s/\[\[(.*?)\|(.*?)\]\]/$1 AS $2/g; - my ( $sth, $errors ) = - execute_query( $sql, $offset, $limit, \@sql_params, $report_id ); + my ( $sth, $errors ) = execute_query( + { + sql => $sql, + offset => 0, + limit => $limit, + sql_params => \@sql_params, + report_id => $report_id, + } + ); if ($sth) { my $lines; if ($report_annotation) { diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index aab868ed19..6062242515 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -829,7 +829,14 @@ elsif ($phase eq 'Run this report'){ } else { my ($sql,$header_types) = get_prepped_report( $sql, \@param_names, \@sql_params); $template->param(header_types => $header_types); - my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, undef, $report_id ); + my ( $sth, $errors ) = execute_query( + { + sql => $sql, + offset => $offset, + limit => $limit, + report_id => $report_id, + } + ); my $total = nb_rows($sql) || 0; unless ($sth) { die "execute_query failed to return sth for report $report_id: $sql"; @@ -841,7 +848,7 @@ elsif ($phase eq 'Run this report'){ push @rows, { cells => \@cells }; } if( $want_full_chart ){ - my ($sth2, $errors2) = execute_query($sql); + my ( $sth2, $errors2 ) = execute_query( { sql => $sql, report_id => $report_id } ); while (my $row = $sth2->fetchrow_arrayref()) { my @cells = map { +{ cell => $_ } } @$row; push @allrows, { cells => \@cells }; @@ -893,7 +900,7 @@ elsif ($phase eq 'Export'){ my $reportfilename = $reportname ? "$reportname-reportresults.$format" : "reportresults.$format" ; ($sql, undef) = get_prepped_report( $sql, \@param_names, \@sql_params ); - my ($sth, $q_errors) = execute_query($sql); + my ( $sth, $q_errors ) = execute_query( { sql => $sql, report_id => $report_id } ); unless ($q_errors and @$q_errors) { my ( $type, $content ); if ($format eq 'tab') { diff --git a/svc/report b/svc/report index 0ab8fb5dcb..8df4c9c23a 100755 --- a/svc/report +++ b/svc/report @@ -59,7 +59,6 @@ if ($cache_active) { } unless ($json_text) { - my $offset = 0; my $limit = C4::Context->preference("SvcMaxReportRows") || 10; my $sql = $report_rec->savedsql; @@ -67,7 +66,15 @@ unless ($json_text) { $sql =~ s/(<<.*?>>)/\?/g; $sql =~ s/\[\[(.*?)\|(.*?)\]\]/$1 AS $2/g; - my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, \@sql_params, $report_id ); + my ( $sth, $errors ) = execute_query( + { + sql => $sql, + offset => 0, + limit => $limit, + sql_params => \@sql_params, + report_id => $report_id, + } + ); if ($sth) { my $lines; if ($report_annotation) { diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t index 4352d48568..2fe1a982c8 100755 --- a/t/db_dependent/Reports/Guided.t +++ b/t/db_dependent/Reports/Guided.t @@ -234,16 +234,24 @@ subtest 'get_saved_reports' => sub { is( scalar @{ get_saved_reports() }, $count, "Report2 and report3 have been deleted" ); - my $sth = execute_query('SELECT COUNT(*) FROM systempreferences', 0, 10); + my $sth = execute_query( + { + sql => 'SELECT COUNT(*) FROM systempreferences', + offset => 0, + limit => 10, + } + ); my $results = $sth->fetchall_arrayref; is(scalar @$results, 1, 'running a query returned a result'); my $version = C4::Context->preference('Version'); $sth = execute_query( - 'SELECT value FROM systempreferences WHERE variable = ?', - 0, - 10, - [ 'Version' ], + { + sql => 'SELECT value FROM systempreferences WHERE variable = ?', + offset => 0, + limit => 10, + sql_params => ['Version'], + } ); $results = $sth->fetchall_arrayref; is_deeply( @@ -254,11 +262,18 @@ subtest 'get_saved_reports' => sub { # for next test, we want to let execute_query capture any SQL errors my $errors; - warning_like {local $dbh->{RaiseError} = 0; ($sth, $errors) = execute_query( - 'SELECT surname FRM borrowers', # error in the query is intentional - 0, 10 ) } - qr/DBD::mysql::st execute failed: You have an error in your SQL syntax;/, - "Wrong SQL syntax raises warning"; + warning_like { + local $dbh->{RaiseError} = 0; + ( $sth, $errors ) = execute_query( + { + sql => 'SELECT surname FRM borrowers', # error in the query is intentional + offset => 0, + limit => 10, + } + ) + } + qr/DBD::mysql::st execute failed: You have an error in your SQL syntax;/, + "Wrong SQL syntax raises warning"; ok( defined($errors) && exists($errors->{queryerr}), 'attempting to run a report with an SQL syntax error returns error message (Bug 12214)' @@ -283,14 +298,14 @@ subtest 'Ensure last_run is populated' => sub { is( $report->last_run, undef, 'Newly created report has null last_run ' ); - execute_query( $report->savedsql, undef, undef, undef, $report->id ); + execute_query( { sql => $report->savedsql, report_id => $report->id } ); $report->discard_changes(); isnt( $report->last_run, undef, 'First run of report populates last_run' ); my $previous_last_run = $report->last_run; sleep(1); # last_run is stored to the second, so we need to ensure at least one second has passed between runs - execute_query( $report->savedsql, undef, undef, undef, $report->id ); + execute_query( { sql => $report->savedsql, report_id => $report->id } ); $report->discard_changes(); isnt( $report->last_run, $previous_last_run, 'Second run of report updates last_run' ); -- 2.30.0