From 274742bfd14115879c79554c31508683b7253cf2 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 7 Jul 2015 11:22:31 -0400 Subject: [PATCH] Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold Koha is currently not engineered to handle multiple holds per record. Until such time that is does, we should not allow them to be created. Test Plan: 1) Apply this patch 2) Log in to the opac 3) Place a hold 4) Hit the back button on your browser 5) Place the hold again 6) Note the new message --- C4/Reserves.pm | 52 +++++++++++++------ .../opac-tmpl/bootstrap/en/modules/opac-user.tt | 7 +++ opac/opac-reserve.pl | 6 ++- opac/opac-user.pl | 18 +++---- t/db_dependent/Reserves.t | 14 ++++- 5 files changed, 65 insertions(+), 32 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index dab2b48..47d0fa4 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -40,8 +40,11 @@ use C4::Dates qw( format_date_in_iso ); use Koha::DateUtils; use Koha::Calendar; use Koha::Database; +use Koha::Hold; +use Koha::Holds; use List::MoreUtils qw( firstidx any ); +use Carp; use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -155,18 +158,31 @@ sub AddReserve { $bibitems, $priority, $resdate, $expdate, $notes, $title, $checkitem, $found ) = @_; + + if ( Koha::Holds->search( { borrowernumber => $borrowernumber, biblionumber => $biblionumber } )->count() > 0 ) { + carp("AddReserve: borrower $borrowernumber already has a hold for biblionumber $biblionumber"); + return; + } + + my $fee = GetReserveFee( $borrowernumber, $biblionumber ); + my $dbh = C4::Context->dbh; + $resdate = format_date_in_iso( $resdate ) if ( $resdate ); $resdate = C4::Dates->today( 'iso' ) unless ( $resdate ); + if ($expdate) { $expdate = format_date_in_iso( $expdate ); } else { undef $expdate; # make reserves.expirationdate default to null rather than '0000-00-00' } - if ( C4::Context->preference( 'AllowHoldDateInFuture' ) ) { - # Make room in reserves for this before those of a later reserve date - $priority = _ShiftPriorityByDateAndPriority( $biblionumber, $resdate, $priority ); + + if ( C4::Context->preference('AllowHoldDateInFuture') ) { + + # Make room in reserves for this before those of a later reserve date + $priority = _ShiftPriorityByDateAndPriority( $biblionumber, $resdate, $priority ); } + my $waitingdate; # If the reserv had the waiting status, we had the value of the resdate @@ -175,20 +191,22 @@ sub AddReserve { } # updates take place here - my $query = qq{ - INSERT INTO reserves - (borrowernumber,biblionumber,reservedate,branchcode, - priority,reservenotes,itemnumber,found,waitingdate,expirationdate) - VALUES - (?,?,?,?,?, - ?,?,?,?,?) - }; - my $sth = $dbh->prepare($query); - $sth->execute( - $borrowernumber, $biblionumber, $resdate, $branch, $priority, - $notes, $checkitem, $found, $waitingdate, $expdate - ); - my $reserve_id = $sth->{mysql_insertid}; + my $hold = Koha::Hold->new( + { + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + reservedate => $resdate, + branchcode => $branch, + priority => $priority, + reservenotes => $notes, + itemnumber => $checkitem, + found => $found, + waitingdate => $waitingdate, + expirationdate => $expdate + } + )->store(); + my $reserve_id = $hold->id(); + # add a reserve fee if needed my $fee = GetReserveFee( $borrowernumber, $biblionumber ); ChargeReserveFee( $borrowernumber, $fee, $title ); diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt index b5bc8d2..8689c3e 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt @@ -48,6 +48,13 @@ [% IF ( patronupdate ) %]

Thank you!

Your corrections have been submitted to the library, and a staff member will update your record as soon as possible.

[% END %] + [% IF failed_holds %] +
+

Notice:

+

One or more holds were not placed due to existing holds.

+
+ [% END %] + [% IF ( BORROWER_INF.warndeparture ) %]
Please note: Your card will expire on [% BORROWER_INF.warndeparture | $KohaDates %]. Please contact the library for more information. diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 4dff9a2..b399bb9 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -221,6 +221,7 @@ if ( $query->param('place_reserve') ) { &get_out($query, $cookie, $template->output); } + my $failed_holds = 0; while (@selectedItems) { my $biblioNum = shift(@selectedItems); my $itemNum = shift(@selectedItems); @@ -283,7 +284,7 @@ if ( $query->param('place_reserve') ) { # Here we actually do the reserveration. Stage 3. if ($canreserve) { - AddReserve( + my $reserve_id = AddReserve( $branch, $borrowernumber, $biblioNum, [$biblioNum], $rank, @@ -291,11 +292,12 @@ if ( $query->param('place_reserve') ) { $notes, $biblioData->{title}, $itemNum, $found ); + $failed_holds++ unless $reserve_id; ++$reserve_cnt; } } - print $query->redirect("/cgi-bin/koha/opac-user.pl#opac-user-holds"); + print $query->redirect("/cgi-bin/koha/opac-user.pl?failed_holds=$failed_holds#opac-user-holds"); exit; } diff --git a/opac/opac-user.pl b/opac/opac-user.pl index d4fbbe3..c1716ac 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -383,17 +383,15 @@ if ( $borr->{'opacnote'} ) { } $template->param( - bor_messages_loop => GetMessages( $borrowernumber, 'B', 'NONE' ), - waiting_count => $wcount, - patronupdate => $patronupdate, - OpacRenewalAllowed => C4::Context->preference("OpacRenewalAllowed"), - userview => 1, -); - -$template->param( - SuspendHoldsOpac => C4::Context->preference('SuspendHoldsOpac'), + bor_messages_loop => GetMessages( $borrowernumber, 'B', 'NONE' ), + waiting_count => $wcount, + patronupdate => $patronupdate, + OpacRenewalAllowed => C4::Context->preference("OpacRenewalAllowed"), + userview => 1, + SuspendHoldsOpac => C4::Context->preference('SuspendHoldsOpac'), AutoResumeSuspendedHolds => C4::Context->preference('AutoResumeSuspendedHolds'), - OpacHoldNotes => C4::Context->preference('OpacHoldNotes'), + OpacHoldNotes => C4::Context->preference('OpacHoldNotes'), + failed_holds => $query->param('failed_holds'), ); output_html_with_http_headers $query, $cookie, $template->output; diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 107cee9..2cb7148 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -153,6 +153,13 @@ $requesters{'CPL'} = AddMember( categorycode => 'PT', surname => 'borrower from CPL', ); +for my $i ( 2 .. 5 ) { + $requesters{"CPL$i"} = AddMember( + branchcode => 'CPL', + categorycode => 'PT', + surname => 'borrower $i from CPL', + ); +} $requesters{'FPL'} = AddMember( branchcode => 'FPL', categorycode => 'PT', @@ -393,11 +400,12 @@ ModReserveAffect( $itemnumber, $requesters{'CPL'} , 0); #confirm hold is(defined $results[3]?1:0, 1, 'GetReservesFromItemnumber returns a future wait (confirmed future hold)'); # End of tests for bug 9788 +$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); # Tests for CalculatePriority (bug 8918) my $p = C4::Reserves::CalculatePriority($bibnum2); is($p, 4, 'CalculatePriority should now return priority 4'); $resdate=undef; -AddReserve('CPL', $requesters{'CPL'}, $bibnum2, +AddReserve('CPL', $requesters{'CPL2'}, $bibnum2, $bibitems, $p, $resdate, $expdate, $notes, $title, $checkitem, $found); $p = C4::Reserves::CalculatePriority($bibnum2); @@ -416,7 +424,7 @@ ModReserveAffect( $itemnumber, $requesters{'CPL'} , 0); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 1, 'CalculatePriority should now return priority 1'); #add another biblio hold, no resdate -AddReserve('CPL', $requesters{'CPL'}, $bibnum, +AddReserve('CPL', $requesters{'CPL2'}, $bibnum, $bibitems, $p, $resdate, $expdate, $notes, $title, $checkitem, $found); $p = C4::Reserves::CalculatePriority($bibnum); @@ -425,7 +433,7 @@ is($p, 2, 'CalculatePriority should now return priority 2'); C4::Context->set_preference('AllowHoldDateInFuture', 1); $resdate= dt_from_string(); $resdate->add_duration(DateTime::Duration->new(days => 1)); -AddReserve('CPL', $requesters{'CPL'}, $bibnum, +AddReserve('CPL', $requesters{'CPL3'}, $bibnum, $bibitems, $p, output_pref($resdate), $expdate, $notes, $title, $checkitem, $found); $p = C4::Reserves::CalculatePriority($bibnum); -- 1.7.2.5