Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
authorNick Clemens <nick@bywatersolutions.com>
Wed, 1 May 2019 09:57:50 +0000 (09:57 +0000)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Thu, 13 Aug 2020 08:13:14 +0000 (10:13 +0200)
To test:
1 - prove -v t/db_dependent/Circulation/TooMany.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>

JD amended patch: tidy the subtest

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

C4/Circulation.pm
t/db_dependent/Circulation/TooMany.t

index c94b773..58b2416 100644 (file)
@@ -386,8 +386,26 @@ sub TooMany {
     $branch = _GetCircControlBranch($item_object->unblessed,$borrower);
     my $type = $item_object->effective_itemtype;
 
+    my ($type_object, $parent_type, $parent_maxissueqty_rule);
+    $type_object = Koha::ItemTypes->find( $type );
+    $parent_type = $type_object->parent_type if $type_object;
+    my $child_types = Koha::ItemTypes->search({ parent_type => $type });
+    # Find any children if we are a parent_type;
+
     # given branch, patron category, and item type, determine
     # applicable issuing rule
+
+    $parent_maxissueqty_rule = Koha::CirculationRules->get_effective_rule(
+        {
+            categorycode => $cat_borrower,
+            itemtype     => $parent_type,
+            branchcode   => $branch,
+            rule_name    => 'maxissueqty',
+        }
+    ) if $parent_type;
+    # If the parent rule is for default type we discount it
+    $parent_maxissueqty_rule = undef if $parent_maxissueqty_rule && !defined $parent_maxissueqty_rule->itemtype;
+
     my $maxissueqty_rule = Koha::CirculationRules->get_effective_rule(
         {
             categorycode => $cat_borrower,
@@ -396,6 +414,8 @@ sub TooMany {
             rule_name    => 'maxissueqty',
         }
     );
+
+
     my $maxonsiteissueqty_rule = Koha::CirculationRules->get_effective_rule(
         {
             categorycode => $cat_borrower,
@@ -409,10 +429,18 @@ sub TooMany {
     # if a rule is found and has a loan limit set, count
     # how many loans the patron already has that meet that
     # rule
-    if (defined($maxissueqty_rule) and $maxissueqty_rule->rule_value ne '') {
+    if (defined($maxissueqty_rule) and defined($maxissueqty_rule->rule_value)) {
+
         my @bind_params;
-        my $count_query = q|
-            SELECT COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts
+        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)
         |;
@@ -422,37 +450,43 @@ sub TooMany {
             # 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 (
-                                    SELECT itemtype FROM circulation_rules
-                                    WHERE branchcode = ?
-                                    AND   (categorycode = ? OR categorycode = ?)
-                                    AND   itemtype IS NOT NULL
-                                    AND   rule_name = 'maxissueqty'
-                                  ) ";
+                $count_query .= " WHERE items.itype NOT IN ( $issuing_itemtypes_query )";
             } else {
-                $count_query .= " JOIN  biblioitems USING (biblionumber)
-                                  WHERE biblioitems.itemtype NOT IN (
-                                    SELECT itemtype FROM circulation_rules
-                                    WHERE branchcode = ?
-                                    AND   (categorycode = ? OR categorycode = ?)
-                                    AND   itemtype IS NOT NULL
-                                    AND   rule_name = 'maxissueqty'
-                                  ) ";
+                $count_query .= " WHERE biblioitems.itemtype NOT IN ( $issuing_itemtypes_query )";
             }
             push @bind_params, $maxissueqty_rule->branchcode;
             push @bind_params, $maxissueqty_rule->categorycode;
             push @bind_params, $cat_borrower;
         } else {
-            # rule has specific item type, so count loans of that
-            # specific item type
+            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 = ? ";
+                $count_query .= " WHERE items.itype IN (" . $types_param . ")";
             } else { 
                 $count_query .= " JOIN  biblioitems USING (biblionumber) 
-                                  WHERE biblioitems.itemtype= ? ";
+                                  WHERE biblioitems.itemtype IN (" . $types_param . ")";
             }
-            push @bind_params, $type;
+            push @bind_params, @types;
         }
 
         $count_query .= " AND borrowernumber = ? ";
@@ -470,38 +504,53 @@ sub TooMany {
             }
         }
 
-        my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $count_query, {}, @bind_params );
+        my ( $checkout_count_type, $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $count_query, {}, @bind_params );
 
-        my $max_checkouts_allowed = $maxissueqty_rule ? $maxissueqty_rule->rule_value : undef;
         my $max_onsite_checkouts_allowed = $maxonsiteissueqty_rule ? $maxonsiteissueqty_rule->rule_value : undef;
 
-        if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) {
-            if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
-                return {
-                    reason => 'TOO_MANY_ONSITE_CHECKOUTS',
-                    count => $onsite_checkout_count,
-                    max_allowed => $max_onsite_checkouts_allowed,
-                }
-            }
-        }
-        if ( C4::Context->preference('ConsiderOnSiteCheckoutsAsNormalCheckouts') ) {
-            my $delta = $switch_onsite_checkout ? 1 : 0;
-            if ( $checkout_count >= $max_checkouts_allowed + $delta ) {
-                return {
-                    reason => 'TOO_MANY_CHECKOUTS',
-                    count => $checkout_count,
-                    max_allowed => $max_checkouts_allowed,
-                };
-            }
-        } elsif ( not $onsite_checkout ) {
-            if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed )  {
-                return {
-                    reason => 'TOO_MANY_CHECKOUTS',
-                    count => $checkout_count - $onsite_checkout_count,
-                    max_allowed => $max_checkouts_allowed,
-                };
-            }
+        # If parent rules exists
+        if ( defined($parent_maxissueqty_rule) and defined($parent_maxissueqty_rule->rule_value) ){
+            my $max_checkouts_allowed = $parent_maxissueqty_rule->rule_value;
+
+            my $qty_over = _check_max_qty({
+                checkout_count => $checkout_count,
+                onsite_checkout_count => $onsite_checkout_count,
+                onsite_checkout => $onsite_checkout,
+                max_checkouts_allowed => $max_checkouts_allowed,
+                max_onsite_checkouts_allowed => $max_onsite_checkouts_allowed,
+                switch_onsite_checkout       => $switch_onsite_checkout
+            });
+            return $qty_over if defined $qty_over;
+
+
+           # If the parent rule is less than or equal to the child, we only need check the parent
+           if( $maxissueqty_rule->rule_value < $parent_maxissueqty_rule->rule_value && defined($maxissueqty_rule->itemtype) ) {
+               my $max_checkouts_allowed = $maxissueqty_rule->rule_value;
+               my $qty_over = _check_max_qty({
+                   checkout_count => $checkout_count_type,
+                   onsite_checkout_count => $onsite_checkout_count,
+                   onsite_checkout => $onsite_checkout,
+                   max_checkouts_allowed => $max_checkouts_allowed,
+                   max_onsite_checkouts_allowed => $max_onsite_checkouts_allowed,
+                   switch_onsite_checkout       => $switch_onsite_checkout
+               });
+               return $qty_over if defined $qty_over;
+           }
+
+        } else {
+            my $max_checkouts_allowed = $maxissueqty_rule->rule_value;
+            my $qty_over = _check_max_qty({
+                checkout_count => $checkout_count,
+                onsite_checkout_count => $onsite_checkout_count,
+                onsite_checkout => $onsite_checkout,
+                max_checkouts_allowed => $max_checkouts_allowed,
+                max_onsite_checkouts_allowed => $max_onsite_checkouts_allowed,
+                switch_onsite_checkout       => $switch_onsite_checkout
+            });
+            return $qty_over if defined $qty_over;
         }
