https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15986 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #54 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 1. 1824 my $admin_email_address = $library->{branchemail} || C4::Context->preference('KohaAdminEmailAddress'); Shouldn't it be $library->inbound_email_address instead? 2. 1844 next if ($mtt eq 'phone' and C4::Context->preference('TalkingTechItivaPhoneNotification') ); There are now 2 mtt, phone and itiva. Here you need to compare with 'itiva', and add a new condition for 'phone'. 3. Shouldn't we rename "send_notice" to "enqueue_notice" as we don't actually send it right away? 4. Why don't we insert a sample notice template and use it as the default for the script? 5. + # If passed message transports we force use those, otherwise we will use the patrons preferences + # for the 'Hold_Filled' notice + my $sending_params = @mtts ? { message_transports => \@mtts } : { message_name => "Hold_Filled" }; We should add that to the POD 6. + # Skip if we already dealt with this borrower + next if ( $done{$patron->borrowernumber} ); What if a patron has more than one hold? We won't be able to send a notice like "You have X holds waiting" 7. (not blocker) please don't forget to perltidy new files and blocks of code. -- You are receiving this mail because: You are watching all bug changes.