From 4abcbd93ac923ab1cc048a8b1f6f86c3b7e14c1c Mon Sep 17 00:00:00 2001 From: Benjamin Rokseth Date: Tue, 13 Dec 2016 14:57:52 +0100 Subject: [PATCH] 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 <> 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 Signed-off-by: Kyle M Hall Signed-off-by: Kyle M Hall (cherry picked from commit 5a08969a71dc27562a1a22af809673a8727f5c82) Signed-off-by: Katrin Fischer --- C4/Reserves.pm | 39 +++++++++------------ t/db_dependent/Reserves/MultiplePerRecord.t | 3 ++ 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 83903e89a1..194fa15412 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1287,12 +1287,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 @@ -1351,7 +1351,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 } ); @@ -1959,7 +1959,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) @@ -1986,9 +1986,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 @@ -1999,28 +2000,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 559cef4007..4ea12c0f4b 100755 --- 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, }, } ); -- 2.39.5