From 4f3a0a4b89c78c9fff1673787893067f66970cf4 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 Signed-off-by: Lisette Scheer Signed-off-by: Alex Arnaud JD amended patch: tidy the subtest Signed-off-by: Jonathan Druart --- C4/Circulation.pm | 235 ++++++++++++++++-------- t/db_dependent/Circulation/TooMany.t | 264 ++++++++++++++++++++++++++- 2 files changed, 418 insertions(+), 81 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index c94b773292..58b24161c7 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -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, diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t index 31ec01563b..5b164579f5 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 => 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(); -- 2.39.5