+
+
     }
 
     # Now count total loans against the limit for the branch
@@ -527,35 +576,18 @@ sub TooMany {
         }
         my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $branch_count_query, {}, @bind_params );
         my $max_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxissueqty};
-        my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxonsiteissueqty};
-
-        if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) {
-            if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
-                return {
-                    reason => 'TOO_MANY_ONSITE_CHECKOUTS',
-                    count => $onsite_checkout_count,
-                    max_allowed => $max_onsite_checkouts_allowed,
-                }
-            }
-        }
-        if ( C4::Context->preference('ConsiderOnSiteCheckoutsAsNormalCheckouts') ) {
-            my $delta = $switch_onsite_checkout ? 1 : 0;
-            if ( $checkout_count >= $max_checkouts_allowed + $delta ) {
-                return {
-                    reason => 'TOO_MANY_CHECKOUTS',
-                    count => $checkout_count,
-                    max_allowed => $max_checkouts_allowed,
-                };
-            }
-        } elsif ( not $onsite_checkout ) {
-            if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed )  {
-                return {
-                    reason => 'TOO_MANY_CHECKOUTS',
-                    count => $checkout_count - $onsite_checkout_count,
-                    max_allowed => $max_checkouts_allowed,
-                };
-            }
-        }
+        my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxonsiteissueqty} || undef;
+
+        my $qty_over = _check_max_qty({
+            checkout_count => $checkout_count,
+            onsite_checkout_count => $onsite_checkout_count,
+            onsite_checkout => $onsite_checkout,
+            max_checkouts_allowed => $max_checkouts_allowed,
+            max_onsite_checkouts_allowed => $max_onsite_checkouts_allowed,
+            switch_onsite_checkout       => $switch_onsite_checkout
+        });
+        return $qty_over if defined $qty_over;
+
     }
 
     if ( not defined( $maxissueqty_rule ) and not defined($branch_borrower_circ_rule->{patron_maxissueqty}) ) {
@@ -566,6 +598,49 @@ sub TooMany {
     return;
 }
 
+sub _check_max_qty {
+    my $params = shift;
+    my $checkout_count = $params->{checkout_count};
+    my $onsite_checkout_count = $params->{onsite_checkout_count};
+    my $onsite_checkout = $params->{onsite_checkout};
+    my $max_checkouts_allowed = $params->{max_checkouts_allowed};
+    my $max_onsite_checkouts_allowed = $params->{max_onsite_checkouts_allowed};
+    my $switch_onsite_checkout = $params->{switch_onsite_checkout};
+
+    if ( $onsite_checkout and defined $max_onsite_checkouts_allowed ) {
+        if( $max_onsite_checkouts_allowed eq '' ){ return;}
+        if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
+            return {
+                reason => 'TOO_MANY_ONSITE_CHECKOUTS',
+                count => $onsite_checkout_count,
+                max_allowed => $max_onsite_checkouts_allowed,
+            }
+        }
+    }
+    if ( C4::Context->preference('ConsiderOnSiteCheckoutsAsNormalCheckouts') ) {
+        if( $max_checkouts_allowed eq '' ){ return;}
+        my $delta = $switch_onsite_checkout ? 1 : 0;
+        if ( $checkout_count >= $max_checkouts_allowed + $delta ) {
+            return {
+                reason => 'TOO_MANY_CHECKOUTS',
+                count => $checkout_count,
+                max_allowed => $max_checkouts_allowed,
+            };
+        }
+    } elsif ( not $onsite_checkout ) {
+        if( $max_checkouts_allowed eq '' ){ return;}
+        if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed )  {
+            return {
+                reason => 'TOO_MANY_CHECKOUTS',
+                count => $checkout_count - $onsite_checkout_count,
+                max_allowed => $max_checkouts_allowed,
+            };
+        }
+    }
+
+    return;
+}
+
 =head2 CanBookBeIssued
 
   ( $issuingimpossible, $needsconfirmation, [ $alerts ] ) =  CanBookBeIssued( $patron,
index 31ec015..5b16457 100644 (file)
@@ -15,7 +15,7 @@
 # with Koha; if not, see <http://www.gnu.org/licenses>.
 
 use Modern::Perl;
-use Test::More tests => 9;
+use Test::More tests => 10;
 use C4::Context;
 
 use C4::Members;
@@ -707,6 +707,268 @@ subtest 'empty string means unlimited' => sub {
         C4::Circulation::TooMany( $patron, $item_object, { onsite_checkout => 1 } ),
         undef,
         'maxonsiteissueqty="" should mean unlimited'
+      );
+};
+
+subtest 'itemtype group tests' => sub {
+    plan tests => 13;
+
+    t::lib::Mocks::mock_preference( 'CircControl', 'ItemHomeLibrary' );
+    Koha::CirculationRules->set_rules(
+        {
+            branchcode   => '*',
+            categorycode => '*',
+            itemtype     => '*',
+            rules        => {
+                maxissueqty       => '',
+                maxonsiteissueqty => '',
+                issuelength       => 1,
+                firstremind       => 1,      # 1 day of grace
+                finedays          => 2,      # 2 days of fine per day of overdue
+                lengthunit        => 'days',
+            }
+        },
+    );
+
+    my $parent_itype = $builder->build(
+        {
+            source => 'Itemtype',
+            value  => {
+                parent_type         => undef,
+                rentalcharge        => undef,
+                rentalcharge_daily  => undef,
+                rentalcharge_hourly => undef,
+                notforloan          => 0,
+            }
+        }
+    );
+    my $child_itype_1 = $builder->build(
+        {
+            source => 'Itemtype',
+            value  => {
+                parent_type         => $parent_itype->{itemtype},
+                rentalcharge        => 0,
+                rentalcharge_daily  => 0,
+                rentalcharge_hourly => 0,
+                notforloan          => 0,
+            }
+        }
+    );
+    my $child_itype_2 = $builder->build(
+        {
+            source => 'Itemtype',
+            value  => {
+                parent_type         => $parent_itype->{itemtype},
+                rentalcharge        => 0,
+                rentalcharge_daily  => 0,
+                rentalcharge_hourly => 0,
+                notforloan          => 0,
+            }
+        }
+    );
+
+    my $branch   = $builder->build( { source => 'Branch', } );
+    my $category = $builder->build( { source => 'Category', } );
+    my $patron   = $builder->build(
+        {
+            source => 'Borrower',
+            value  => {
+                categorycode => $category->{categorycode},
+                branchcode   => $branch->{branchcode},
+            },
+        }
+    );
+    my $item = $builder->build_sample_item(
+        {
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            itype         => $child_itype_1->{itemtype}
+        }
+    );
+
+    my $all_iq_rule = $builder->build(
+        {
+            source => 'CirculationRule',
+            value  => {
+                branchcode   => $branch->{branchcode},
+                categorycode => $category->{categorycode},
+                itemtype     => undef,
+                rule_name    => 'maxissueqty',
+                rule_value   => 1
+            }
+        }
+    );
+    is( C4::Circulation::TooMany( $patron, $item ),
+        undef, 'Checkout allowed, using all rule of 1' );
+
+    #Checkout an item
+    my $issue =
+      C4::Circulation::AddIssue( $patron, $item->barcode, dt_from_string() );
+    like( $issue->issue_id, qr|^\d+$|, 'The issue should have been inserted' );
+
+    #Patron has 1 checkout of child itype1
+
+    my $parent_iq_rule = $builder->build(
+        {
+            source => 'CirculationRule',
+            value  => {
+                branchcode   => $branch->{branchcode},
+                categorycode => $category->{categorycode},
+                itemtype     => $parent_itype->{itemtype},
+                rule_name    => 'maxissueqty',
+                rule_value   => 2
+            }
+        }
+    );
+
+    is( C4::Circulation::TooMany( $patron, $item ),
+        undef, 'Checkout allowed, using parent type rule of 2' );
+
+    my $child1_iq_rule = $builder->build_object(
+        {
+            class => 'Koha::CirculationRules',
+            value => {
+                branchcode   => $branch->{branchcode},
+                categorycode => $category->{categorycode},
+                itemtype     => $child_itype_1->{itemtype},
+                rule_name    => 'maxissueqty',
+                rule_value   => 1
+            }
+        }
+    );
+
+    is_deeply(
+        C4::Circulation::TooMany( $patron, $item ),
+        {
+            reason      => 'TOO_MANY_CHECKOUTS',
+            count       => 1,
+            max_allowed => 1,
+        },
+        'Checkout not allowed, using specific type rule of 1'
+    );
+
+    my $item_1 = $builder->build_sample_item(
+        {
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            itype         => $child_itype_2->{itemtype}
+        }
+    );
+
+    my $child2_iq_rule = $builder->build(
+        {
+            source => 'CirculationRule',
+            value  => {
+                branchcode   => $branch->{branchcode},
+                categorycode => $category->{categorycode},
+                itemtype     => $child_itype_2->{itemtype},
+                rule_name    => 'maxissueqty',
+                rule_value   => 3
+            }
+        }
+    );
+
+    is( C4::Circulation::TooMany( $patron, $item_1 ),
+        undef, 'Checkout allowed' );
+
+    #checkout an item
+    $issue =
+      C4::Circulation::AddIssue( $patron, $item_1->barcode, dt_from_string() );
+    like( $issue->issue_id, qr|^\d+$|, 'the issue should have been inserted' );
+
+    #patron has 1 checkout of childitype1 and 1 checkout of childitype2
+
+    is_deeply(
+        C4::Circulation::TooMany( $patron, $item ),
+        {
+            reason      => 'TOO_MANY_CHECKOUTS',
+            count       => 2,
+            max_allowed => 2,
+        },
+'Checkout not allowed, using parent type rule of 2, checkout of sibling itemtype counted'
+    );
+
+    my $parent_item = $builder->build_sample_item(
+        {
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            itype         => $parent_itype->{itemtype}
+        }
+    );
+
+    is_deeply(
+        C4::Circulation::TooMany( $patron, $parent_item ),
+        {
+            reason      => 'TOO_MANY_CHECKOUTS',
+            count       => 2,
+            max_allowed => 2,
+        },
+'Checkout not allowed, using parent type rule of 2, checkout of child itemtypes counted'
+    );
+
+    #increase parent type to greater than specific
+    my $circ_rule_object =
+      Koha::CirculationRules->find( $parent_iq_rule->{id} );
+    $circ_rule_object->rule_value(4)->store();
+
+    is( C4::Circulation::TooMany( $patron, $item_1 ),
+        undef, 'Checkout allowed, using specific type rule of 3' );
+
+    my $item_2 = $builder->build_sample_item(
+        {
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            itype         => $child_itype_2->{itemtype}
+        }
+    );
+
+    #checkout an item
+    $issue =
+      C4::Circulation::AddIssue( $patron, $item_2->barcode, dt_from_string(),
+        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
+
+    is(
+        C4::Circulation::TooMany( $patron, $item_2 ),
+        undef,
+'Checkout allowed, using specific type rule of 3, checkout of sibling itemtype not counted'
+    );
+
+    $child1_iq_rule->rule_value(2)->store(); #Allow 2 checkouts for child type 1
+
+    my $item_3 = $builder->build_sample_item(
+        {
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            itype         => $child_itype_1->{itemtype}
+        }
+    );
+    my $item_4 = $builder->build_sample_item(
+        {
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            itype         => $child_itype_2->{itemtype}
+        }
+    );
+
+    #checkout an item
+    $issue =
+      C4::Circulation::AddIssue( $patron, $item_4->barcode, dt_from_string(),
+        undef, undef, undef );
+    like( $issue->issue_id, qr|^\d+$|, 'the issue should have been inserted' );
+
+    #patron has 1 checkout of childitype 1 and 3 of childitype2
+
+    is_deeply(
+        C4::Circulation::TooMany( $patron, $item_3 ),
+        {
+            reason      => 'TOO_MANY_CHECKOUTS',
+            max_allowed => 4,
+            count       => 4,
+        },
+'Checkout not allowed, using specific type rule of 2, checkout of sibling itemtype not counted, but parent rule (4) prevents another'
     );
 
     teardown();