From 21e1eb644b62832af800d99a714eef91bb607faf Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 7 Dec 2016 14:42:48 +0100 Subject: [PATCH] Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=utf-8 This patch replace the different calls to GetReservesFromBorrowernumber with a calls to Koha::Patron->get_holds. In some places we need to get a restricted set of holds, that's why we process a search on this holds returned by ->get_holds (on the found status for instance). The changes are quite trivial and reading the diff should be enough to catch bugs. Test plan: I would suggest to test this patch with patches from bug 17736 and bug 17737, to place different kind of holds (biblio and item level, future and past). Then do a whole workflow to detect bug, view a record, delete record, order, place a hold on an item which has been ordered, etc. The hold's informations should always be the same without or without these patches. Tested both patches together, works as expected. Signed-off-by: Marc Véron Signed-off-by: Kyle M Hall Signed-off-by: Marcel de Rooy --- C4/ILSDI/Services.pm | 30 ++++++++++++++++++------------ C4/Members.pm | 8 +++++--- C4/SIP/ILS/Patron.pm | 20 +++++++++++++------- Koha/Patron/Discharge.pm | 9 +++++---- circ/returns.pl | 12 ++++-------- members/discharge.pl | 6 ++++-- opac/opac-reserve.pl | 14 ++++++++------ t/db_dependent/Holds.t | 17 ++++++++++------- 8 files changed, 67 insertions(+), 49 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 287ac02..c1632d5 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -25,7 +25,7 @@ use C4::Items; use C4::Circulation; use C4::Accounts; use C4::Biblio; -use C4::Reserves qw(AddReserve GetReservesFromBorrowernumber CanBookBeReserved CanItemBeReserved IsAvailableForItemLevelRequest); +use C4::Reserves qw(AddReserve CanBookBeReserved CanItemBeReserved IsAvailableForItemLevelRequest); use C4::Context; use C4::AuthoritiesMarc; use XML::Simple; @@ -37,6 +37,7 @@ use C4::Members::Attributes qw(GetBorrowerAttributes); use Koha::Biblios; use Koha::Libraries; +use Koha::Patrons; =head1 NAME @@ -375,6 +376,7 @@ sub GetPatronInfo { my $borrowernumber = $cgi->param('patron_id'); my $borrower = GetMember( borrowernumber => $borrowernumber ); return { code => 'PatronNotFound' } unless $$borrower{borrowernumber}; + my $patron = Koha::Patrons->find( $borrowernumber ); # Cleaning the borrower hashref my $flags = C4::Members::patronflags( $borrower ); @@ -414,23 +416,25 @@ sub GetPatronInfo { if ( $cgi->param('show_holds') && $cgi->param('show_holds') eq "1" ) { # Get borrower's reserves - my @reserves = GetReservesFromBorrowernumber( $borrowernumber, undef ); - foreach my $reserve (@reserves) { + my $holds = $patron->holds; + while ( my $hold = $holds->next ) { + my $unblessed_hold = $hold->unblessed; # Get additional informations - my $item = GetBiblioFromItemNumber( $reserve->{'itemnumber'}, undef ); - my $library = Koha::Libraries->find( $reserve->{branchcode} ); + my $item = GetBiblioFromItemNumber( $hold->itemnumber, undef ); + my $library = Koha::Libraries->find( $hold->branchcode ); # Should $hold->get_library my $branchname = $library ? $library->branchname : ''; # Remove unwanted fields delete $item->{'more_subfields_xml'}; # Add additional fields - $reserve->{'item'} = $item; - $reserve->{'branchname'} = $branchname; - $reserve->{'title'} = GetBiblio( $reserve->{'biblionumber'} )->{'title'}; + $unblessed_hold->{item} = $item; + $unblessed_hold->{branchname} = $branchname; + $unblessed_hold->{title} = GetBiblio( $hold->biblionumber )->{'title'}; # Should be $hold->get_biblio + + push @{ $borrower->{holds}{hold} }, $unblessed_hold; } - $borrower->{'holds'}->{'hold'} = \@reserves; } # Issues management @@ -500,6 +504,8 @@ sub GetServices { my $borrower = GetMember( borrowernumber => $borrowernumber ); return { code => 'PatronNotFound' } unless $$borrower{borrowernumber}; + my $patron = Koha::Patrons->find( $borrowernumber ); + # Get the item, or return an error code if not found my $itemnumber = $cgi->param('item_id'); my $item = GetItem( $itemnumber ); @@ -519,10 +525,10 @@ sub GetServices { } # Reserve cancellation management - my @reserves = GetReservesFromBorrowernumber( $borrowernumber, undef ); + my $holds = $patron->holds; my @reserveditems; - foreach my $reserve (@reserves) { - push @reserveditems, $reserve->{'itemnumber'}; + while ( my $hold = $holds->next ) { # FIXME This could be improved + push @reserveditems, $hold->itemnumber; } if ( grep { $itemnumber eq $_ } @reserveditems ) { push @availablefor, 'hold cancellation'; diff --git a/C4/Members.pm b/C4/Members.pm index 6527c79..8e99583 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -267,12 +267,14 @@ sub patronflags { } $flags{'ODUES'} = \%flaginfo; } - my @itemswaiting = C4::Reserves::GetReservesFromBorrowernumber( $patroninformation->{'borrowernumber'},'W' ); - my $nowaiting = scalar @itemswaiting; + + my $patron = Koha::Patrons->find( $patroninformation->{borrowernumber} ); + my $waiting_holds = $patron->holds->search({ found => 'W' }); + my $nowaiting = $waiting_holds->count; if ( $nowaiting > 0 ) { my %flaginfo; $flaginfo{'message'} = "Reserved items available"; - $flaginfo{'itemlist'} = \@itemswaiting; + $flaginfo{'itemlist'} = $waiting_holds->unblessed; $flags{'WAITING'} = \%flaginfo; } return ( \%flags ); diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm index 0012776..64ef4f6 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -24,6 +24,7 @@ use C4::Items qw( GetBarcodeFromItemnumber GetItemnumbersForBiblio); use C4::Auth qw(checkpw); use Koha::Libraries; +use Koha::Patrons; our $kp; # koha patron @@ -431,20 +432,25 @@ sub _get_address { sub _get_outstanding_holds { my $borrowernumber = shift; - my @hold_array = grep { !defined $_->{found} || $_->{found} ne 'W'} GetReservesFromBorrowernumber($borrowernumber); - foreach my $h (@hold_array) { + + my $patron = Koha::Patrons->find( $borrowernumber ); + my $holds = $patron->holds->search( { -or => [ { found => undef }, { found => { '!=' => 'W' } } ] } ); + my @holds; + while ( my $hold = $holds->next ) { my $item; - if ($h->{itemnumber}) { - $item = $h->{itemnumber}; + if ($hold->itemnumber) { + $item = $hold->itemnumber; } else { # We need to return a barcode for the biblio so the client # can request the biblio info - $item = ( GetItemnumbersForBiblio($h->{biblionumber}) )->[0]; + $item = ( GetItemnumbersForBiblio($hold->biblionumber) )->[0]; } - $h->{barcode} = GetBarcodeFromItemnumber($item); + my $unblessed_hold = $hold->unblessed; + $unblessed_hold->{barcode} = GetBarcodeFromItemnumber($item); + push @holds, $unblessed_hold; } - return \@hold_array; + return \@holds; } 1; diff --git a/Koha/Patron/Discharge.pm b/Koha/Patron/Discharge.pm index 289fdb0..580d4d1 100644 --- a/Koha/Patron/Discharge.pm +++ b/Koha/Patron/Discharge.pm @@ -7,7 +7,7 @@ use Carp; use C4::Templates qw ( gettemplate ); use C4::Members qw( GetPendingIssues ); -use C4::Reserves qw( GetReservesFromBorrowernumber CancelReserve ); +use C4::Reserves qw( CancelReserve ); use Koha::Database; use Koha::DateUtils qw( dt_from_string output_pref ); @@ -79,9 +79,10 @@ sub discharge { return unless $borrowernumber and can_be_discharged( { borrowernumber => $borrowernumber } ); # Cancel reserves - my @reserves = GetReservesFromBorrowernumber($borrowernumber); - for my $reserve (@reserves) { - CancelReserve( { reserve_id => $reserve->{reserve_id} } ); + my $patron = Koha::Patrons->find( $borrowernumber ); + my $holds = $patron->holds; + while ( my $hold = $holds->next ) { + CancelReserve( { reserve_id => $hold->reserve_id } ); } # Debar the member diff --git a/circ/returns.pl b/circ/returns.pl index d1b95e8..a6041d1 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -50,8 +50,9 @@ use C4::RotatingCollections; use Koha::AuthorisedValues; use Koha::DateUtils; use Koha::Calendar; -use Koha::Checkouts; use Koha::BiblioFrameworks; +use Koha::Checkouts; +use Koha::Patrons; my $query = new CGI; @@ -339,13 +340,8 @@ if ($barcode) { if (C4::Context->preference("WaitingNotifyAtCheckin") ) { #Check for waiting holds - my @reserves = GetReservesFromBorrowernumber($borrower->{'borrowernumber'}); - my $waiting_holds = 0; - foreach my $num_res (@reserves) { - if ( $num_res->{'found'} eq 'W' && $num_res->{'branchcode'} eq $userenv_branch) { - $waiting_holds++; - } - } + my $patron = Koha::Patrons->find( $borrower->{borrowernumber} ); + my $waiting_holds = $patron->holds->search({ found => 'W', branchcode => $userenv_branch })->count; if ($waiting_holds > 0) { $template->param( waiting_holds => $waiting_holds, diff --git a/members/discharge.pl b/members/discharge.pl index ededd23..23b4378 100755 --- a/members/discharge.pl +++ b/members/discharge.pl @@ -38,6 +38,7 @@ use C4::Reserves; use C4::Letters; use Koha::Patron::Discharge; use Koha::Patron::Images; +use Koha::Patrons; use Koha::DateUtils; @@ -70,8 +71,9 @@ if ( $input->param('borrowernumber') ) { }); # Getting reserves - my @reserves = GetReservesFromBorrowernumber($borrowernumber); - my $has_reserves = scalar(@reserves); + my $patron = Koha::Patrons->find( $borrowernumber ); + my $holds = $patron->holds; + my $has_reserves = $holds->count; # Generating discharge if needed if ( $input->param('discharge') and $can_be_discharged ) { diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 7de7b97..2cf8e59 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -199,7 +199,8 @@ foreach my $biblioNumber (@biblionumbers) { if ( $query->param('place_reserve') ) { my $reserve_cnt = 0; if ($maxreserves) { - $reserve_cnt = GetReservesFromBorrowernumber( $borrowernumber ); + my $patron = Koha::Patrons->find( $borrowernumber ); + $reserve_cnt = $patron->holds->count; } # List is composed of alternating biblio/item/branch @@ -350,7 +351,8 @@ if ( $borr->{lost} && ($borr->{lost} == 1) ) { ); } -if ( Koha::Patrons->find( $borrowernumber )->is_debarred ) { +my $patron = Koha::Patrons->find( $borrowernumber ); +if ( $patron->is_debarred ) { $noreserves = 1; $template->param( message => 1, @@ -360,13 +362,13 @@ if ( Koha::Patrons->find( $borrowernumber )->is_debarred ) { ); } -my @reserves = GetReservesFromBorrowernumber( $borrowernumber ); -my $reserves_count = scalar(@reserves); -$template->param( RESERVES => \@reserves ); +my $holds = $patron->holds; +my $reserves_count = $holds->count; +$template->param( RESERVES => $holds->unblessed ); if ( $maxreserves && ( $reserves_count >= $maxreserves ) ) { $template->param( message => 1 ); $noreserves = 1; - $template->param( too_many_reserves => scalar(@reserves)); + $template->param( too_many_reserves => $holds->count ); } unless ( $noreserves ) { diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index b00a6da..ad87b26 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -17,6 +17,7 @@ use Koha::Database; use Koha::DateUtils qw( dt_from_string output_pref ); use Koha::Biblios; use Koha::Holds; +use Koha::Patrons; BEGIN { use FindBin; @@ -121,8 +122,9 @@ my $hold_found = $hold->found(); $hold->set({ found => 'W'})->store(); is( Koha::Holds->waiting()->count(), 1, "Koha::Holds->waiting returns waiting holds" ); -my ( $reserve ) = GetReservesFromBorrowernumber($borrowernumbers[0]); -ok( $reserve->{'borrowernumber'} eq $borrowernumbers[0], "Test GetReservesFromBorrowernumber()"); +my $patron = Koha::Patrons->find( $borrowernumbers[0] ); +$holds = $patron->holds; +is( $holds->next->borrowernumber, $borrowernumbers[0], "Test Koha::Patron->holds"); ok( GetReserveCount( $borrowernumbers[0] ), "Test GetReserveCount()" ); @@ -146,7 +148,7 @@ ModReserve({ suspend_until => output_pref( { dt => dt_from_string( "2013-01-01", "iso" ), dateonly => 1 } ), }); -$reserve = GetReserve( $reserve_id ); +my $reserve = GetReserve( $reserve_id ); ok( $reserve->{'priority'} eq '4', "Test GetReserve(), priority changed correctly" ); ok( $reserve->{'suspend'}, "Test GetReserve(), suspend hold" ); is( $reserve->{'suspend_until'}, '2013-01-01 00:00:00', "Test GetReserve(), suspend until date" ); @@ -196,17 +198,18 @@ AddReserve( my $checkitem, my $found, ); -( $reserve ) = GetReservesFromBorrowernumber($borrowernumber); +$patron = Koha::Patrons->find( $borrowernumber ); +$holds = $patron->holds; my $reserveid = C4::Reserves::GetReserveId( { biblionumber => $biblionumber, borrowernumber => $borrowernumber } ); -is( $reserveid, $reserve->{reserve_id}, "Test GetReserveId" ); +is( $reserveid, $holds->next->reserve_id, "Test GetReserveId" ); ModReserveMinusPriority( $itemnumber, $reserve->{'reserve_id'} ); -( $reserve ) = GetReservesFromBorrowernumber($borrowernumber); -ok( $reserve->{'itemnumber'} eq $itemnumber, "Test ModReserveMinusPriority()" ); +$holds = $patron->holds; +is( $holds->next->itemnumber, $itemnumber, "Test ModReserveMinusPriority()" ); my $reserve2 = GetReserveInfo( $reserve->{'reserve_id'} ); -- 2.1.4