From aa450994422fb62742e3b20931c6f2f07c638602 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 Sponsored-by: Hypernova Oy Signed-off-by: Liz Rea Signed-off-by: Kyle M Hall Signed-off-by: Jonathan Druart --- C4/Letters.pm | 36 ++++++++++++------- ...Additional_delivery_notes_to_messages.perl | 8 +++++ installer/data/mysql/kohastructure.sql | 1 + .../prog/en/modules/members/notices.tt | 4 ++- t/db_dependent/Letters.t | 8 ++++- 5 files changed, 42 insertions(+), 15 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 40d2f18a3c..a2c63cb2e7 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -963,9 +963,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, reply_address, content_type ) +( borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, reply_address, content_type, delivery_note ) VALUES -( ?, ?, ?, ?, ?, ?, ?, NOW(), ?, ?, ?, ? ) +( ?, ?, ?, ?, ?, ?, ?, NOW(), ?, ?, ?, ?, ? ) ENDSQL my $sth = $dbh->prepare($statement); @@ -981,6 +981,7 @@ ENDSQL $params->{'from_address'}, # from_address $params->{'reply_address'}, # reply_address $params->{'letter'}->{'content-type'}, # content_type + $params->{'delivery_note'} || '', # delivery_note ); return $dbh->last_insert_id(undef,undef,'message_queue', undef); } @@ -1123,7 +1124,7 @@ sub GetQueuedMessages { my $dbh = C4::Context->dbh(); my $statement = << 'ENDSQL'; -SELECT message_id, borrowernumber, subject, content, message_transport_type, status, time_queued, updated_on +SELECT message_id, borrowernumber, subject, content, message_transport_type, status, time_queued, updated_on, delivery_note FROM message_queue ENDSQL @@ -1177,7 +1178,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, updated_on, to_address, from_address, reply_address, content_type + SELECT message_id, borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, updated_on, to_address, from_address, reply_address, content_type, delivery_note FROM message_queue WHERE message_id = ? |, {}, $message_id ); @@ -1282,7 +1283,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.reply_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.reply_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 = ? @@ -1333,7 +1334,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; @@ -1341,7 +1343,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; } } @@ -1417,7 +1420,8 @@ sub _send_message_by_email { _set_message_status( { message_id => $message->{'message_id'}, - status => 'sent' + status => 'sent', + delivery_note => '' } ); return 1; @@ -1426,7 +1430,8 @@ sub _send_message_by_email { _set_message_status( { message_id => $message->{'message_id'}, - status => 'failed' + status => 'failed', + delivery_note => $Mail::Sendmail::error } ); carp "$_"; @@ -1477,13 +1482,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; } @@ -1491,7 +1498,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; } @@ -1515,9 +1524,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 0000000000..5f03d8200d --- /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 mediumtext"); + + # 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 aa39f89043..e41efb653b 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3625,6 +3625,7 @@ CREATE TABLE `message_queue` ( `from_address` longtext COLLATE utf8mb4_unicode_ci DEFAULT NULL, `reply_address` longtext COLLATE utf8mb4_unicode_ci DEFAULT NULL, `content_type` mediumtext COLLATE utf8mb4_unicode_ci DEFAULT NULL, + `delivery_note` mediumtext COLLATE utf8mb4_unicode_ci DEFAULT NULL, PRIMARY KEY (`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 0b7bdffd15..f14231707a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -51,6 +51,7 @@ Status Updated on Time created + Delivery note @@ -90,7 +91,8 @@ [% QUEUED_MESSAGE.updated_on | $KohaDates with_hours => 1 %] [% QUEUED_MESSAGE.time_queued | $KohaDates with_hours => 1 %] - + [% QUEUED_MESSAGE.delivery_note | html %] + [% END %] diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 35eb84d32e..5913e7af96 100755 --- 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 => 84; +use Test::More tests => 86; use Test::MockModule; use Test::Warn; use Test::Exception; @@ -146,6 +146,7 @@ is( $messages->[0]->{message_transport_type}, $my_message->{message_transport_ty is( $messages->[0]->{status}, 'pending', 'EnqueueLetter stores the status pending correctly' ); isnt( $messages->[0]->{time_queued}, undef, 'Time queued inserted by default in message_queue table' ); is( $messages->[0]->{updated_on}, $messages->[0]->{time_queued}, 'Time status changed equals time queued when created in message_queue table' ); +is( $messages->[0]->{delivery_note}, '', 'Delivery note for successful message correctly empty'); # Setting time_queued to something else than now my $yesterday = dt_from_string->subtract( days => 1 ); @@ -175,6 +176,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.39.5