Bugzilla – Attachment 40823 Details for
Bug 5144
Duplicate holds allowed if patron clicks back button after placing hold
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug-5144---Duplicate-holds-allowed-if-patron-click.patch (text/plain), 13.24 KB, created by
Kyle M Hall (khall)
on 2015-07-07 15:38:12 UTC
(
hide
)
Description:
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2015-07-07 15:38:12 UTC
Size:
13.24 KB
patch
obsolete
>From 66b5eed00fa430c934e56e4c4820c31b2ef68617 Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >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 <kyle@bywatersolutions.com> >+ >+=cut >+ >+1; >diff --git a/Koha/Holds.pm b/Koha/Holds.pm >new file mode 100644 >index 0000000..8b3d623 >--- /dev/null >+++ b/Koha/Holds.pm >@@ -0,0 +1,58 @@ >+package Koha::Holds; >+ >+# Copyright ByWater Solutions 2014 >+# >+# 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 <kyle@bywatersolutions.com> >+ >+=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 ) %]<div class="alert alert-info"><h3>Thank you!</h3><p>Your corrections have been submitted to the library, and a staff member will update your record as soon as possible.</p></div>[% END %] > >+ [% IF failed_holds %] >+ <div class="alert alert-info"> >+ <h3>Notice:</h3> >+ <p>One or more holds were not placed due to existing holds.</p> >+ </div> >+ [% END %] >+ > [% IF ( BORROWER_INF.warndeparture ) %] > <div class="alert" id="warndeparture"> > <strong>Please note:</strong><span> Your card will expire on <span id="warndeparture_date">[% BORROWER_INF.warndeparture | $KohaDates %]</span>. Please contact the library for more information.</span> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 5144
:
28657
|
28658
|
28659
|
34983
|
34984
|
34985
|
34986
|
34987
|
34988
|
35090
|
35091
|
35092
|
35093
|
35451
|
40823
|
40824
|
42530
|
42792
|
42848
|
43118
|
43119
|
43120
|
43121
|
43122
|
43125
|
43126
|
43164
|
43165
|
43166
|
43167