From 1de9e54d53832b6df06c09e0b71186272bd00af8 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. Find a borrower with notices at members/notices.pl 2. Observe that there is no column for Delivery notes 3. Apply patch and run the given database update 4. Repeat step 1. 5. Observe that there is now a column for Delivery notes Includes PageObject Selenium test for delivery failures in C4::Letters. --- C4/Letters.pm | 27 ++- ...4723_-_Additional_delivery_notes_to_messages.pl | 8 + .../prog/en/modules/members/notices.tt | 4 + t/db_dependent/MessageDeliveryNote.t | 208 +++++++++++++++++++++ 4 files changed, 238 insertions(+), 9 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..f49da44 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,13 +1243,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; } @@ -1253,7 +1259,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; } @@ -1271,9 +1279,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..65cdd35 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt @@ -49,6 +49,7 @@ Type Status Time + Delivery note @@ -75,6 +76,9 @@ [% ELSE %][% QUEUED_MESSAGE.status %][% END %] [% QUEUED_MESSAGE.time_queued | $KohaDates with_hours => 1 %] + + [% QUEUED_MESSAGE.delivery_note %] + [% END %] diff --git a/t/db_dependent/MessageDeliveryNote.t b/t/db_dependent/MessageDeliveryNote.t new file mode 100644 index 0000000..d5964de --- /dev/null +++ b/t/db_dependent/MessageDeliveryNote.t @@ -0,0 +1,208 @@ +#!/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([ + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'sms', + from_address => 'A001@example.com', + }, + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'sms', + from_address => 'A002@example.com', + }, + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => '', + from_address => 'B001@example.com', + }, + {letter => { + title => "Test title", + content => "Tessst content", + }, + cardnumber => $borrowers->{'superuberadmin'}->cardnumber, + message_transport_type => 'email', + to_address => 'nobody@example.com', + from_address => 'B002@example.com', + }, + {letter => { + title => "INVALID USER", + content => "INVALID USER", + }, + 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' + }, + ], 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