Bugzilla – Attachment 42227 Details for
Bug 14723
Additional delivery notes to messages
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14723 - Additional delivery notes to messages
Bug-14723---Additional-delivery-notes-to-messages.patch (text/plain), 15.09 KB, created by
Lari Taskula
on 2015-09-02 14:57:29 UTC
(
hide
)
Description:
Bug 14723 - Additional delivery notes to messages
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2015-09-02 14:57:29 UTC
Size:
15.09 KB
patch
obsolete
>From 805d3517a4800c4fa4840625f47d67244c516b47 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. > >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 | 28 ++- > ...4723_-_Additional_delivery_notes_to_messages.pl | 8 + > .../prog/en/modules/members/notices.tt | 4 + > t/db_dependent/MessageDeliveryNote.t | 208 +++++++++++++++++++++ > 4 files changed, 239 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..489d7ce 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,26 @@ 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') } ); >+ status => ($success ? 'sent' : 'failed'), >+ delivery_note => ($success ? '' : 'No notes from SMS driver') } ); >+ > return $success; > } > >@@ -1271,9 +1280,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 @@ > <th>Type</th> > <th>Status</th> > <th>Time</th> >+ <th>Delivery note</th> > </tr> > </thead> > <tbody> >@@ -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..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 <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; >+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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=42227">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
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