From ba160803046aa58d3b6ce0ade8d7e554431ef913 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 13 May 2015 16:56:03 +0200 Subject: [PATCH] Bug 14045: Add specific quotas to on-site checkouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch set adds the ability to defined independent quotas for on-site checkouts. This will be done using the circulation rules matrix where a new column “Current on-site checkouts allow” will be added. This feature is going to use the same method as the existing fields maxissueqty ("Current checkouts allowed"), the new fields will be added to the different tables (see the "DB changes" patch) and will be named maxonsiteissueqty (for consistency). In order to keep the existing behavior and to let more flexibility, a new system preference is added (ConsiderOnSiteCheckoutsAsNormalCheckouts). This syspref will let the liberty to the library to decide if an on-site checkout should be considered as a "normal" checkout or not. To keep the existing behavior, the syspref will be disabled (i.e. an on-site checkout is considered as a normal checkout) and the number of on-site checkouts will be the same as the number of checkout (maxissueqty == maxonsiteissueqty). Technically: There are only very few tests for the Circulation module, and the 2 subroutines impacted by this patch set were not tested at all. It is necessary to introduce non-regression tests for this area. The 2 subroutines are: C4::Circulation::GetBranchBorrowerCircRule and C4::Circulation::TooMany (only called by C4::Circulation::CanBookBeIssued, so we will take the liberty to change the prototype to raise a better warning to the end user). Test plan: I. Confirm there is no regression and the existing behavior is kept 0/ Let the syspref disabled 1/ Set a rule to limit to 2 the number of checkouts allowed 2/ Do a normal checkout 3/ Do an on-site checkout 4/ Try to checkout (on-site or normal) an item again. You should not be allowed. II. Test the new feature - pref disabled 0/ Let the syspref disabled 1/ Set a rule to limit to 2 the number of checkouts allowed and to 1 the number of on-site checkouts allowed. 2/ Do an on-site checkout 3/ Try to do another one, you should not be allowed to do it. 4/ A normal checkout should pass successfully Note that it does not make sense to have the number of on-site checkouts alowed > number of checkouts allowed. III. Test the new feature - pref enabled 0/ Enable the syspref Now an on-site checkout is *not* counted as a normal checkout. This means you can have the number of on-site checkouts > number of checkouts allowed. 1/ Set the values you want for the 2 types of checkouts (normal vs on-site). 2/ Even if a patron has reached the maximum of checkouts allowed, he will be allowed to do a on-site checkout (vice versa). IV. Stress the developper Using the different configurations available in the circulation matrix, try to find one where the checkout is allowed and not should be. Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Nicolas Legrand --- C4/Circulation.pm | 69 +++++-- circ/circulation.pl | 2 +- t/db_dependent/Circulation/TooMany.t | 331 ++++++++++++++++++++++++++++++++++ 3 files changed, 382 insertions(+), 20 deletions(-) create mode 100644 t/db_dependent/Circulation/TooMany.t diff --git a/C4/Circulation.pm b/C4/Circulation.pm index f991680..075c88c 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -378,6 +378,8 @@ sub TooMany { my $borrower = shift; my $biblionumber = shift; my $item = shift; + my $params = shift; + my $onsite_checkout = $params->{onsite_checkout} || 0; my $cat_borrower = $borrower->{'categorycode'}; my $dbh = C4::Context->dbh; my $branch; @@ -396,8 +398,11 @@ sub TooMany { # rule if (defined($issuing_rule) and defined($issuing_rule->{'maxissueqty'})) { my @bind_params; - my $count_query = "SELECT COUNT(*) FROM issues - JOIN items USING (itemnumber) "; + my $count_query = q| + SELECT COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts + FROM issues + JOIN items USING (itemnumber) + |; my $rule_itemtype = $issuing_rule->{itemtype}; if ($rule_itemtype eq "*") { @@ -450,13 +455,24 @@ sub TooMany { } } - my $count_sth = $dbh->prepare($count_query); - $count_sth->execute(@bind_params); - my ($current_loan_count) = $count_sth->fetchrow_array; + my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $count_query, {}, @bind_params ); + + my $max_checkouts_allowed = $issuing_rule->{maxissueqty}; + my $max_onsite_checkouts_allowed = $issuing_rule->{maxonsiteissueqty}; - my $max_loans_allowed = $issuing_rule->{'maxissueqty'}; - if ($current_loan_count >= $max_loans_allowed) { - return ($current_loan_count, $max_loans_allowed); + if ( $onsite_checkout ) { + if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { + return ($onsite_checkout_count, $max_onsite_checkouts_allowed); + } + } + if ( C4::Context->preference('ConsiderOnSiteCheckoutsAsNormalCheckouts') ) { + if ( $checkout_count >= $max_checkouts_allowed ) { + return ($checkout_count, $max_checkouts_allowed); + } + } elsif ( not $onsite_checkout ) { + if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { + return ($checkout_count - $onsite_checkout_count, $max_checkouts_allowed); + } } } @@ -464,9 +480,12 @@ sub TooMany { my $branch_borrower_circ_rule = GetBranchBorrowerCircRule($branch, $cat_borrower); if (defined($branch_borrower_circ_rule->{maxissueqty})) { my @bind_params = (); - my $branch_count_query = "SELECT COUNT(*) FROM issues - JOIN items USING (itemnumber) - WHERE borrowernumber = ? "; + my $branch_count_query = q| + SELECT COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts + FROM issues + JOIN items USING (itemnumber) + WHERE borrowernumber = ? + |; push @bind_params, $borrower->{borrowernumber}; if (C4::Context->preference('CircControl') eq 'PickupLibrary') { @@ -478,13 +497,23 @@ sub TooMany { $branch_count_query .= " AND items.homebranch = ? "; push @bind_params, $branch; } - my $branch_count_sth = $dbh->prepare($branch_count_query); - $branch_count_sth->execute(@bind_params); - my ($current_loan_count) = $branch_count_sth->fetchrow_array; + my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $branch_count_query, {}, @bind_params ); + my $max_checkouts_allowed = $branch_borrower_circ_rule->{maxissueqty}; + my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{maxonsiteissueqty}; - my $max_loans_allowed = $branch_borrower_circ_rule->{maxissueqty}; - if ($current_loan_count >= $max_loans_allowed) { - return ($current_loan_count, $max_loans_allowed); + if ( $onsite_checkout ) { + if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { + return ($onsite_checkout_count, $max_onsite_checkouts_allowed); + } + } + if ( C4::Context->preference('ConsiderOnSiteCheckoutsAsNormalCheckouts') ) { + if ( $checkout_count >= $max_checkouts_allowed ) { + return ($checkout_count, $max_checkouts_allowed); + } + } elsif ( not $onsite_checkout ) { + if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { + return ($checkout_count - $onsite_checkout_count, $max_checkouts_allowed); + } } } @@ -695,11 +724,13 @@ if the borrower borrows to much things =cut sub CanBookBeIssued { - my ( $borrower, $barcode, $duedate, $inprocess, $ignore_reserves ) = @_; + my ( $borrower, $barcode, $duedate, $inprocess, $ignore_reserves, $params ) = @_; my %needsconfirmation; # filled with problems that needs confirmations my %issuingimpossible; # filled with problems that causes the issue to be IMPOSSIBLE my %alerts; # filled with messages that shouldn't stop issuing, but the librarian should be aware of. + my $onsite_checkout = $params->{onsite_checkout} || 0; + my $item = GetItem(GetItemnumberFromBarcode( $barcode )); my $issue = GetItemIssue($item->{itemnumber}); my $biblioitem = GetBiblioItemData($item->{biblioitemnumber}); @@ -839,7 +870,7 @@ sub CanBookBeIssued { # # JB34 CHECKS IF BORROWERS DONT HAVE ISSUE TOO MANY BOOKS # - my ($current_loan_count, $max_loans_allowed) = TooMany( $borrower, $item->{biblionumber}, $item ); + my ($current_loan_count, $max_loans_allowed) = TooMany( $borrower, $item->{biblionumber}, $item, { onsite_checkout => $onsite_checkout } ); # if TooMany max_loans_allowed returns 0 the user doesn't have permission to check out this book if (defined $max_loans_allowed && $max_loans_allowed == 0) { $needsconfirmation{PATRON_CANT} = 1; diff --git a/circ/circulation.pl b/circ/circulation.pl index ab6db3b..3d59231 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -300,7 +300,7 @@ if ($borrowernumber) { if ($barcode) { # always check for blockers on issuing my ( $error, $question, $alerts ) = - CanBookBeIssued( $borrower, $barcode, $datedue , $inprocess ); + CanBookBeIssued( $borrower, $barcode, $datedue , $inprocess, undef, { onsite_checkout => $onsite_checkout } ); my $blocker = $invalidduedate ? 1 : 0; $template->param( alert => $alerts ); diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t new file mode 100644 index 0000000..bf158e5 --- /dev/null +++ b/t/db_dependent/Circulation/TooMany.t @@ -0,0 +1,331 @@ +#!/usr/bin/perl + +use Modern::Perl; +use Test::More tests => 6; +use C4::Context; + +use C4::Biblio; +use C4::Members; +use C4::Branch; +use C4::Circulation; +use C4::Items; +use C4::Context; + +use Koha::DateUtils qw( dt_from_string ); + +use t::lib::TestBuilder; +use t::lib::Mocks; + +our $dbh = C4::Context->dbh; +$dbh->{AutoCommit} = 0; +$dbh->{RaiseError} = 1; + +$dbh->do(q|DELETE FROM issues|); +$dbh->do(q|DELETE FROM items|); +$dbh->do(q|DELETE FROM borrowers|); +$dbh->do(q|DELETE FROM branches|); +$dbh->do(q|DELETE FROM categories|); +$dbh->do(q|DELETE FROM accountlines|); +$dbh->do(q|DELETE FROM itemtypes|); +$dbh->do(q|DELETE FROM branch_item_rules|); +$dbh->do(q|DELETE FROM branch_borrower_circ_rules|); +$dbh->do(q|DELETE FROM default_branch_circ_rules|); +$dbh->do(q|DELETE FROM default_circ_rules|); +$dbh->do(q|DELETE FROM default_branch_item_rules|); + +my $builder = t::lib::TestBuilder->new(); + +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 $biblio = $builder->build({ + source => 'Biblio', + value => { + branchcode => $branch->{branchcode}, + }, +}); +my $item = $builder->build({ + source => 'Item', + value => { + biblionumber => $biblio->{biblionumber}, + homebranch => $branch->{branchcode}, + holdingbranch => $branch->{branchcode}, + }, +}); + +C4::Context->_new_userenv ('DUMMY_SESSION_ID'); +C4::Context->set_userenv($patron->{borrowernumber}, $patron->{userid}, 'usercnum', 'First name', 'Surname', $branch->{branchcode}, 'My Library', 0); + +# TooMany return ($current_loan_count, $max_loans_allowed) or undef +# CO = Checkout +# OSCO: On-site checkout + +subtest 'no rules exist' => sub { + plan tests => 2; + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ), + undef, + 'CO should be allowed, in any cases' + ); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ), + undef, + 'OSCO should be allowed, in any cases' + ); +}; + +subtest '1 Issuingrule exist 0 0: no issue allowed' => sub { + plan tests => 4; + my $issuingrule = $builder->build({ + source => 'Issuingrule', + value => { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => '*', + maxissueqty => 0, + maxonsiteissueqty => 0, + }, + }); + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 0, 0 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 0, 0 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 0, 0 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 0, 0 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + + teardown(); +}; + +subtest '1 Issuingrule exist 1 1: issue is allowed' => sub { + plan tests => 4; + my $issuingrule = $builder->build({ + source => 'Issuingrule', + value => { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => '*', + maxissueqty => 1, + maxonsiteissueqty => 1, + }, + }); + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ), + undef, + 'CO should be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ), + undef, + 'OSCO should be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ), + undef, + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ), + undef, + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + + teardown(); +}; + +subtest '1 Issuingrule exist: 1 CO allowed, 1 OSCO allowed. Do a CO' => sub { + plan tests => 5; + my $issuingrule = $builder->build({ + source => 'Issuingrule', + value => { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => '*', + maxissueqty => 1, + maxonsiteissueqty => 1, + }, + }); + + my $issue = C4::Circulation::AddIssue( $patron, $item->{barcode}, dt_from_string() ); + like( $issue->issue_id, qr|^\d+$|, 'The issue should have been inserted' ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 1, 1 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ), + undef, + 'OSCO should be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 1, 1 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 1, 1 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + + teardown(); +}; + +subtest '1 Issuingrule exist: 1 CO allowed, 1 OSCO allowed, Do a OSCO' => sub { + plan tests => 5; + my $issuingrule = $builder->build({ + source => 'Issuingrule', + value => { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => '*', + maxissueqty => 1, + maxonsiteissueqty => 1, + }, + }); + + my $issue = C4::Circulation::AddIssue( $patron, $item->{barcode}, dt_from_string(), undef, undef, undef, { onsite_checkout => 1 } ); + like( $issue->issue_id, qr|^\d+$|, 'The issue should have been inserted' ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ), + undef, + 'CO should be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 1, 1 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 1, 1 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 1, 1 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + + teardown(); +}; + +subtest '1 BranchBorrowerCircRule exist: 1 CO allowed, 1 OSCO allowed' => sub { + # Note: the same test coul be done for + # DefaultBorrowerCircRule, DefaultBranchCircRule, DefaultBranchItemRule ans DefaultCircRule.pm + + plan tests => 10; + my $issuingrule = $builder->build({ + source => 'BranchBorrowerCircRule', + value => { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + maxissueqty => 1, + maxonsiteissueqty => 1, + }, + }); + + my $issue = C4::Circulation::AddIssue( $patron, $item->{barcode}, dt_from_string(), undef, undef, undef ); + like( $issue->issue_id, qr|^\d+$|, 'The issue should have been inserted' ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 1, 1 ], + 'CO should be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ), + undef, + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 1, 1 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 1, 1 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + + teardown(); + + $issue = C4::Circulation::AddIssue( $patron, $item->{barcode}, dt_from_string(), undef, undef, undef, { onsite_checkout => 1 } ); + like( $issue->issue_id, qr|^\d+$|, 'The issue should have been inserted' ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); + is( + C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ), + undef, + 'CO should be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 1, 1 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 0' + ); + + t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item ) ], + [ 1, 1 ], + 'CO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + is_deeply( + [ C4::Circulation::TooMany( $patron, $biblio->{biblionumber}, $item, { onsite_checkout => 1 } ) ], + [ 1, 1 ], + 'OSCO should not be allowed if ConsiderOnSiteCheckoutsAsNormalCheckouts == 1' + ); + + teardown(); +}; + +sub teardown { + $dbh->do(q|DELETE FROM issues|); + $dbh->do(q|DELETE FROM issuingrules|); +} -- 1.7.10.4