From 4440702570f807d1232ee9dc7a1e658e7c5d92e6 Mon Sep 17 00:00:00 2001 From: Lari Taskula 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. 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 Includes PageObject Selenium test for delivery failures in C4::Letters. --- C4/Letters.pm | 34 ++-- ...4723_-_Additional_delivery_notes_to_messages.pl | 14 ++ .../prog/en/modules/members/notices.tt | 6 +- t/db_dependent/Letters.t | 7 +- t/db_dependent/MessageDeliveryNote.t | 202 +++++++++++++++++++++ 5 files changed, 248 insertions(+), 15 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 824d621..08e73e6 100644 --- a/C4/Letters.pm +++ b/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 @@ -1071,7 +1072,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 ); @@ -1195,7 +1196,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'} ); @@ -1203,7 +1205,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; } } @@ -1244,11 +1247,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; } @@ -1297,13 +1302,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; } @@ -1311,7 +1318,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; } @@ -1329,9 +1338,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..d3fdbc8 --- /dev/null +++ b/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"; +} \ No newline at end of file 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 3dcb059..0208374 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -10,7 +10,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" })); @@ -49,6 +49,7 @@ Type Status Time + Delivery note @@ -85,6 +86,9 @@ [% END %] [% QUEUED_MESSAGE.time_queued | $KohaDates with_hours => 1 %] + + [% QUEUED_MESSAGE.delivery_note %] + [% END %] diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 78cb458..d3ef012 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 => 69; +use Test::More tests => 71; 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(); @@ -146,6 +146,9 @@ 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' ); diff --git a/t/db_dependent/MessageDeliveryNote.t b/t/db_dependent/MessageDeliveryNote.t new file mode 100644 index 0000000..938303b --- /dev/null +++ b/t/db_dependent/MessageDeliveryNote.t @@ -0,0 +1,202 @@ +#!/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([ + {subject => "Test title", + content => "Tessst content", + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'sms', + from_address => 'A001@example.com', + }, + {subject => "Test title", + content => "Tessst content", + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'sms', + from_address => 'A002@example.com', + }, + {subject => "Test title", + content => "Tessst content", + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => '', + from_address => 'B001@example.com', + }, + {subject => "Test title", + content => "Tessst content", + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => 'nobody@example.com', + from_address => 'B002@example.com', + }, + {subject => "Test title", + content => "Tessst content", + 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; +} \ No newline at end of file -- 1.9.1