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 <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index b6dd63b..01ab4cd 100644 (file)
--- 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 d4fb1ea..0f4843b 100644 (file)
--- 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 ),