From 76feff2f7ef813dfb5f1a336bf99f9e671a8b016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20=C5=A0iman?= Date: Tue, 10 Oct 2017 19:40:18 +0200 Subject: [PATCH] Bug 17509: (follow-up) Logic moved from C4 to Koha namespace This patch simplifies logic by using Koha objects. Automated test has been moved from t/db_dependent/Reserves to t/db_dependent/Holds. Test plan: 1) apply patch 2) run database update 3) enable sending an email to the patrons to return an item whenever a hold request is placed on it (NotifyToReturnItemFromLibrary) 4) place a hold 5) patrons having checked-out the item on hold are notified by email (see HOLDPLACED_CONTACT letter) Signed-off-by: Josef Moravec --- C4/Reserves.pm | 76 ++--------------------- Koha/Hold.pm | 36 +++++++++++ t/db_dependent/{Reserves => Holds}/Notification.t | 12 ++-- 3 files changed, 47 insertions(+), 77 deletions(-) rename t/db_dependent/{Reserves => Holds}/Notification.t (92%) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index e70b80cbd9..715fd93c35 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -140,7 +140,6 @@ BEGIN { IsItemOnHoldAndFound GetMaxPatronHoldsForRecord - GetBorrowersToSatisfyHold ); @EXPORT_OK = qw( MergeHolds ); } @@ -265,20 +264,20 @@ sub AddReserve { # Send email to borrowers asking to return item whenever a hold is placed on it if (C4::Context->preference("NotifyToReturnItemWhenHoldIsPlaced")) { - my @borrowers = GetBorrowersToSatisfyHold($hold); + my @borrowers = $hold->borrowers_to_satisfy(); foreach my $borrower (@borrowers) { - if ( !$borrower->{email} ) { + if ( !$borrower->email ) { next; } - my $library = Koha::Libraries->find($borrower->{branchcode})->unblessed; + my $library = Koha::Libraries->find($borrower->branchcode)->unblessed; if ( my $letter = C4::Letters::GetPreparedLetter ( module => 'reserves', letter_code => 'HOLDPLACED_CONTACT', branchcode => $branch, tables => { 'branches' => $library, - 'borrowers' => $borrower, + 'borrowers' => $borrower->unblessed, 'biblio' => $biblionumber, 'biblioitems' => $biblionumber, 'items' => $checkitem, @@ -289,10 +288,10 @@ sub AddReserve { C4::Letters::EnqueueLetter( { letter => $letter, - borrowernumber => $borrower->{borrowernumber}, + borrowernumber => $borrower->borrowernumber, message_transport_type => 'email', from_address => $admin_email_address, - to_address => $borrower->{email}, + to_address => $borrower->email, } ); } @@ -2202,69 +2201,6 @@ sub GetHoldRule { return $sth->fetchrow_hashref(); } -=head2 GetBorrowersToSatisfyHold - -my @borrowers = GetBorrowersToSatisfyHold( $hold ); - -Returns borrowers who can return item to satisfy a given hold. - -=cut - -sub GetBorrowersToSatisfyHold { - my ( $hold ) = @_; - - my $query = ""; - - if ($hold->item()) { - $query = q{ - SELECT borrowers.borrowernumber - FROM reserves - JOIN items ON reserves.itemnumber = items.itemnumber - JOIN issues ON issues.itemnumber = items.itemnumber - JOIN borrowers ON issues.borrowernumber = borrowers.borrowernumber - WHERE reserves.reserve_id = ? - }; - } - elsif ($hold->biblio()) { - $query = q{ - SELECT borrowers.borrowernumber - FROM reserves - JOIN biblio ON reserves.biblionumber = biblio.biblionumber - JOIN items ON items.biblionumber = biblio.biblionumber - JOIN issues ON issues.itemnumber = items.itemnumber - JOIN borrowers ON issues.borrowernumber = borrowers.borrowernumber - WHERE reserves.reserve_id = ? - }; - } - - my $library = C4::Context->preference("NotifyToReturnItemFromLibrary"); - my $dbh = C4::Context->dbh; - my $sth; - - if ($library eq 'RequestorLibrary') { - $query .= " AND borrowers.branchcode = ?"; - $sth = $dbh->prepare( $query ); - $sth->execute( $hold->id(), $hold->borrower()->branchcode ); - } - elsif ($library eq 'ItemHomeLibrary') { - $query .= " AND items.homebranch = ?"; - $sth = $dbh->prepare( $query ); - $sth->execute( $hold->id(), $hold->borrower()->branchcode ); - } - else { - $sth = $dbh->prepare( $query ); - $sth->execute( $hold->id() ); - } - - my @results = (); - while ( my $data = $sth->fetchrow_hashref ) { - my $borrower = C4::Members::GetMember(borrowernumber => $data->{borrowernumber}); - push( @results, $borrower ); - } - - return @results; -} - =head1 AUTHOR Koha Development Team diff --git a/Koha/Hold.pm b/Koha/Hold.pm index a1ef5aafa5..70da54a126 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -325,6 +325,42 @@ sub borrower { return $self->{_borrower}; } +=head3 borrowers_to_satisfy + +Returns Koha::Patron objects able to return item to satisfy this Hold + +=cut + +sub borrowers_to_satisfy { + my ($self) = @_; + + my @items = (); + if ( my $item = $self->item() ) { + push ( @items, $item ); + } + elsif ( my $biblio = $self->biblio() ) { + push ( @items, $biblio->items() ); + } + + my $library = C4::Context->preference("NotifyToReturnItemFromLibrary"); + my @patrons = (); + foreach my $item ( @items ) { + + next unless $item->checkout(); + + my $patron = $item->checkout()->patron(); + if ( ( ($library eq 'RequestorLibrary') && ($patron->branchcode eq $self->borrower()->branchcode) ) + || ( ($library eq 'ItemHomeLibrary') && ($item->homebranch eq $self->borrower()->branchcode) ) + || ( $library eq 'AnyLibrary' ) ) { + + push ( @patrons, $patron ); + } + } + + return @patrons; +} + + =head3 is_suspended my $bool = $hold->is_suspended(); diff --git a/t/db_dependent/Reserves/Notification.t b/t/db_dependent/Holds/Notification.t similarity index 92% rename from t/db_dependent/Reserves/Notification.t rename to t/db_dependent/Holds/Notification.t index 35ac5dc5d7..5de396895b 100755 --- a/t/db_dependent/Reserves/Notification.t +++ b/t/db_dependent/Holds/Notification.t @@ -23,10 +23,8 @@ use Test::More tests => 5; use t::lib::TestBuilder; use t::lib::Mocks; -use C4::Reserves qw( GetBorrowersToSatisfyHold ); use Koha::Database; use Koha::Holds; -use Data::Dumper; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; @@ -149,18 +147,18 @@ my $hold = Koha::Hold->new( my @borrowers; t::lib::Mocks::mock_preference('NotifyToReturnItemFromLibrary', 'AnyLibrary'); # Assuming items from any library -@borrowers = GetBorrowersToSatisfyHold($hold); +@borrowers = $hold->borrowers_to_satisfy(); is(scalar(grep {defined $_} @borrowers), 2, "2 borrowers with requested item found in any library"); t::lib::Mocks::mock_preference('NotifyToReturnItemFromLibrary', 'RequestorLibrary'); # Assuming items from requestor library -@borrowers = GetBorrowersToSatisfyHold($hold); +@borrowers = $hold->borrowers_to_satisfy(); is(scalar(grep {defined $_} @borrowers), 1, "1 item found in requestor library"); -is($borrowers[0]->{cardnumber}, "002", " ...and it is in 002's hands"); +is($borrowers[0]->cardnumber, "002", " ...and it is in 002's hands"); t::lib::Mocks::mock_preference('NotifyToReturnItemFromLibrary', 'ItemHomeLibrary'); # Assuming items from the same library as requested item -@borrowers = GetBorrowersToSatisfyHold($hold); +@borrowers = $hold->borrowers_to_satisfy(); is(scalar(grep {defined $_} @borrowers), 1, "1 item found in the same library as the requested item"); -is($borrowers[0]->{cardnumber}, "003", " ...and it is in 003's hands"); +is($borrowers[0]->cardnumber, "003", " ...and it is in 003's hands"); $schema->storage->txn_rollback; -- 2.11.0