Attachment 107735 Details for Bug 26132 – Bug 26132: Remove raw sql query

[patch] Bug 26132: Remove raw sql query

Bug-26132-Remove-raw-sql-query.patch (text/plain), 8.77 KB, created by Jonathan Druart on 2020-08-04 10:34:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2020-08-04 10:34:20 UTC

Size: 8.77 KB

patch

obsolete

>From 4fa2652feef4f7be4255a63fd63f4676bec227b1 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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)
>---
> 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 7fc90e72b3..d0d67184db 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -432,81 +432,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,
>@@ -611,7 +590,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.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 26132: 107734 | 107735 | 107736 | 107737 | 107738 | 107796 | 107797 | 107798 | 107799 | 107800 | 107801 | 108147 | 108148 | 108149 | 108150 | 108151 | 108152 | 108153 | 113006 | 113007 | 113014 | 113034