From: Colin Campbell Date: Tue, 28 Aug 2012 13:34:55 +0000 (+0100) Subject: Bug 6976 Close loophole allowing borrowers extra holds via opac X-Git-Url: http://git.rot13.org/?p=koha.git;a=commitdiff_plain;h=3b608056d9cd86375b51d63a56c054fec1f4a16d Bug 6976 Close loophole allowing borrowers extra holds via opac The check that the max number of reserves has not been reached needs to take account of the fact that we may have added enouch reserves to reach the limit while this script is running. Add a check against the incrementing count The variable canreserve was only used when looping through multiple holds but was not being reinitialized on every iteration make variable local to loop it is used in and initialize to zero Have used perltidy to correct the indention of the while loop as it was misleading to the reader Some numeric comparisons were using a string operator fix them before strange bugs manifest themselves The loophole manifests thus: borrowers are only allowed Y holds. If holds are done individually, it is obeying that law i.e. X has held Y separate books – when I tried to do a 6th book it told me that I could not place a hold. However, if a borrower checks the catalogue, gets the intial list up and clicks items on the “results” box and then clicks “place hold” they can hold more than Y items Please enter the commit message for your changes. Lines starting Signed-off-by: Chris Cormack Signed-off-by: Paul Poulain --- diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 2bbb4be2f6..bb23752a04 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -172,7 +172,10 @@ foreach my $biblioNumber (@biblionumbers) { # if ( $query->param('place_reserve') ) { my $notes = $query->param('notes'); - my $canreserve=0; + my $reserve_cnt = 0; + if ($MAXIMUM_NUMBER_OF_RESERVES) { + $reserve_cnt = GetReservesFromBorrowernumber( $borrowernumber ); + } # List is composed of alternating biblio/item/branch my $selectedItems = $query->param('selecteditems'); @@ -203,55 +206,74 @@ if ( $query->param('place_reserve') ) { while (@selectedItems) { my $biblioNum = shift(@selectedItems); my $itemNum = shift(@selectedItems); - my $branch = shift(@selectedItems); # i.e., branch code, not name + my $branch = shift(@selectedItems); # i.e., branch code, not name + + my $canreserve = 0; my $singleBranchMode = C4::Context->preference("singleBranchMode"); - if ($singleBranchMode || ! $OPACChooseBranch) { # single branch mode or disabled user choosing + if ( $singleBranchMode || !$OPACChooseBranch ) + { # single branch mode or disabled user choosing $branch = $borr->{'branchcode'}; } - #item may belong to a host biblio, if yes change biblioNum to hosts bilbionumber - if ($itemNum ne '') { - my $hostbiblioNum = GetBiblionumberFromItemnumber($itemNum); - if ($hostbiblioNum ne $biblioNum) { - $biblioNum = $hostbiblioNum; - } - } +#item may belong to a host biblio, if yes change biblioNum to hosts bilbionumber + if ( $itemNum ne '' ) { + my $hostbiblioNum = GetBiblionumberFromItemnumber($itemNum); + if ( $hostbiblioNum ne $biblioNum ) { + $biblioNum = $hostbiblioNum; + } + } my $biblioData = $biblioDataHash{$biblioNum}; my $found; - # Check for user supplied reserve date - my $startdate; - if ( - C4::Context->preference( 'AllowHoldDateInFuture' ) && - C4::Context->preference( 'OPACAllowHoldDateInFuture' ) - ) { - $startdate = $query->param("reserve_date_$biblioNum"); - } - - my $expiration_date = $query->param("expiration_date_$biblioNum"); + # Check for user supplied reserve date + my $startdate; + if ( C4::Context->preference('AllowHoldDateInFuture') + && C4::Context->preference('OPACAllowHoldDateInFuture') ) + { + $startdate = $query->param("reserve_date_$biblioNum"); + } + + my $expiration_date = $query->param("expiration_date_$biblioNum"); - # If a specific item was selected and the pickup branch is the same as the - # holdingbranch, force the value $rank and $found. + # If a specific item was selected and the pickup branch is the same as the + # holdingbranch, force the value $rank and $found. my $rank = $biblioData->{rank}; - if ($itemNum ne ''){ - $canreserve = 1 if CanItemBeReserved($borrowernumber,$itemNum); + if ( $itemNum ne '' ) { + $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ); $rank = '0' unless C4::Context->preference('ReservesNeedReturns'); my $item = GetItem($itemNum); - if ( $item->{'holdingbranch'} eq $branch ){ - $found = 'W' unless C4::Context->preference('ReservesNeedReturns'); + if ( $item->{'holdingbranch'} eq $branch ) { + $found = 'W' + unless C4::Context->preference('ReservesNeedReturns'); } } else { - $canreserve = 1 if CanBookBeReserved($borrowernumber,$biblioNum); + $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ); + # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; } + if ( $MAXIMUM_NUMBER_OF_RESERVES + && $reserve_cnt >= $MAXIMUM_NUMBER_OF_RESERVES ) + { + $canreserve = 0; + } + # Here we actually do the reserveration. Stage 3. - AddReserve($branch, $borrowernumber, $biblioNum, 'a', [$biblioNum], $rank, $startdate, $expiration_date, $notes, - $biblioData->{'title'}, $itemNum, $found) if ($canreserve); + if ($canreserve) { + AddReserve( + $branch, $borrowernumber, + $biblioNum, 'a', + [$biblioNum], $rank, + $startdate, $expiration_date, + $notes, $biblioData->{title}, + $itemNum, $found + ); + ++$reserve_cnt; + } } print $query->redirect("/cgi-bin/koha/opac-user.pl#opac-user-holds"); @@ -272,14 +294,14 @@ if ( $borr->{'amountoutstanding'} && ($borr->{'amountoutstanding'} > $maxoutstan $noreserves = 1; $template->param( too_much_oweing => $amount ); } -if ( $borr->{gonenoaddress} && ($borr->{gonenoaddress} eq 1) ) { +if ( $borr->{gonenoaddress} && ($borr->{gonenoaddress} == 1) ) { $noreserves = 1; $template->param( message => 1, GNA => 1 ); } -if ( $borr->{lost} && ($borr->{lost} eq 1) ) { +if ( $borr->{lost} && ($borr->{lost} == 1) ) { $noreserves = 1; $template->param( message => 1,