From a75ff2d5d7507396270f961134b509a818d7f9a8 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 1 May 2019 09:57:50 +0000 Subject: [PATCH] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea --- C4/Circulation.pm | 210 +++++++++++++++++++++---------- t/db_dependent/Circulation/TooMany.t | 233 ++++++++++++++++++++++++++++++++++- 2 files changed, 378 insertions(+), 65 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b9ff0f2824..f6b04facb8 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -389,8 +389,26 @@ sub TooMany { ? $item->{'itype'} # item-level : $item->{'itemtype'}; # biblio-level + 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 && $parent_maxissueqty_rule->itemtype eq '*'; + my $maxissueqty_rule = Koha::CirculationRules->get_effective_rule( { categorycode => $cat_borrower, @@ -399,6 +417,8 @@ sub TooMany { rule_name => 'maxissueqty', } ); + + my $maxonsiteissueqty_rule = Koha::CirculationRules->get_effective_rule( { categorycode => $cat_borrower, @@ -413,9 +433,17 @@ sub TooMany { # how many loans the patron already has that meet that # rule 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.itype = '| .$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) |; @@ -445,15 +473,31 @@ sub TooMany { 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 + push @types, $parent_type; + my $children = Koha::ItemTypes->search({ parent_type => $parent_type }); + while ( my $child = $children->next ){ + push @types, $child->itemtype; + } + } elsif ( $child_types ) { + # If we are a parent type, we need to count all child types and our own type + while ( my $child = $child_types->next ){ + push @types, $child->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 = ? "; @@ -471,38 +515,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 defined $max_onsite_checkouts_allowed ) { - 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 @@ -528,35 +587,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}) ) { @@ -567,6 +609,46 @@ 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 ( $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, + }; + } + } + + return; +} + =head2 CanBookBeIssued ( $issuingimpossible, $needsconfirmation, [ $alerts ] ) = CanBookBeIssued( $patron, diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t index 1f1c6a7ee0..6c1c2a29f0 100644 --- a/t/db_dependent/Circulation/TooMany.t +++ b/t/db_dependent/Circulation/TooMany.t @@ -15,7 +15,7 @@ # with Koha; if not, see . use Modern::Perl; -use Test::More tests => 8; +use Test::More tests => 9; use C4::Context; use C4::Members; @@ -659,7 +659,238 @@ subtest 'General vs specific rules limit quantity correctly' => sub { 'We are allowed one from the branch specifically now' ); +}; + +subtest 'itemtype group tests' => sub { + plan tests => 13; + + t::lib::Mocks::mock_preference('CircControl', 'ItemHomeLibrary'); + my $rule = $builder->build({ + source => 'Issuingrule', + value => { + categorycode => '*', + itemtype => '*', + branchcode => '*', + 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->biblionumber, $item->unblessed ), + 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->biblionumber, $item->unblessed ), + 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->biblionumber, $item->unblessed ), + { + 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->biblionumber, $item_1->unblessed ), + 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->biblionumber, $item->unblessed ), + { + 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->biblionumber, $parent_item->unblessed ), + { + 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->biblionumber, $item_1->unblessed ), + 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->biblionumber, $item_2->unblessed ), + 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->biblionumber, $item_3->unblessed ), + { + 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' + ); }; -- 2.11.0