From b066b5ac633c002158a28a2304f5903e9d0e3513 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Radek=20=C5=A0iman?= <rbit@rbit.cz>
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 <josef.moravec@gmail.com>
---
 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 101face..bfdfffe 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -139,7 +139,6 @@ BEGIN {
         IsItemOnHoldAndFound
 
         GetMaxPatronHoldsForRecord
-        GetBorrowersToSatisfyHold
     );
     @EXPORT_OK = qw( MergeHolds );
 }
@@ -264,20 +263,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,
@@ -288,10 +287,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,
                     }
             );
             }
@@ -2176,69 +2175,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 <http://koha-community.org/>
diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index 23d5d44..240b204 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -311,6 +311,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 35ac5dc..5de3968 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.1.4