Attachment 116465 Details for Bug 27380 – Bug 27380: Move get_prepped_report to object and use for svc/reports

[patch] Bug 27380: Move get_prepped_report to object and use for svc/reports

Bug-27380-Move-getpreppedreport-to-object-and-use-.patch (text/plain), 12.37 KB, created by Katrin Fischer on 2021-02-07 11:47:28 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Katrin Fischer

Created: 2021-02-07 11:47:28 UTC

Size: 12.37 KB

patch

obsolete

>From b41162872c24d3f5290a25915289ab96a348f85b Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Mon, 11 Jan 2021 20:07:39 +0000
>Subject: [PATCH] Bug 27380: Move get_prepped_report to object and use for
> svc/reports
>
>This patch moves get_prepped_report to Koha:Report->prep_report
>and adds some basic tests
>
>To test:
>1 - Using the report created in last test, hit the report svc api like:
>http://localhost:8081/cgi-bin/koha/svc/report?id=6&param_name=One&sql_params=One&param_name=Listy|list&sql_params=2345%0D%0A423%0D%0A3%0D%0A2%0D%0A12
>2 - Note the use of %0D%0A to separate list params
>3 - Test with combinations with and without param_name specified
>http://localhost:8081/cgi-bin/koha/svc/report?id=6&sql_params=5&sql_params=2345%0D%0A423%0D%0A3%0D%0A2%0D%0A12
>
>Signed-off-by: Owen Leonard <oleonard@myacpl.org>
>
>Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
>---
> C4/Reports/Guided.pm          | 12 ++++++++++
> Koha/Report.pm                | 53 +++++++++++++++++++++++++++++++++++++++++++
> opac/svc/report               | 10 ++++----
> reports/guided_reports.pl     | 49 ++-------------------------------------
> svc/report                    | 11 ++++-----
> t/db_dependent/Koha/Reports.t | 23 ++++++++++++++++++-
> 6 files changed, 99 insertions(+), 59 deletions(-)
>
>diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
>index 7bc1504c53..a76d86ba44 100644
>--- a/C4/Reports/Guided.pm
>+++ b/C4/Reports/Guided.pm
>@@ -557,6 +557,18 @@ sub execute_query {
>         return (undef, { queryerr => 'Missing SELECT'} );
>     }
> 
>+    foreach my $sql_param ( @$sql_params ){
>+        if ( $sql_param =~ m/\n/ ){
>+            my @list = split /\n/, $sql_param;
>+            my @quoted_list;
>+            foreach my $item ( @list ){
>+                $item =~ s/\r//;
>+              push @quoted_list, C4::Context->dbh->quote($item);
>+            }
>+            $sql_param = "(".join(",",@quoted_list).")";
>+        }
>+    }
>+
>     my ($useroffset, $userlimit);
> 
>     # Grab offset/limit from user supplied LIMIT and drop the LIMIT so we can control pagination
>diff --git a/Koha/Report.pm b/Koha/Report.pm
>index 01b7dff786..b0647bddb4 100644
>--- a/Koha/Report.pm
>+++ b/Koha/Report.pm
>@@ -90,6 +90,59 @@ sub new_from_mana {
>     Koha::Report->new($data)->store;
> }
> 
>+=head3 prep_report
>+
>+Prep the report and return executable sql with parameters embedded and a list of header types
>+for building batch action links in the template
>+
>+=cut
>+
>+sub prep_report {
>+    my ($self, $param_names, $sql_params ) = @_;
>+    my $sql = $self->savedsql;
>+
>+    # First we split out the placeholders
>+    # This part of the code supports using [[ table.field | alias ]] in the
>+    # query and replaces it by table.field AS alias. This is used to build
>+    # the batch action links foir cardnumbers, itemnumbers, and biblionumbers in the template
>+    # while allowing the library to alter the column names
>+    my @split = split /\[\[|\]\]/,$sql;
>+    my $headers;
>+    for(my $i=0;$i<$#split/2;$i++){ #The placeholders are always the odd elements of the array
>+        my ($type,$name) = split /\|/,$split[$i*2+1]; # We split them on '|'
>+        $headers->{$name} = $type; # Store as a lookup for the template
>+        $headers->{$name} =~ s/^\w*\.//; # strip the table name just as in $sth->{NAME} array
>+        $split[$i*2+1] =~ s/(\||\?|\.|\*|$|$|\%)/\\$1/g; #Quote any special characters so we can replace the placeholders
>+        $name = C4::Context->dbh->quote($name);
>+        $sql =~ s/\[\[$split[$i*2+1]\]\]/$type AS $name/; # Remove placeholders from SQL
>+    }
>+
>+    my %lookup;
>+    @lookup{@$param_names} = @$sql_params;
>+    @split = split /<<|>>/,$sql;
>+    for(my $i=0;$i<$#split/2;$i++) {
>+        my $quoted = @$param_names ? $lookup{ $split[$i*2+1] } : @$sql_params[$i];
>+        # if there are special regexp chars, we must \ them
>+        $split[$i*2+1] =~ s/(\||\?|\.|\*|$|$|\%)/\\$1/g;
>+        if ($split[$i*2+1] =~ /\|\s*date\s*$/) {
>+            $quoted = output_pref({ dt => dt_from_string($quoted), dateformat => 'iso', dateonly => 1 }) if $quoted;
>+        }
>+        unless( $split[$i*2+1] =~ /\|\s*list\s*$/ && $quoted ){
>+            $quoted = C4::Context->dbh->quote($quoted);
>+        } else {
>+            my @list = split /\n/, $quoted;
>+            my @quoted_list;
>+            foreach my $item ( @list ){
>+                $item =~ s/\r//;
>+                push @quoted_list, C4::Context->dbh->quote($item);
>+            }
>+            $quoted="(".join(",",@quoted_list).")";
>+        }
>+        $sql =~ s/<<$split[$i*2+1]>>/$quoted/;
>+    }
>+    return $sql, $headers;
>+}
>+
> =head3 _type
> 
> Returns name of corresponding DBIC resultset
>diff --git a/opac/svc/report b/opac/svc/report
>index ef07334573..08e1ddc1f7 100755
>--- a/opac/svc/report
>+++ b/opac/svc/report
>@@ -43,6 +43,7 @@ my $report_rec = $report_recs->next();
> die "Sorry this report is not public\n" unless $report_rec->public;
> 
> my @sql_params  = $query->multi_param('sql_params');
>+my @param_names  = $query->multi_param('param_names');
> 
> my $cache = Koha::Caches->get_instance();
> my $cache_active = $cache->is_cache_active;
>@@ -51,7 +52,8 @@ if ($cache_active) {
>     $cache_key =
>         "opac:report:"
>       . ( $report_name ? "name:$report_name:" : "id:$report_id:" )
>-      . join( '-', @sql_params );
>+      . join( '-', @sql_params )
>+      . join( '_'. @param_names );
>     $json_text = $cache->get_from_cache($cache_key);
> }
> 
>@@ -60,12 +62,10 @@ unless ($json_text) {
>     my $limit = C4::Context->preference("SvcMaxReportRows") || 10;
>     my $sql = $report_rec->savedsql;
> 
>-    # convert SQL parameters to placeholders
>-    $sql =~ s/(<<.*?>>)/\?/g;
>-    $sql =~ s/\[\[(.*?)\|(.*?)\]\]/$1 AS $2/g;
>+    my ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params );
> 
>     my ( $sth, $errors ) =
>-      execute_query( $sql, $offset, $limit, \@sql_params, $report_id );
>+      execute_query( $sql, $offset, $limit, undef, $report_id );
>     if ($sth) {
>         my $lines;
>         if ($report_annotation) {
>diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl
>index e847d05e1c..cff8da8ffe 100755
>--- a/reports/guided_reports.pl
>+++ b/reports/guided_reports.pl
>@@ -829,7 +829,7 @@ elsif ($phase eq 'Run this report'){
>                             'reports'      => $report_id,
>                             );
>         } else {
>-            my ($sql,$header_types) = get_prepped_report( $sql, \@param_names, \@sql_params);
>+            my ($sql,$header_types) = $report->prep_report( \@param_names, \@sql_params );
>             $template->param(header_types => $header_types);
>             my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, undef, $report_id );
>             my $total = nb_rows($sql) || 0;
>@@ -894,7 +894,7 @@ elsif ($phase eq 'Export'){
>     my $reportname     = $input->param('reportname');
>     my $reportfilename = $reportname ? "$reportname-reportresults.$format" : "reportresults.$format" ;
> 
>-    ($sql, undef) = get_prepped_report( $sql, \@param_names, \@sql_params );
>+    ($sql, undef) = $report->prep_report( \@param_names, \@sql_params );
> 	my ($sth, $q_errors) = execute_query($sql);
>     unless ($q_errors and @$q_errors) {
>         my ( $type, $content );
>@@ -1090,48 +1090,3 @@ sub create_non_existing_group_and_subgroup {
>     }
> }
> 
>-# pass $sth and sql_params, get back an executable query
>-sub get_prepped_report {
>-    my ($sql, $param_names, $sql_params ) = @_;
>-
>-    # First we split out the placeholders
>-    # This part of the code supports using [[ table.field | alias ]] in the
>-    # query and replaces it by table.field AS alias. Not sure why we would
>-    # need it if we can type the latter (which is simpler)?
>-    my @split = split /\[\[|\]\]/,$sql;
>-    my $headers;
>-    for(my $i=0;$i<$#split/2;$i++){ #The placeholders are always the odd elements of the array
>-        my ($type,$name) = split /\|/,$split[$i*2+1]; # We split them on '|'
>-        $headers->{$name} = $type; # Store as a lookup for the template
>-        $headers->{$name} =~ s/^\w*\.//; # strip the table name just as in $sth->{NAME} array
>-        $split[$i*2+1] =~ s/(\||\?|\.|\*|$|$|\%)/\\$1/g; #Quote any special characters so we can replace the placeholders
>-        $name = C4::Context->dbh->quote($name);
>-        $sql =~ s/\[\[$split[$i*2+1]\]\]/$type AS $name/; # Remove placeholders from SQL
>-    }
>-
>-    my %lookup;
>-    @lookup{@$param_names} = @$sql_params;
>-    @split = split /<<|>>/,$sql;
>-    my @tmpl_parameters;
>-    for(my $i=0;$i<$#split/2;$i++) {
>-        my $quoted = @$param_names ? $lookup{ $split[$i*2+1] } : @$sql_params[$i];
>-        # if there are special regexp chars, we must \ them
>-        $split[$i*2+1] =~ s/(\||\?|\.|\*|$|$|\%)/\\$1/g;
>-        if ($split[$i*2+1] =~ /\|\s*date\s*$/) {
>-            $quoted = output_pref({ dt => dt_from_string($quoted), dateformat => 'iso', dateonly => 1 }) if $quoted;
>-        }
>-        unless( $split[$i*2+1] =~ /\|\s*list\s*$/ && $quoted ){
>-            $quoted = C4::Context->dbh->quote($quoted);
>-        } else {
>-            my @list = split /\n/, $quoted;
>-            my @quoted_list;
>-            foreach my $item ( @list ){
>-                $item =~ s/\r//;
>-              push @quoted_list, C4::Context->dbh->quote($item);
>-            }
>-            $quoted="(".join(",",@quoted_list).")";
>-        }
>-        $sql =~ s/<<$split[$i*2+1]>>/$quoted/;
>-    }
>-    return $sql,$headers;
>-}
>diff --git a/svc/report b/svc/report
>index 0ab8fb5dcb..18b0f2bd4b 100755
>--- a/svc/report
>+++ b/svc/report
>@@ -39,6 +39,7 @@ if (!$report_recs || $report_recs->count == 0 ) { die "There is no such report.\
> my $report_rec = $report_recs->next();
> 
> my @sql_params  = $query->multi_param('sql_params');
>+my @param_names  = $query->multi_param('param_names');
> 
> my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
>     {
>@@ -54,20 +55,18 @@ my $cache_active = $cache->is_cache_active;
> my ($cache_key, $json_text);
> if ($cache_active) {
>     $cache_key = "intranet:report:".($report_name ? "report_name:$report_name:" : "id:$report_id:")
>-    . join( '-', @sql_params );
>+    . join( '-', @sql_params )
>+      . join( '_'. @param_names );
>     $json_text = $cache->get_from_cache($cache_key);
> }
> 
> unless ($json_text) {
>     my $offset = 0;
>     my $limit  = C4::Context->preference("SvcMaxReportRows") || 10;
>-    my $sql = $report_rec->savedsql;
> 
>-    # convert SQL parameters to placeholders
>-    $sql =~ s/(<<.*?>>)/\?/g;
>-    $sql =~ s/\[\[(.*?)\|(.*?)\]\]/$1 AS $2/g;
>+    my ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params );
> 
>-    my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, \@sql_params, $report_id );
>+    my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, undef, $report_id );
>     if ($sth) {
>         my $lines;
>         if ($report_annotation) {
>diff --git a/t/db_dependent/Koha/Reports.t b/t/db_dependent/Koha/Reports.t
>index 45c5662247..ab982c84d7 100755
>--- a/t/db_dependent/Koha/Reports.t
>+++ b/t/db_dependent/Koha/Reports.t
>@@ -17,7 +17,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 4;
>+use Test::More tests => 5;
> 
> use Koha::Report;
> use Koha::Reports;
>@@ -48,4 +48,25 @@ is( $retrieved_report_1->report_name, $new_report_1->report_name, 'Find a report
> $retrieved_report_1->delete;
> is( Koha::Reports->search->count, $nb_of_reports + 1, 'Delete should have deleted the report' );
> 
>+subtest 'prep_report' => sub {
>+    plan tests => 3;
>+
>+    my $report = Koha::Report->new({
>+        report_name => 'report_name_for_test_1',
>+        savedsql => 'SELECT * FROM items WHERE itemnumber IN <<Test|list>>',
>+    })->store;
>+
>+    my ($sql, undef) = $report->prep_report( ['Test|list'],["1\n12\n\r243"] );
>+    is( $sql, q{SELECT * FROM items WHERE itemnumber IN ('1','12','243')},'Expected sql generated correctly with single param and name');
>+
>+    $report->savedsql('SELECT * FROM items WHERE itemnumber IN <<Test|list>> AND <<Another>> AND <<Test|list>>')->store;
>+
>+    ($sql, undef) = $report->prep_report( ['Test|list','Another'],["1\n12\n\r243",'the other'] );
>+    is( $sql, q{SELECT * FROM items WHERE itemnumber IN ('1','12','243') AND 'the other' AND ('1','12','243')},'Expected sql generated correctly with multiple params and names');
>+
>+    ($sql, undef) = $report->prep_report( [],["1\n12\n\r243",'the other',"42\n32\n22\n12"] );
>+    is( $sql, q{SELECT * FROM items WHERE itemnumber IN ('1','12','243') AND 'the other' AND ('42','32','22','12')},'Expected sql generated correctly with multiple params and no names');
>+
>+};
>+
> $schema->storage->txn_rollback;
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 27380: 115044 | 115047 | 115048 | 115084 | 115085 | 116160 | 116161 | 116162 | 116163 | 116464 | 116465 | 116466 | 116818 | 116830 | 116908