From 234b175a08d89f7dbfbd060f2b81124880dff8d6 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Wed, 12 Apr 2017 11:14:44 +0000 Subject: [PATCH] Bug 14723: Additional delivery notes to messages This patch adds additional delivery notes to messages in message queue as there can be multiple reasons for a delivery to fail. Currently in message_queue we are given only two delivery statuses for messages, "sent" and "failed". When the status becomes failed, we have no idea why it fails. This feature can be useful with SMS gateway providers. Many SMS gateways inform the application the reason of SMS delivery failure. With this feature, this information can now be stored. As well as for emails, instead of simply logging failures, we can now store the reason of failure directly into the message row of message_queue. Test plan: 1. Enable EnhancedMessagingPreferences syspref 2. Find a borrower with notices at members/notices.pl 3. Observe that there is no column for Delivery notes 4. Apply patch and run the given database update 5. Repeat step 1. 6. Observe that there is now a column for Delivery notes --- C4/Letters.pm | 36 ++++++++++++++-------- ...23_-_Additional_delivery_notes_to_messages.perl | 8 +++++ installer/data/mysql/kohastructure.sql | 1 + .../prog/en/modules/members/notices.tt | 6 +++- t/db_dependent/Letters.t | 9 ++++-- 5 files changed, 44 insertions(+), 16 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.perl diff --git a/C4/Letters.pm b/C4/Letters.pm index 52db4d0..968378a 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -906,9 +906,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); @@ -923,6 +923,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); } @@ -1056,7 +1057,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 @@ -1110,7 +1111,7 @@ sub GetMessage { return unless $message_id; my $dbh = C4::Context->dbh; return $dbh->selectrow_hashref(q| - SELECT message_id, borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, content_type + SELECT message_id, borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, content_type, delivery_note FROM message_queue WHERE message_id = ? |, {}, $message_id ); @@ -1214,7 +1215,7 @@ sub _get_unsent_messages { my $dbh = C4::Context->dbh(); my $statement = qq{ - SELECT mq.message_id, mq.borrowernumber, mq.subject, mq.content, mq.message_transport_type, mq.status, mq.time_queued, mq.from_address, mq.to_address, mq.content_type, b.branchcode, mq.letter_code + SELECT mq.message_id, mq.borrowernumber, mq.subject, mq.content, mq.message_transport_type, mq.status, mq.time_queued, mq.from_address, mq.to_address, mq.content_type, b.branchcode, mq.letter_code, mq.delivery_note FROM message_queue mq LEFT JOIN borrowers b ON b.borrowernumber = mq.borrowernumber WHERE status = ? @@ -1261,7 +1262,8 @@ sub _send_message_by_email { unless ($patron) { 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 = $patron->notice_email_address; @@ -1269,7 +1271,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; } } @@ -1311,11 +1314,13 @@ sub _send_message_by_email { if ( Mail::Sendmail::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; } @@ -1364,13 +1369,15 @@ sub _send_message_by_sms { unless ( $patron and $patron->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; } @@ -1378,7 +1385,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; } @@ -1396,9 +1405,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; } diff --git a/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.perl b/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.perl new file mode 100644 index 0000000..3b1cbe1 --- /dev/null +++ b/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.perl @@ -0,0 +1,8 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do("ALTER TABLE message_queue ADD delivery_note TEXT"); + + # Always end with this (adjust the bug info) + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 14723 - Additional delivery notes to messages)\n"; +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 45fd117..be59ade 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -2503,6 +2503,7 @@ CREATE TABLE `message_queue` ( `to_address` LONGTEXT, `from_address` LONGTEXT, `content_type` MEDIUMTEXT, + `delivery_note` MEDIUMTEXT, PRIMARY KEY `message_id` (`message_id`), KEY `borrowernumber` (`borrowernumber`), KEY `message_transport_type` (`message_transport_type`), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt index a26d5f0..4bb5bb6 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -36,6 +36,7 @@ Type Status Time + Delivery note @@ -74,6 +75,9 @@ [% END %] [% QUEUED_MESSAGE.time_queued | $KohaDates with_hours => 1 | html %] + + [% QUEUED_MESSAGE.delivery_note | html %] + [% END %] @@ -97,7 +101,7 @@ $(document).ready(function() { $("#noticestable").dataTable($.extend(true, {}, dataTablesDefaults, { "aaSorting": [[ 3, "desc" ]], - "aoColumns": [ null,null,null,{ "sType": "title-string" } ], + "aoColumns": [ null,null,null,{ "sType": "title-string" }, null ], "sPaginationType": "four_button" })); diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 344473c..ec3b9c1 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 64; +use Test::More tests => 66; use Test::MockModule; use Test::Warn; @@ -132,7 +132,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( { type => 'email' }); @@ -156,6 +156,11 @@ is( $resent, 0, 'The message should not have been resent again' ); $resent = C4::Letters::ResendMessage(); is( $resent, undef, 'ResendMessage should return undef if not message_id given' ); +# 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' ); -- 2.1.4