Bugzilla – Attachment 23862 Details for
Bug 11336
Priority is not updated on deleting holds
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[PASSED QA] Bug 11336: Priority is not updated on deleting holds
PASSED-QA-Bug-11336-Priority-is-not-updated-on-del.patch (text/plain), 10.99 KB, created by
Katrin Fischer
on 2013-12-29 13:01:25 UTC
(
hide
)
Description:
[PASSED QA] Bug 11336: Priority is not updated on deleting holds
Filename:
MIME Type:
Creator:
Katrin Fischer
Created:
2013-12-29 13:01:25 UTC
Size:
10.99 KB
patch
obsolete
>From 5ccb0c1408a5ac9267b3ab6aa61a891f7ffccfca Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@biblibre.com> >Date: Fri, 13 Dec 2013 17:42:55 +0100 >Subject: [PATCH] [PASSED QA] 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 (click on the red cross) > >Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> >Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> >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 >--- > 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.8.3.2
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 11336
:
23286
|
23384
|
23522
|
23531
|
23532
|
23567
|
23629
|
23706
|
23707
|
23721
|
23787
|
23831
| 23862 |
23863