From a2e5c105cca3ed11eba9956a5f69604fbe72dcd7 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. --- 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 @@ Type Status Time - + Delivery note + [% FOREACH QUEUED_MESSAGE IN QUEUED_MESSAGES %] @@ -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..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 . +$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