From 0d80c7d440565421b04ae2dfab390507ec8dfbca Mon Sep 17 00:00:00 2001 From: Slava Shishkin Date: Mon, 24 Aug 2026 18:03:22 +0300 Subject: [PATCH] Bug 43296: Support valid patron attribute codes in circulation stats Circulation statistics only recognise numeric patron attribute codes, while patron attribute type codes are not restricted to numbers. Validate Row, Column and Filter_borrower_attributes against the available patron attribute types. Use generated SQL aliases and pass attribute codes as bind parameters. Test plan: 1. In Administration > Patron attribute types, create attribute types with codes SHOW-BCODE, SHOW.BCODE, SHOW BCODE and SQL); DROP TABLE borrowers; --.. 2. Go to Reports > Statistics wizards > Circulation. 3. Use each attribute as Row and Column with an ordinary field on the other axis. 4. Confirm the report fails or uses the wrong field. Apply the patch. 5. Restart Plack. 6. Repeat steps 3-4. 7. Confirm the reports succeed and use the selected patron attribute. 8. Test the same attributes as filters. 9. Confirm ordinary Row, Column and Filter fields still work. Signed-off-by: David Nind Signed-off-by: Jonathan Druart Signed-off-by: Pedro Amorim --- reports/issues_stats.pl | 126 +++++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 61 deletions(-) diff --git a/reports/issues_stats.pl b/reports/issues_stats.pl index 98248ba20f..71abe8bf97 100755 --- a/reports/issues_stats.pl +++ b/reports/issues_stats.pl @@ -68,14 +68,22 @@ my ( $template, $borrowernumber, $cookie ) = get_template_and_user( } ); -my $attribute_filters; -my $vars = $input->Vars; -for my $k ( keys %$vars ) { - if ( $k =~ /^Filter_borrower_attributes\.(\d+)$/ ) { - my $val = $vars->{$k}; - $val =~ s/\*/%/g if $val; - $attribute_filters->{$1} = $val; - } +my $library_id = C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef; +my @attribute_types = + Koha::Patron::Attribute::Types->search_with_library_limits( {}, {}, $library_id )->as_list; +my %allowed_attribute_codes = map { $_->code => 1 } @attribute_types; + +my $attribute_filters = {}; +my $vars = $input->Vars; +for my $key ( keys %$vars ) { + next unless index( $key, 'Filter_borrower_attributes.' ) == 0; + + my $code = substr( $key, length('Filter_borrower_attributes.') ); + next unless exists $allowed_attribute_codes{$code}; + + my $value = $vars->{$key}; + $value =~ s/\*/%/g if $value; + $attribute_filters->{$code} = $value; } # Allowed values for Line and Column @@ -94,21 +102,8 @@ my %allowed_fields = ( "borrowers.branchcode" => 1, ); -# Validate Line parameter -my $line = "branch"; -if ( $line_input && $allowed_fields{$line_input} ) { - $line = $line_input; -} elsif ( $line_input && $line_input =~ /^borrower_attributes\.(\d+)$/ ) { - $line = $line_input; -} - -# Validate Column parameter -my $column = "datetime"; -if ( $column_input && $allowed_fields{$column_input} ) { - $column = $column_input; -} elsif ( $column_input && $column_input =~ /^borrower_attributes\.(\d+)$/ ) { - $column = $column_input; -} +my $line = report_field( $line_input, 'branch', \%allowed_fields, \%allowed_attribute_codes ); +my $column = report_field( $column_input, 'datetime', \%allowed_fields, \%allowed_attribute_codes ); # Validate DisplayBy parameter my $podsp = ""; @@ -253,10 +248,8 @@ foreach ( sort { $ccodes->{$a} cmp $ccodes->{$b} } keys %$ccodes ) { my $CGIextChoice = ('CSV'); # FIXME translation my $CGIsepChoice = GetDelimiterChoices; -my $library_id = C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef; -my $attribute_types = Koha::Patron::Attribute::Types->search_with_library_limits( {}, {}, $library_id ); my %attribute_types_by_class; -while ( my ($attribute_type) = $attribute_types->next ) { +for my $attribute_type (@attribute_types) { $attribute_type = $attribute_type->unblessed; if ( $attribute_type->{authorised_value_category} ) { my $authorised_values = C4::Koha::GetAuthorisedValues( $attribute_type->{authorised_value_category} ); @@ -354,7 +347,7 @@ sub calculate { : ( $line =~ /homebranch/ ) ? @$filters[11] : ( $line =~ /holdingbranch/ ) ? @$filters[12] : ( $line =~ /borrowers.branchcode/ ) ? @$filters[13] - : ($line_attribute_type) ? $attribute_filters->{$line_attribute_type} + : ( defined $line_attribute_type ) ? $attribute_filters->{$line_attribute_type} : undef; if ( $line =~ /ccode/ or $line =~ /location/ or $line =~ /homebranch/ or $line =~ /holdingbranch/ ) { @@ -375,7 +368,7 @@ sub calculate { : ( $column =~ /homebranch/ ) ? @$filters[11] : ( $column =~ /holdingbranch/ ) ? @$filters[12] : ( $column =~ /borrowers.branchcode/ ) ? @$filters[13] - : ($column_attribute_type) ? $attribute_filters->{$column_attribute_type} + : ( defined $column_attribute_type ) ? $attribute_filters->{$column_attribute_type} : undef; if ( $column =~ /ccode/ or $column =~ /location/ or $column =~ /homebranch/ or $column =~ /holdingbranch/ ) { @@ -402,7 +395,7 @@ sub calculate { : $linefield; my $strsth; - if ($line_attribute_type) { + if ( defined $line_attribute_type ) { $strsth = "SELECT attribute FROM borrower_attributes WHERE code = ? "; } else { $strsth = "SELECT distinctrow $linefield FROM statistics "; @@ -435,7 +428,7 @@ sub calculate { push @loopfilter, { crit => 'SQL =', sql => 1, filter => $strsth }; my $sth = $dbh->prepare($strsth); my @bind = (); - if ($line_attribute_type) { + if ( defined $line_attribute_type ) { push @bind, $line_attribute_type; } if ( (@linefilter) and ( $linefilter[0] ) and ( $linefilter[1] ) ) { @@ -500,7 +493,7 @@ sub calculate { : ( $colfield =~ /^month/ ) ? " month($column)" : $colfield; my $strsth2; - if ($column_attribute_type) { + if ( defined $column_attribute_type ) { $strsth2 = "SELECT attribute FROM borrower_attributes WHERE code = ? "; } else { $strsth2 = "SELECT distinctrow $colfield FROM statistics "; @@ -534,7 +527,7 @@ sub calculate { push @loopfilter, { crit => 'SQL =', sql => 1, filter => $strsth2 }; my $sth2 = $dbh->prepare($strsth2); my @bind2 = (); - if ($column_attribute_type) { + if ( defined $column_attribute_type ) { push @bind2, $column_attribute_type; } if ( (@colfilter) and ( $colfilter[0] ) and ( $colfilter[1] ) ) { @@ -587,15 +580,26 @@ sub calculate { table_set( \%table, $row->{rowtitle}, 'totalrow', 0 ); } + my %required_attribute_codes = map { $_ => 1 } grep { $attribute_filters->{$_} } keys %$attribute_filters; + $required_attribute_codes{$line_attribute_type} = 1 if defined $line_attribute_type; + $required_attribute_codes{$column_attribute_type} = 1 if defined $column_attribute_type; + + my @required_attribute_codes = sort keys %required_attribute_codes; + my %attribute_aliases; + my $attribute_index = 0; + for my $attribute_code (@required_attribute_codes) { + $attribute_aliases{$attribute_code} = 'attribute_' . $attribute_index++; + } + # preparing calculation my $strcalc = "SELECT "; - if ($line_attribute_type) { - $strcalc .= "TRIM(attribute_$line_attribute_type.attribute) AS line_attribute, "; + if ( defined $line_attribute_type ) { + $strcalc .= "TRIM($attribute_aliases{$line_attribute_type}.attribute) AS line_attribute, "; } else { $strcalc .= "TRIM($linefield), "; } - if ($column_attribute_type) { - $strcalc .= "TRIM(attribute_$column_attribute_type.attribute) AS column_attribute, "; + if ( defined $column_attribute_type ) { + $strcalc .= "TRIM($attribute_aliases{$column_attribute_type}.attribute) AS column_attribute, "; } else { $strcalc .= "TRIM($colfield), "; } @@ -615,14 +619,10 @@ sub calculate { FROM statistics LEFT JOIN borrowers ON statistics.borrowernumber=borrowers.borrowernumber "; - foreach my $type ( keys %$attribute_filters ) { - if ( ( $line_attribute_type and $line_attribute_type eq $type ) - or $column_attribute_type and $column_attribute_type eq $type - or $attribute_filters->{$type} ) - { - $strcalc .= - " LEFT JOIN borrower_attributes AS attribute_$type ON (statistics.borrowernumber = attribute_$type.borrowernumber AND attribute_$type.code = ?) "; - } + for my $type (@required_attribute_codes) { + my $alias = $attribute_aliases{$type}; + $strcalc .= + " LEFT JOIN borrower_attributes AS $alias ON (statistics.borrowernumber = $alias.borrowernumber AND $alias.code = ?) "; } $strcalc .= "LEFT JOIN (SELECT * FROM items UNION SELECT * FROM deleteditems) items ON statistics.itemnumber=items.itemnumber " @@ -661,21 +661,16 @@ sub calculate { $strcalc .= " AND monthname(datetime) = ?" if ($monthsel); $strcalc .= " AND statistics.type = ?" if ($type); - foreach ( keys %$attribute_filters ) { - if ( $attribute_filters->{$_} ) { - $strcalc .= " AND attribute_$_.attribute LIKE ?"; + for my $type (@required_attribute_codes) { + if ( $attribute_filters->{$type} ) { + $strcalc .= " AND $attribute_aliases{$type}.attribute LIKE ?"; } } # Build bind array for the main calculation query # First, add the attribute filter codes for the JOINs (in the same order as the JOINs above) - for my $type ( keys %$attribute_filters ) { - if ( ( $line_attribute_type and $line_attribute_type eq $type ) - or $column_attribute_type and $column_attribute_type eq $type - or $attribute_filters->{$type} ) - { - push @bind3, $type; - } + for my $type (@required_attribute_codes) { + push @bind3, $type; } # Then add the filter values @@ -698,31 +693,31 @@ sub calculate { push @bind3, $type if ($type); # Add attribute filter values - for my $type ( keys %$attribute_filters ) { + for my $type (@required_attribute_codes) { if ( $attribute_filters->{$type} ) { push @bind3, $attribute_filters->{$type}; } } $strcalc .= " GROUP BY "; - if ($line_attribute_type) { + if ( defined $line_attribute_type ) { $strcalc .= " line_attribute, "; } else { $strcalc .= " $linefield, "; } - if ($column_attribute_type) { + if ( defined $column_attribute_type ) { $strcalc .= " column_attribute "; } else { $strcalc .= " $colfield "; } $strcalc .= " ORDER BY "; - if ($line_attribute_type) { + if ( defined $line_attribute_type ) { $strcalc .= " line_attribute, "; } else { $strcalc .= " $lineorder, "; } - if ($column_attribute_type) { + if ( defined $column_attribute_type ) { $strcalc .= " column_attribute "; } else { $strcalc .= " $colorder "; @@ -781,11 +776,20 @@ sub calculate { # # the foot (totals by borrower type) $globalline{loopfooter} = \@loopfooter; $globalline{total} = $grantotal; - $globalline{line} = $line_attribute_type ? $line_attribute_type : $line; - $globalline{column} = $column_attribute_type ? $column_attribute_type : $column; + $globalline{line} = defined $line_attribute_type ? $line_attribute_type : $line; + $globalline{column} = defined $column_attribute_type ? $column_attribute_type : $column; return [ ( \%globalline ) ]; } +sub report_field { + my ( $value, $default, $allowed_fields, $allowed_attribute_codes ) = @_; + return $value if defined $value && exists $allowed_fields->{$value}; + return $default unless defined $value && index( $value, 'borrower_attributes.' ) == 0; + + my $code = substr( $value, length('borrower_attributes.') ); + return exists $allowed_attribute_codes->{$code} ? $value : $default; +} + sub null_to_zzempty { my $string = shift;