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 <wizzyrea@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/C4/Letters.pm b/C4/Letters.pm
index 40d2f18..a2c63cb 100644 (file)
--- 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 (file)
index 0000000..5f03d82
--- /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 aa39f89..e41efb6 100644 (file)
--- 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 0b7bdff..f142317 100644 (file)
--- 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 @@
                 <th>Status</th>
                 <th>Updated on</th>
                 <th>Time created</th>
+                <th>Delivery note</th>
             </tr>
         </thead>
        <tbody>
@@ -90,7 +91,8 @@
         </td>
         <td data-order="[% QUEUED_MESSAGE.updated_on | html %]">[% QUEUED_MESSAGE.updated_on | $KohaDates  with_hours => 1 %]</td>
         <td data-order="[% QUEUED_MESSAGE.time_queued | html %]">[% QUEUED_MESSAGE.time_queued | $KohaDates  with_hours => 1 %]</td>
-           </tr>
+           <td>[% QUEUED_MESSAGE.delivery_note | html %]</td>
+        </tr>
            [% END %]
        </tbody>
     </table>

diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t
index 35eb84d..5913e7a 100755 (executable)
--- a/t/db_dependent/Letters.t
+++ b/t/db_dependent/Letters.t
@@ -18,7 +18,7 @@
 # along with Koha; if not, see <http://www.gnu.org/licenses>.
 
 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' );