Bug 17766 - Patron notification does not work with multi item holds

This patch fixes notification when same biblio has multiple reserves with same borrower,
introduced in Bug 14695. C4::Reserves::ModReserveAffect uses internal method
_koha_notify_reserve but sends itemnumber and biblionumber instead of record_id.

To test:
Prerequisites:
- One biblio with two items attached
- A patron with hold_filled notification activated
- A letter for HOLD with <<reserves.reserve_id>> in it
1) Place two reservations on same biblio
2) checkin item x on pickup branch, observe patron message generated
3) checkin item y on pickup branch, observe patron message generated
4) note that reserve_id is the same on both messages, which is wrong
5) apply this patch and repeat 1-3
6) now observe notifications have correct (different) reserve_id

Signed-off-by: Hugo Agud <hagud@orex.es>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 9ef33ad..3f124a6 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -1282,12 +1282,12 @@ sub ModReserveStatus {
 
 =head2 ModReserveAffect
 
-  &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend);
+  &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id);
 
-This function affect an item and a status for a given reserve
-The itemnumber parameter is used to find the biblionumber.
-with the biblionumber & the borrowernumber, we can affect the itemnumber
-to the correct reserve.
+This function affect an item and a status for a given reserve, either fetched directly
+by record_id, or by borrowernumber and itemnumber or biblionumber. If only biblionumber
+is given, only first reserve returned is affected, which is ok for anything but
+multi-item holds.
 
 if $transferToDo is not set, then the status is set to "Waiting" as well.
 otherwise, a transfer is on the way, and the end of the transfer will
@@ -1346,7 +1346,7 @@ sub ModReserveAffect {
     }
     $hold->store();
 
-    _koha_notify_reserve( $itemnumber, $borrowernumber, $biblionumber )
+    _koha_notify_reserve( $hold->reserve_id )
       if ( !$transferToDo && !$already_on_shelf );
 
     _FixPriority( { biblionumber => $biblionumber } );
@@ -1952,7 +1952,7 @@ sub _Findgroupreserve {
 
 =head2 _koha_notify_reserve
 
-  _koha_notify_reserve( $itemnumber, $borrowernumber, $biblionumber );
+  _koha_notify_reserve( $hold->reserve_id );
 
 Sends a notification to the patron that their hold has been filled (through
 ModReserveAffect, _not_ ModReserveFill)
@@ -1979,9 +1979,10 @@ The following tables are availalbe witin the notice:
 =cut
 
 sub _koha_notify_reserve {
-    my ($itemnumber, $borrowernumber, $biblionumber) = @_;
+    my $reserve_id = shift;
+    my $hold = Koha::Holds->find($reserve_id);
+    my $borrowernumber = $hold->borrowernumber;
 
-    my $dbh = C4::Context->dbh;
     my $borrower = C4::Members::GetMember(borrowernumber => $borrowernumber);
 
     # Try to get the borrower's email address
@@ -1992,28 +1993,20 @@ sub _koha_notify_reserve {
             message_name => 'Hold_Filled'
     } );
 
-    my $sth = $dbh->prepare("
-        SELECT *
-        FROM   reserves
-        WHERE  borrowernumber = ?
-            AND biblionumber = ?
-    ");
-    $sth->execute( $borrowernumber, $biblionumber );
-    my $reserve = $sth->fetchrow_hashref;
-    my $library = Koha::Libraries->find( $reserve->{branchcode} )->unblessed;
+    my $library = Koha::Libraries->find( $hold->branchcode )->unblessed;
 
     my $admin_email_address = $library->{branchemail} || C4::Context->preference('KohaAdminEmailAddress');
 
     my %letter_params = (
         module => 'reserves',
-        branchcode => $reserve->{branchcode},
+        branchcode => $hold->branchcode,
         tables => {
             'branches'       => $library,
             'borrowers'      => $borrower,
-            'biblio'         => $biblionumber,
-            'biblioitems'    => $biblionumber,
-            'reserves'       => $reserve,
-            'items'          => $reserve->{'itemnumber'},
+            'biblio'         => $hold->biblionumber,
+            'biblioitems'    => $hold->biblionumber,
+            'reserves'       => $hold->unblessed,
+            'items'          => $hold->itemnumber,
         },
         substitute => { today => output_pref( { dt => dt_from_string, dateonly => 1 } ) },
     );

diff --git a/t/db_dependent/Reserves/MultiplePerRecord.t b/t/db_dependent/Reserves/MultiplePerRecord.t
index 559cef4..4ea12c0 100755 (executable)
--- a/t/db_dependent/Reserves/MultiplePerRecord.t
+++ b/t/db_dependent/Reserves/MultiplePerRecord.t
@@ -80,6 +80,7 @@ my $item1 = $builder->build(
             itype         => $itemtype1->{itemtype},
             homebranch    => $library->{branchcode},
             holdingbranch => $library->{branchcode},
+            damaged       => 0,
         },
     }
 );
@@ -91,6 +92,7 @@ my $item2 = $builder->build(
             itype         => $itemtype2->{itemtype},
             homebranch    => $library->{branchcode},
             holdingbranch => $library->{branchcode},
+            damaged       => 0,
         },
     }
 );
@@ -102,6 +104,7 @@ my $item3 = $builder->build(
             itype         => $itemtype2->{itemtype},
             homebranch    => $library->{branchcode},
             holdingbranch => $library->{branchcode},
+            damaged       => 0,
         },
     }
 );