Attachment 42053 Details for Bug 14723 – Bug 14723 - Additional delivery notes to messages

[patch] Bug 14723 - Additional delivery notes to messages

Bug-14723---Additional-delivery-notes-to-messages.patch (text/plain), 11.88 KB, created by Lari Taskula on 2015-08-28 09:53:08 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2015-08-28 09:53:08 UTC

Size: 11.88 KB

patch

obsolete

>From a2e5c105cca3ed11eba9956a5f69604fbe72dcd7 Mon Sep 17 00:00:00 2001
>From: Lari Taskula <larit@student.uef.fi>
>Date: Fri, 28 Aug 2015 12:16:19 +0300
>Subject: [PATCH] Bug 14723 - Additional delivery notes to messages
>
>This patch adds additional delivery notes to messages in message queue. They are
>useful, because there can be multiple reasons for a delivery to fail.
>
>Currently at 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.
>
>Some SMS gateway providers send the customer a report about the status of delivery.
>For example, in our local library we have a provider that does this. With the SMS
>HTTP request to their Gateway, we also provide them a "report" parameter that includes
>an URL. Once the SMS delivery has been finished, they will notify us with the status
>to the given report URL.
>
>If they notify us that the delivery was failed, we will modify its status in the
>database accordingly. They also provide a short description of why the delivery
>was failed.
>
>To be able to store more detailed information about the delivery, I propose adding
>a new column (e.g "delivery_notes") to message_queue table.
>
>In our local library we sometimes have angry borrowers complaining that they have
>not received SMS messages, so this type of enhancement would be very useful
>to be able to find out why such has happened.
>---
> C4/Letters.pm                                      |  40 ++++++--
> ...4723_-_Additional_delivery_notes_to_messages.pl |   8 ++
> .../prog/en/modules/members/notices.tt             |   6 +-
> t/db_dependent/MessageDeliveryNote.t               | 111 +++++++++++++++++++++
> 4 files changed, 154 insertions(+), 11 deletions(-)
> create mode 100644 installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.pl
> create mode 100644 t/db_dependent/MessageDeliveryNote.t
>
>diff --git a/C4/Letters.pm b/C4/Letters.pm
>index db9f987..49c314a 100644
>--- a/C4/Letters.pm
>+++ b/C4/Letters.pm
>@@ -1004,7 +1004,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
> 
>@@ -1137,7 +1137,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'} );
>@@ -1145,7 +1146,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;
>         }
>     }
>@@ -1186,11 +1188,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;
>     }
>@@ -1239,21 +1243,36 @@ 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;
>     }
> 
>     my $success = C4::SMS->send_sms( { destination => $member->{'smsalertnumber'},
>                                        message     => $message->{'content'},
>+                                       message_id  => $message->{'message_id'}
>                                      } );
>-    _set_message_status( { message_id => $message->{'message_id'},
>-                           status     => ($success ? 'sent' : 'failed') } );
>+    
>+    # Some SMS::Send drivers return a hash with delivery note, and some only
>+    # success value. Let's add an appropriate delivery note if driver returns
>+    # us one.
>+    if (ref($success) eq 'HASH') {
>+        _set_message_status( { message_id => $message->{'message_id'},
>+                           status     => ($success->{status} ? 'sent' : 'failed'),
>+                           delivery_note => $success->{delivery_note} } );
>+    } else {
>+        _set_message_status( { message_id => $message->{'message_id'},
>+                           status     => ($success ? 'sent' : 'failed'),
>+                           delivery_note => ($success ? '' : 'No notes from SMS driver') } );
>+    }
>+  
>     return $success;
> }
> 
>@@ -1271,9 +1290,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.pl b/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.pl
>new file mode 100644
>index 0000000..31495e3
>--- /dev/null
>+++ b/installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.pl
>@@ -0,0 +1,8 @@
>+#! /usr/bin/perl
>+use strict;
>+use warnings;
>+use C4::Context;
>+my $dbh = C4::Context->dbh;
>+
>+$dbh->do("ALTER TABLE message_queue ADD delivery_note TEXT");
>+print "Upgrade to done (Bug 14723 - Additional delivery notes to messages)\n";
>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 72cb466..327b010 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
>@@ -49,7 +49,8 @@
> 		<th>Type</th>
> 		<th>Status</th>
> 		<th>Time</th>
>-	    </tr>
>+	        <th>Delivery note</th>
>+            </tr>
> 	</thead>
> 	<tbody>
> 	    [% FOREACH QUEUED_MESSAGE IN QUEUED_MESSAGES %]
>@@ -75,6 +76,9 @@
>             [% ELSE %][% QUEUED_MESSAGE.status %][% END %]
>         </td>
>         <td><span title="[% QUEUED_MESSAGE.time_queued %]">[% QUEUED_MESSAGE.time_queued | $KohaDates with_hours => 1 %]</span></td>
>+                <td>
>+            [% QUEUED_MESSAGE.delivery_note %]
>+        </td>
> 	    </tr>
> 	    [% END %]
> 	</tbody>
>diff --git a/t/db_dependent/MessageDeliveryNote.t b/t/db_dependent/MessageDeliveryNote.t
>new file mode 100644
>index 0000000..9956389
>--- /dev/null
>+++ b/t/db_dependent/MessageDeliveryNote.t
>@@ -0,0 +1,111 @@
>+#!/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 <http://www.gnu.org/licenses>.
>+$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;
>+
>+##Setting up the test context
>+my $testContext = {};
>+
>+my $password = '1234';
>+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',
>+             message_id => '123',
>+            },
>+        ], 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, op => 'modify', destination => 'circ', categorycode => 'PT'});
>+
>+    # login and check that our table is displayed correctly
>+    $notices->doPasswordLogin($borrowers->{'superuberadmin'}->userid(), $password)->
>+    hasDeliveryNoteColumnInNoticesTable()->
>+    hasTextInTableCell(""); # exactly one should be empty (delivery note)
>+    
>+    # let's send the message
>+    C4::Letters::SendQueuedMessages({ username => "test", password=> "lol" });
>+    
>+    # message sending should fail, lets check it. refresh page first
>+    $notices->navigateToNotices()->
>+    hasTextInTableCell("failed")->
>+    hasTextInTableCell("Missing SMS number");
>+    
>+};
>+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         ###
>+######################################################
>+
>-- 
>1.9.1

Actions: View | Diff | Splinter Review

Attachments on bug 14723: 42053 | 42074 | 42159 | 42170 | 42227 | 42228 | 42384 | 42453 | 42455 | 42466 | 42505 | 42553 | 42574 | 42668 | 42669 | 44367 | 44413 | 62080 | 62138 | 78313 | 87525 | 87531 | 105776 | 105777 | 116642 | 116643 | 118225 | 120252 | 120309 | 120310 | 120312 | 120314 | 120376 | 120487 | 120488 | 120489 | 120490 | 120491 | 120492 | 120667