Bug 27380: Move get_prepped_report to object and use for svc/reports
authorNick Clemens <nick@bywatersolutions.com>
Mon, 11 Jan 2021 20:07:39 +0000 (20:07 +0000)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 12 Feb 2021 11:29:56 +0000 (12:29 +0100)
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>
JD Amended patch: Perltidy prep_report

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
C4/Reports/Guided.pm
Koha/Report.pm
opac/svc/report
reports/guided_reports.pl
svc/report
t/db_dependent/Koha/Reports.t

index 7bc1504..a76d86b 100644 (file)
@@ -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
index 01b7dff..391434e 100644 (file)
@@ -90,6 +90,73 @@ 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
index ef07334..08e1ddc 100755 (executable)
@@ -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) {
index e847d05..cff8da8 100755 (executable)
@@ -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;
-}
index 0ab8fb5..18b0f2b 100755 (executable)
@@ -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) {
index 45c5662..ab982c8 100755 (executable)
@@ -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;