From 1af5b8a7d72c8e49f857907c85809a38ee5b1a07 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 4 Aug 2020 12:05:53 +0200 Subject: [PATCH] Bug 26132: Remove raw sql query Making use of Koha::Checkouts make the code much more readable here. It fixes 2 flaws: * $type was not quote escaped * the effective itemtype was not used which could lead to wrong calculation (for instance item-level_itypes is set but the item does not have the itype defined) However there is something to note, we are going to make things a bit less effective as we are now fetching the items to get their effective itemtype (vs a SUM done at DB level) Signed-off-by: Nick Clemens Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart --- C4/Circulation.pm | 115 +++++++++++---------------- t/db_dependent/Circulation/TooMany.t | 2 +- 2 files changed, 48 insertions(+), 69 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b6dd63bfe3..01ab4cd643 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -431,81 +431,60 @@ sub TooMany { # rule if (defined($maxissueqty_rule) and $maxissueqty_rule->rule_value ne "") { - my @bind_params; - my $count_query = ""; - - if (C4::Context->preference('item-level_itypes')) { - $count_query .= q|SELECT COALESCE( SUM( IF(items.itype = '| .$type . q|',1,0) ), 0) as type_total, COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts|; - } else{ - $count_query .= q|SELECT COALESCE(SUM( IF(biblioitems.itemtype = '| .$type . q|',1,0) ), 0) as type_total, COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts|; - } - - $count_query .= q| - FROM issues - JOIN items USING (itemnumber) - |; - - my $rule_itemtype = $maxissueqty_rule->itemtype; - unless ($rule_itemtype) { - # matching rule has the default item type, so count only - # those existing loans that don't fall under a more - # specific rule - my $issuing_itemtypes_query = q{ - SELECT itemtype FROM circulation_rules - WHERE branchcode = ? - AND (categorycode = ? OR categorycode = ?) - AND itemtype IS NOT NULL - AND rule_name = 'maxissueqty' - }; - if (C4::Context->preference('item-level_itypes')) { - $count_query .= " WHERE items.itype NOT IN ( $issuing_itemtypes_query )"; + my $patron = Koha::Patrons->find($borrower->{borrowernumber}); + my $checkouts = $patron->checkouts->search( {}, { prefetch => 'item' } ); + if ( $maxissueqty_rule->branchcode ) { + if ( C4::Context->preference('CircControl') eq 'PickupLibrary' ) { + $checkouts = $checkouts->search({ 'me.branchcode' => $maxissueqty_rule->branchcode }); + } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') { + ; # if branch is the patron's home branch, then count all loans by patron } else { - $count_query .= " JOIN biblioitems USING (biblionumber) - WHERE biblioitems.itemtype NOT IN ( $issuing_itemtypes_query )"; + $checkouts = $checkouts->search({ 'item.homebranch' => $maxissueqty_rule->branchcode }); } - push @bind_params, $maxissueqty_rule->branchcode; - push @bind_params, $maxissueqty_rule->categorycode; - push @bind_params, $cat_borrower; - } else { - my @types; - if ( $parent_maxissueqty_rule ) { - # if we have a parent item type then we count loans of the - # specific item type or its siblings or parent - my $children = Koha::ItemTypes->search({ parent_type => $parent_type }); - @types = $children->get_column('itemtype'); - push @types, $parent_type; - } elsif ( $child_types ) { - # If we are a parent type, we need to count all child types and our own type - @types = $child_types->get_column('itemtype'); - push @types, $type; # And don't forget to count our own types - } else { push @types, $type; } # Otherwise only count the specific itemtype - my $types_param = ( '?,' ) x @types; - $types_param =~ s/,$//; - if (C4::Context->preference('item-level_itypes')) { - $count_query .= " WHERE items.itype IN (" . $types_param . ")"; - } else { - $count_query .= " JOIN biblioitems USING (biblionumber) - WHERE biblioitems.itemtype IN (" . $types_param . ")"; - } - push @bind_params, @types; } + my $sum_checkouts; + my $rule_itemtype = $maxissueqty_rule->itemtype; + while ( my $c = $checkouts->next ) { + my $itemtype = $c->item->effective_itemtype; + my @types; + unless ( $rule_itemtype ) { + # matching rule has the default item type, so count only + # those existing loans that don't fall under a more + # specific rule + @types = Koha::CirculationRules->search( + { + branchcode => $maxissueqty_rule->branchcode, + categorycode => [ $maxissueqty_rule->categorycode, $cat_borrower ], + itemtype => { '!=' => undef }, + rule_name => 'maxissueqty' + } + )->get_column('itemtype'); - $count_query .= " AND borrowernumber = ? "; - push @bind_params, $borrower->{'borrowernumber'}; - my $rule_branch = $maxissueqty_rule->branchcode; - if ($rule_branch) { - if (C4::Context->preference('CircControl') eq 'PickupLibrary') { - $count_query .= " AND issues.branchcode = ? "; - push @bind_params, $rule_branch; - } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') { - ; # if branch is the patron's home branch, then count all loans by patron + next if grep {$_ eq $itemtype} @types; } else { - $count_query .= " AND items.homebranch = ? "; - push @bind_params, $rule_branch; + my @types; + if ( $parent_maxissueqty_rule ) { + # if we have a parent item type then we count loans of the + # specific item type or its siblings or parent + my $children = Koha::ItemTypes->search({ parent_type => $parent_type }); + @types = $children->get_column('itemtype'); + push @types, $parent_type; + } elsif ( $child_types ) { + # If we are a parent type, we need to count all child types and our own type + @types = $child_types->get_column('itemtype'); + push @types, $type; # And don't forget to count our own types + } else { push @types, $type; } # Otherwise only count the specific itemtype + + next unless grep {$_ eq $itemtype} @types; } + $sum_checkouts->{total}++; + $sum_checkouts->{onsite_checkouts}++ if $c->onsite_checkout; + $sum_checkouts->{itemtype}->{$itemtype}++; } - my ( $checkout_count_type, $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $count_query, {}, @bind_params ); + my $checkout_count_type = $sum_checkouts->{itemtype}->{$type} || 0; + my $checkout_count = $sum_checkouts->{total} || 0; + my $onsite_checkout_count = $sum_checkouts->{onsite_checkouts} || 0; my $checkout_rules = { checkout_count => $checkout_count, @@ -610,7 +589,7 @@ sub _check_max_qty { } } elsif ( not $onsite_checkout ) { if( $max_checkouts_allowed eq '' ){ return;} - if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { + if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { return { reason => 'TOO_MANY_CHECKOUTS', count => $checkout_count - $onsite_checkout_count, diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t index d4fb1ea5e5..0f4843b822 100644 --- a/t/db_dependent/Circulation/TooMany.t +++ b/t/db_dependent/Circulation/TooMany.t @@ -919,7 +919,7 @@ subtest 'itemtype group tests' => sub { undef, undef, undef ); like( $issue->issue_id, qr|^\d+$|, 'the issue should have been inserted' ); - #patron has 1 checkoout of childitype1 and 2 of childitype2 + #patron has 1 checkout of childitype1 and 2 of childitype2 is( C4::Circulation::TooMany( $patron, $item_2 ), -- 2.39.5