From 290a1ff0bf8950bf907b14536cf2420a4d513159 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Dec 2013 17:42:55 +0100 Subject: [PATCH] Bug 11336: Priority is not updated on deleting holds There are a lot of places where a hold deletion is possible. But I just found 1 place where it works! To reproduce: - select or create 2 users U1 and U2 - select or create an holdable item - place on hold for both U1 and U2. U1 has priority 1 and U2 has priority 2. - delete the hold for U1 - go on circ/circulation.pl?borrowernumber=XXXX for U2 (or in the DB directly) and verify the priority has not been set to 1 The issue is repeatable (at least) on these 2 pages: * circ/circulation.pl?borrowernumber=XXXX (tab 'Holds', select "yes" in the dropdown list and submit the form) * reserve/request.pl?biblionumber=XXXX (clic on the red cross) Signed-off-by: Christopher Brannon --- C4/Reserves.pm | 71 +++++++++++++++++++++++------------------------- t/db_dependent/Holds.t | 70 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 101 insertions(+), 40 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index d1bca25..f9ec5eb 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -252,6 +252,8 @@ sub AddReserve { $res = GetReserve( $reserve_id ); + Return the current reserve or the old reserve. + =cut sub GetReserve { @@ -261,8 +263,7 @@ sub GetReserve { my $query = "SELECT * FROM reserves WHERE reserve_id = ?"; my $sth = $dbh->prepare( $query ); $sth->execute( $reserve_id ); - my $res = $sth->fetchrow_hashref(); - return $res; + return $sth->fetchrow_hashref(); } =head2 GetReservesFromBiblionumber @@ -1008,6 +1009,8 @@ sub CancelReserve { my $dbh = C4::Context->dbh; + my $reserve = GetReserve( $reserve_id ); + my $query = " UPDATE reserves SET cancellationdate = now(), @@ -1034,7 +1037,10 @@ sub CancelReserve { $sth->execute( $reserve_id ); # now fix the priority on the others.... - _FixPriority( $reserve_id ); + _FixPriority({ + reserve_id => $reserve_id, + biblionumber => $reserve->{biblionumber}, + }); } =head2 ModReserve @@ -1092,28 +1098,7 @@ sub ModReserve { my $dbh = C4::Context->dbh; if ( $rank eq "del" ) { - my $query = " - UPDATE reserves - SET cancellationdate=now() - WHERE reserve_id = ? - "; - my $sth = $dbh->prepare($query); - $sth->execute( $reserve_id ); - $query = " - INSERT INTO old_reserves - SELECT * - FROM reserves - WHERE reserve_id = ? - "; - $sth = $dbh->prepare($query); - $sth->execute( $reserve_id ); - $query = " - DELETE FROM reserves - WHERE reserve_id = ? - "; - $sth = $dbh->prepare($query); - $sth->execute( $reserve_id ); - + CancelReserve({ reserve_id => $reserve_id }); } elsif ($rank =~ /^\d+/ and $rank > 0) { my $query = " @@ -1132,7 +1117,7 @@ sub ModReserve { } } - _FixPriority( $reserve_id, $rank ); + _FixPriority({ reserve_id => $reserve_id, rank =>$rank }); } } @@ -1199,7 +1184,7 @@ sub ModReserveFill { # now fix the priority on the others (if the priority wasn't # already sorted!).... unless ( $priority == 0 ) { - _FixPriority( $reserve_id ); + _FixPriority({ reserve_id => $reserve_id }); } } @@ -1338,7 +1323,7 @@ sub ModReserveMinusPriority { my $sth_upd = $dbh->prepare($query); $sth_upd->execute( $itemnumber, $reserve_id ); # second step update all others reservs - _FixPriority( $reserve_id, '0'); + _FixPriority({ reserve_id => $reserve_id, rank => '0' }); } =head2 GetReserveInfo @@ -1491,15 +1476,15 @@ sub AlterPriority { my $priority = $reserve->{'priority'}; $priority = $where eq 'up' ? $priority - 1 : $priority + 1; - _FixPriority( $reserve_id, $priority ) + _FixPriority({ reserve_id => $reserve_id, rank => $priority }) } elsif ( $where eq 'top' ) { - _FixPriority( $reserve_id, '1' ) + _FixPriority({ reserve_id => $reserve_id, rank => '1' }) } elsif ( $where eq 'bottom' ) { - _FixPriority( $reserve_id, '999999' ) + _FixPriority({ reserve_id => $reserve_id, rank => '999999' }); } } @@ -1520,7 +1505,7 @@ sub ToggleLowestPriority { my $sth = $dbh->prepare( "UPDATE reserves SET lowestPriority = NOT lowestPriority WHERE reserve_id = ?"); $sth->execute( $reserve_id ); - _FixPriority( $reserve_id, '999999' ); + _FixPriority({ reserve_id => $reserve_id, rank => '999999' }); } =head2 ToggleSuspend @@ -1613,7 +1598,7 @@ sub SuspendAll { =head2 _FixPriority - &_FixPriority( $reserve_id, $rank, $ignoreSetLowestRank); + &_FixPriority({ reserve_id => $reserve_id, rank => $rank, ignoreSetLowestRank => $ignoreSetLowestRank }); Only used internally (so don't export it) Changed how this functions works # @@ -1625,10 +1610,18 @@ in new priority rank =cut sub _FixPriority { - my ( $reserve_id, $rank, $ignoreSetLowestRank ) = @_; + my ( $params ) = @_; + my $reserve_id = $params->{reserve_id}; + my $rank = $params->{rank}; + my $ignoreSetLowestRank = $params->{ignoreSetLowestRank}; + my $biblionumber = $params->{biblionumber}; + my $dbh = C4::Context->dbh; - my $res = GetReserve( $reserve_id ); + unless ( $biblionumber ) { + my $res = GetReserve( $reserve_id ); + $biblionumber = $res->{biblionumber}; + } if ( $rank eq "del" ) { CancelReserve({ reserve_id => $reserve_id }); @@ -1656,7 +1649,7 @@ sub _FixPriority { ORDER BY priority ASC "; my $sth = $dbh->prepare($query); - $sth->execute( $res->{'biblionumber'} ); + $sth->execute( $biblionumber ); while ( my $line = $sth->fetchrow_hashref ) { push( @priority, $line ); } @@ -1698,7 +1691,11 @@ sub _FixPriority { unless ( $ignoreSetLowestRank ) { while ( my $res = $sth->fetchrow_hashref() ) { - _FixPriority( $res->{'reserve_id'}, '999999', 1 ); + _FixPriority({ + reserve_id => $res->{'reserve_id'}, + rank => '999999', + ignoreSetLowestRank => 1 + }); } } } diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 74cecfa..2c28c07 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -1,13 +1,12 @@ #!/usr/bin/perl -use strict; -use warnings; +use Modern::Perl; use t::lib::Mocks; use C4::Context; use C4::Branch; -use Test::More tests => 22; +use Test::More tests => 25; use MARC::Record; use C4::Biblio; use C4::Items; @@ -215,6 +214,71 @@ ok( '... unless canreservefromotherbranches is ON (bug 2394)' ); +# Regression test for bug 11336 +($bibnum, $title, $bibitemnum) = create_helper_biblio(); +my ( $hold1, $hold2 ); +($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => 'CPL', holdingbranch => 'CPL' } , $bibnum); +AddReserve( + $branch, + $borrowernumbers[0], + $bibnum, + 'a', + '', + 1, +); + +my $reserveid1 = C4::Reserves::GetReserveId( + { + biblionumber => $bibnum, + borrowernumber => $borrowernumbers[0] + } +); + +($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => 'CPL', holdingbranch => 'CPL' } , $bibnum); +AddReserve( + $branch, + $borrowernumbers[1], + $bibnum, + 'a', + '', + 2, +); +my $reserveid2 = C4::Reserves::GetReserveId( + { + biblionumber => $bibnum, + borrowernumber => $borrowernumbers[1] + } +); + +CancelReserve({ reserve_id => $reserveid1 }); + +$reserve2 = GetReserve( $reserveid2 ); +is( $reserve2->{priority}, 1, "After cancelreserve, the 2nd reserve becomes the first on the waiting list" ); + +($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => 'CPL', holdingbranch => 'CPL' } , $bibnum); +AddReserve( + $branch, + $borrowernumbers[0], + $bibnum, + 'a', + '', + 2, +); +my $reserveid3 = C4::Reserves::GetReserveId( + { + biblionumber => $bibnum, + borrowernumber => $borrowernumbers[0] + } +); + +my $reserve3 = GetReserve( $reserveid3 ); +is( $reserve3->{priority}, 2, "New reserve for patron 0, the reserve has a priority = 2" ); + +ModReserve({ reserve_id => $reserveid2, rank => 'del' }); +$reserve3 = GetReserve( $reserveid3 ); +is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" ); + + # Helper method to set up a Biblio. sub create_helper_biblio { my $bib = MARC::Record->new(); -- 1.7.2.5