From 55ef5b1c953b6f07d339fbf52a8bf2856a235ab0 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 | 66 ++++++++++---------- Koha/Hold.pm | 52 +++++++++++++++ Koha/Holds.pm | 58 +++++++++++++++++ .../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 +++- 7 files changed, 174 insertions(+), 47 deletions(-) create mode 100644 Koha/Hold.pm create mode 100644 Koha/Holds.pm diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 6252b86..ef10ad3 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); @@ -156,22 +159,34 @@ sub AddReserve { $constraint, $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, $constraint, $bibitems ); + my $dbh = C4::Context->dbh; my $const = lc substr( $constraint, 0, 1 ); + $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 @@ -194,22 +209,22 @@ sub AddReserve { "Reserve Charge - $title", $fee ); } - #if ($const eq 'a'){ - my $query = qq{ - INSERT INTO reserves - (borrowernumber,biblionumber,reservedate,branchcode,constrainttype, - priority,reservenotes,itemnumber,found,waitingdate,expirationdate) - VALUES - (?,?,?,?,?, - ?,?,?,?,?,?) - }; - my $sth = $dbh->prepare($query); - $sth->execute( - $borrowernumber, $biblionumber, $resdate, $branch, - $const, $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, + constrainttype => $const, + priority => $priority, + reservenotes => $notes, + itemnumber => $checkitem, + found => $found, + waitingdate => $waitingdate, + expirationdate => $expdate + } + )->store(); + my $reserve_id = $hold->id(); # Send e-mail to librarian if syspref is active if(C4::Context->preference("emailLibrarianWhenHoldIsPlaced")){ @@ -240,19 +255,6 @@ sub AddReserve { } } - #} - ($const eq "o" || $const eq "e") or return; # FIXME: why not have a useful return value? - $query = qq{ - INSERT INTO reserveconstraints - (borrowernumber,biblionumber,reservedate,biblioitemnumber) - VALUES - (?,?,?,?) - }; - $sth = $dbh->prepare($query); # keep prepare outside the loop! - foreach (@$bibitems) { - $sth->execute($borrowernumber, $biblionumber, $resdate, $_); - } - return $reserve_id; } diff --git a/Koha/Hold.pm b/Koha/Hold.pm new file mode 100644 index 0000000..d8c0ea4 --- /dev/null +++ b/Koha/Hold.pm @@ -0,0 +1,52 @@ +package Koha::Hold; + +# Copyright ByWater Solutions 2015 +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use base qw(Koha::Object); + +=head1 NAME + +Koha::Hold - Koha Hold Object class + +=head1 API + +=head2 Class Methods + +=cut + +=head3 type + +=cut + +sub type { + return 'Reserve'; +} + +=head1 AUTHOR + +Kyle M Hall + +=cut + +1; diff --git a/Koha/Holds.pm b/Koha/Holds.pm new file mode 100644 index 0000000..b4b45c5 --- /dev/null +++ b/Koha/Holds.pm @@ -0,0 +1,58 @@ +package Koha::Holds; + +# Copyright ByWater Solutions 2015 +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use Koha::Hold; + +use base qw(Koha::Objects); + +=head1 NAME + +Koha::Hold - Koha Hold Object class + +=head1 API + +=head2 Class Methods + +=cut + +=head3 type + +=cut + +sub type { + return 'Reserve'; +} + +sub object_class { + return 'Koha::Hold'; +} + +=head1 AUTHOR + +Kyle M Hall + +=cut + +1; 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 68abc0a..7f340ad 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, 'a', [$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 fa0cf94..0f86eba 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', @@ -389,11 +396,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, $constraint, $bibitems, $p, $resdate, $expdate, $notes, $title, $checkitem, $found); $p = C4::Reserves::CalculatePriority($bibnum2); @@ -412,7 +420,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, $constraint, $bibitems, $p, $resdate, $expdate, $notes, $title, $checkitem, $found); $p = C4::Reserves::CalculatePriority($bibnum); @@ -421,7 +429,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, $constraint, $bibitems, $p, output_pref($resdate), $expdate, $notes, $title, $checkitem, $found); $p = C4::Reserves::CalculatePriority($bibnum); -- 1.7.2.5