From 78037c5573f70939077dd00f296c735b198e6124 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Dec 2013 17:42:55 +0100 Subject: [PATCH] Bug 11336: update hold queue priorities correctly when deleting holds In various places, deleting a hold request did not trigger recalculating the priority of the other holds on the bib: To reproduce the bug: - 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 (click on the red cross) Signed-off-by: Christopher Brannon Signed-off-by: Katrin Fischer Reran my tests: Preparations: - Create holds for different patrons on a record: * 1st - title level hold * 2nd - item level hold * 3rd - title level hold * 4th - title level hold - AllowOnShelfHolds = On/Allow (items were not checked out) Tests: Deleted holds from various pages, confirming bugs first, then testing with applied patches. Reloading database after each test. 1) Cancel holds from OPAC patron account /cgi-bin/koha/opac-user.pl#opac-user-holds - Cancel 4th - ok, before and after applying the patch - Cancel 2nd - ok, after applying the patch 2) Cancel hold from holds tab on staff detail page /cgi-bin/koha/reserve/request.pl?biblionumber=7 a) Setting priority to 'del', submitting with 'Update holds' - Cancel first (1st) - ok, before and after - Cancel hold in the middle (was 3rd) - ok, before and after - Cancel last (was 4th) -ok, before and after b) Using red X - Repeating tests from a) - before the patch is applied holds get totally 'out of order' - after applying the patch, it works correctly Additional tests done on this page: - Change priority using up, down, to top, to bottom icons - Change priority with 'toggle to lowest' 3) Cancel hold from the patron's account a) Check out tab - Delete? Yes, 'Cancel marked holds' /cgi-bin/koha/circ/circulation.pl?borrowernumber=X - Cancel first (1st) - ok, after applying the patch - Cancel hold in the middle (was 3rd) - ok, after applying the patch - Cancel last (was 4th) - ok, after applying the patch b) Details tab - Delete? yes, 'Cancel marked holds' /cgi-bin/koha/members/moremember.pl?borrowernumber=X - Cancel first (1st) - ok, after applying the patch - Cancel hold in the middle (was 3rd) - ok, after applying the patch - Cancel last (was 4th) - ok, after applying the patch Without the patch, holds priorities get out of order. Additional tests done: - Check in one item to trigger first hold - Check in one item to trigger second hold - Check out first item Priorities are kept while the item is waiting, when it's checked out, priorities of remaining holds get reset correctly. Conclusion: Big improvement, no regressions found. Passes all tests in t, xt and QA script. Also: t/db_dependent/Holds.t t/db_dependent/HoldsQueue.t t/db_dependent/Reserves.t Signed-off-by: Galen Charlton --- 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 8d83e876f6..93d9cae168 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 }); } } @@ -1343,7 +1328,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 @@ -1496,15 +1481,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' }); } } @@ -1525,7 +1510,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 @@ -1618,7 +1603,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 # @@ -1630,10 +1615,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 }); @@ -1661,7 +1654,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 ); } @@ -1703,7 +1696,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 74cecfac35..2c28c07456 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(); -- 2.39.5