@@ -, +, @@ --- C4/Letters.pm | 32 ++-- ...4723_-_Additional_delivery_notes_to_messages.pl | 14 ++ .../prog/en/modules/members/notices.tt | 4 + t/db_dependent/Letters.t | 7 +- t/db_dependent/MessageDeliveryNote.t | 212 +++++++++++++++++++++ 5 files changed, 256 insertions(+), 13 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.pl create mode 100644 t/db_dependent/MessageDeliveryNote.t --- a/C4/Letters.pm +++ a/C4/Letters.pm @@ -911,9 +911,9 @@ sub EnqueueLetter { my $dbh = C4::Context->dbh(); my $statement = << 'ENDSQL'; INSERT INTO message_queue -( borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, content_type ) +( borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, content_type, delivery_note ) VALUES -( ?, ?, ?, ?, ?, ?, ?, NOW(), ?, ?, ? ) +( ?, ?, ?, ?, ?, ?, ?, NOW(), ?, ?, ?, ? ) ENDSQL my $sth = $dbh->prepare($statement); @@ -928,6 +928,7 @@ ENDSQL $params->{'to_address'}, # to_address $params->{'from_address'}, # from_address $params->{'letter'}->{'content-type'}, # content_type + $params->{'delivery_note'} || '', # delivery_note ); return $dbh->last_insert_id(undef,undef,'message_queue', undef); } @@ -1017,7 +1018,7 @@ sub GetQueuedMessages { my $dbh = C4::Context->dbh(); my $statement = << 'ENDSQL'; -SELECT message_id, borrowernumber, subject, content, message_transport_type, status, time_queued +SELECT message_id, borrowernumber, subject, content, message_transport_type, status, time_queued, delivery_note FROM message_queue ENDSQL @@ -1150,7 +1151,8 @@ sub _send_message_by_email { unless ($member) { warn "FAIL: No 'to_address' and INVALID borrowernumber ($message->{borrowernumber})"; _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); + status => 'failed', + delivery_note => 'Invalid borrowernumber '.$message->{borrowernumber} } ); return; } $to_address = C4::Members::GetNoticeEmailAddress( $message->{'borrowernumber'} ); @@ -1158,7 +1160,8 @@ sub _send_message_by_email { # warn "FAIL: No 'to_address' and no email for " . ($member->{surname} ||'') . ", borrowernumber ($message->{borrowernumber})"; # warning too verbose for this more common case? _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); + status => 'failed', + delivery_note => 'Unable to find an email address for this borrower' } ); return; } } @@ -1199,11 +1202,13 @@ sub _send_message_by_email { _update_message_to_address($message->{'message_id'},$to_address) unless $message->{to_address}; #if initial message address was empty, coming here means that a to address was found and queue should be updated if ( sendmail( %sendmail_params ) ) { _set_message_status( { message_id => $message->{'message_id'}, - status => 'sent' } ); + status => 'sent', + delivery_note => '' } ); return 1; } else { _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); + status => 'failed', + delivery_note => $Mail::Sendmail::error } ); carp $Mail::Sendmail::error; return; } @@ -1252,13 +1257,15 @@ sub _send_message_by_sms { unless ( $member->{smsalertnumber} ) { _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); + status => 'failed', + delivery_note => 'Missing SMS number' } ); return; } if ( _is_duplicate( $message ) ) { _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' } ); + status => 'failed', + delivery_note => 'Message is duplicate' } ); return; } @@ -1266,7 +1273,9 @@ sub _send_message_by_sms { message => $message->{'content'}, } ); _set_message_status( { message_id => $message->{'message_id'}, - status => ($success ? 'sent' : 'failed') } ); + status => ($success ? 'sent' : 'failed'), + delivery_note => ($success ? '' : 'No notes from SMS driver') } ); + return $success; } @@ -1284,9 +1293,10 @@ sub _set_message_status { } my $dbh = C4::Context->dbh(); - my $statement = 'UPDATE message_queue SET status= ? WHERE message_id = ?'; + my $statement = 'UPDATE message_queue SET status= ?, delivery_note= ? WHERE message_id = ?'; my $sth = $dbh->prepare( $statement ); my $result = $sth->execute( $params->{'status'}, + $params->{'delivery_note'} || '', $params->{'message_id'} ); return $result; } --- a/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.pl +++ a/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.pl @@ -0,0 +1,14 @@ +#! /usr/bin/perl + +use strict; +use warnings; +use C4::Context; +use Koha::AtomicUpdater; + +my $dbh = C4::Context->dbh; +my $atomicUpdater = Koha::AtomicUpdater->new(); + +unless($atomicUpdater->find('Bug14723')) { + $dbh->do("ALTER TABLE message_queue ADD delivery_note TEXT"); + print "Upgrade to done (Bug 14723 - Additional delivery notes to messages)\n"; +} --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -49,6 +49,7 @@ Type Status Time + Delivery note @@ -75,6 +76,9 @@ [% ELSE %][% QUEUED_MESSAGE.status %][% END %] [% QUEUED_MESSAGE.time_queued | $KohaDates with_hours => 1 %] + + [% QUEUED_MESSAGE.delivery_note %] + [% END %] --- a/t/db_dependent/Letters.t +++ a/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 65; +use Test::More tests => 67; use Test::MockModule; use Test::Warn; @@ -123,7 +123,7 @@ is( $messages->[0]->{subject}, $my_message->{letter}->{title}, 'EnqueueLetter st is( $messages->[0]->{content}, $my_message->{letter}->{content}, 'EnqueueLetter stores the content correctly' ); is( $messages->[0]->{message_transport_type}, $my_message->{message_transport_type}, 'EnqueueLetter stores the message type correctly' ); is( $messages->[0]->{status}, 'pending', 'EnqueueLetter stores the status pending correctly' ); - +is( $messages->[0]->{delivery_note}, '', 'Delivery note for successful message correctly empty'); # SendQueuedMessages my $messages_processed = C4::Letters::SendQueuedMessages(); @@ -136,6 +136,9 @@ is( 'message marked failed if tried to send SMS message for borrower with no smsalertnumber set (bug 11208)' ); +# Delivery notes +is($messages->[0]->{delivery_note}, 'Missing SMS number', 'Delivery note for no smsalertnumber correctly set'); + # GetLetters my $letters = C4::Letters::GetLetters(); is( @$letters, 0, 'GetLetters returns the correct number of letters' ); --- a/t/db_dependent/MessageDeliveryNote.t +++ a/t/db_dependent/MessageDeliveryNote.t @@ -0,0 +1,212 @@ +#!/usr/bin/env perl + +# Copyright 2015 Open Source Freedom Fighters +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . +$ENV{KOHA_PAGEOBJECT_DEBUG} = 1; +use Modern::Perl; + +use Test::More; +use Try::Tiny; #Even Selenium::Remote::Driver uses Try::Tiny :) + +use Koha::Auth::PermissionManager; + +use t::lib::Page::Mainpage; +use t::lib::Page::Opac::OpacMain; +use t::lib::Page::Opac::OpacMemberentry; +use t::lib::Page::Members::Memberentry; +use t::lib::Page::Members::Moremember; +use t::lib::Page::Members::Notices; + +use t::lib::TestObjects::BorrowerFactory; +use t::lib::TestObjects::MessageQueueFactory; +use t::lib::TestObjects::SystemPreferenceFactory; + +use C4::Context; +use C4::Members; + +##Setting up the test context +my $testContext = {}; + +my $password = '1234'; +my $invalidBorrowerNumber = '000'; +my $borrowerFactory = t::lib::TestObjects::BorrowerFactory->new(); +my $borrowers = $borrowerFactory->createTestGroup([ + {firstname => 'Testthree', + surname => 'Testfour', + cardnumber => 'superuberadmin', + branchcode => 'CPL', + userid => 'god', + address => 'testi', + city => 'joensuu', + zipcode => '80100', + password => $password, + }, + ], undef, $testContext); +my $messages = t::lib::TestObjects::MessageQueueFactory->createTestGroup([ + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'sms', + from_address => 'A001@example.com', + }, + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'sms', + from_address => 'A002@example.com', + }, + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => '', + from_address => 'B001@example.com', + }, + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => 'nobody@example.com', + from_address => 'B002@example.com', + }, + {letter => { + title => "INVALID USER", + content => "INVALID USER", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => '', + from_address => 'B003@example.com', + }, + ], undef, $testContext); +my $systempreferences = t::lib::TestObjects::SystemPreferenceFactory->createTestGroup([ + {preference => 'SMSSendDriver', + value => 'nonexistentdriver' + }, + { + preference => 'EnhancedMessagingPreferences', + value => '1' + } + ], undef, $testContext); + +my $permissionManager = Koha::Auth::PermissionManager->new(); +$permissionManager->grantPermissions($borrowers->{'superuberadmin'}, {superlibrarian => 'superlibrarian'}); + +eval { + + # staff client + my $notices = t::lib::Page::Members::Notices->new({borrowernumber => $borrowers->{'superuberadmin'}->borrowernumber}); + + # let's send the messages + + # first, send one email message with invalid borrower number + $messages->{'B003@example.com'}->{borrowernumber} = $invalidBorrowerNumber; + C4::Letters::_send_message_by_email($messages->{'B003@example.com'}); + + # the rest should produce errors automatically without our modificaitons + C4::Letters::SendQueuedMessages(); + + # then, login to intranet + $notices->doPasswordLogin($borrowers->{'superuberadmin'}->userid(), $password); + + # test delivery notes for SMS messaging + DoSMSTests($notices); + # check delivery notes for email messaging + CheckEmailMessages($notices); + +}; +if ($@) { #Catch all leaking errors and gracefully terminate. + warn $@; + tearDown(); + exit 1; +} + +##All tests done, tear down test context +tearDown(); +done_testing; + +sub tearDown { + t::lib::TestObjects::ObjectFactory->tearDownTestContext($testContext); +} + + + + + + + + +###################################################### + ### STARTING TEST IMPLEMENTATIONS ### +###################################################### + +sub DoSMSTests { + my ($notices) = @_; + + # login and check that our table is displayed correctly + $notices-> + hasDeliveryNoteColumnInNoticesTable()-> + hasTextInTableCell("Missing SMS number"); + + # now let's give the user a SMS number so we proceed to next step of failure + ModMember( borrowernumber => $borrowers->{superuberadmin}->borrowernumber, + smsalertnumber => 'just_some_number' ); + + # set first to sent and second to pending to check "duplicate" (Letters::_is_duplicate()) + setMessageStatus($messages->{'A001@example.com'}->{message_id}, "sent"); + setMessageStatus($messages->{'A002@example.com'}->{message_id}, "pending"); + + C4::Letters::SendQueuedMessages(); + # A002 should go into failed: message is duplicate status + + # set A001 back to pending to check that we get no reply from driver + setMessageStatus($messages->{'A001@example.com'}->{message_id}, "pending"); + + # let's send the message and check for delivery note, we should get a warning + # for non-installed driver. + C4::Letters::SendQueuedMessages(); + + $notices->navigateToNotices()-> + hasTextInTableCell("Message is duplicate")-> + hasTextInTableCell("No notes from SMS driver"); +} + +sub CheckEmailMessages { + my ($notices) = @_; + + $notices-> + hasTextInTableCell("Unable to find an email address")-> + hasTextInTableCell("Invalid borrowernumber")-> + hasTextInTableCell("Connection refused"); #sendmail should also fail +} +sub setMessageStatus { + my ($message_id, $status) = @_; + my $dbh = C4::Context->dbh; + + my $sth = $dbh->prepare("UPDATE message_queue SET status=?, delivery_note='' WHERE message_id=?"); + my $result = $sth->execute($status, $message_id); + + return $result; +} --