Bugzilla – Attachment 42553 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), 17.63 KB, created by
Lari Taskula
on 2015-09-15 10:10:24 UTC
(
hide
)
Description:
Bug 14723 - Additional delivery notes to messages
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2015-09-15 10:10:24 UTC
Size:
17.63 KB
patch
obsolete
>From e43fe43999e37d8bbc212ef49d10e296c16cb17d 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. 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 | 32 ++-- > ...4723_-_Additional_delivery_notes_to_messages.pl | 14 ++ > .../prog/en/modules/members/notices.tt | 4 + > t/db_dependent/Letters.t | 7 +- > t/db_dependent/MessageDeliveryNote.t | 202 +++++++++++++++++++++ > 5 files changed, 246 insertions(+), 13 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 2e6c4ce..c39bf72 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 > >@@ -1150,7 +1151,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'} ); >@@ -1158,7 +1160,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; > } > } >@@ -1199,11 +1202,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; > } >@@ -1252,13 +1257,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; > } > >@@ -1266,7 +1273,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; > } > >@@ -1284,9 +1293,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 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/Letters.t b/t/db_dependent/Letters.t >index cc3242c..8a0cc1c 100644 >--- a/t/db_dependent/Letters.t >+++ b/t/db_dependent/Letters.t >@@ -18,7 +18,7 @@ > # along with Koha; if not, see <http://www.gnu.org/licenses>. > > use Modern::Perl; >-use Test::More tests => 65; >+use Test::More tests => 67; > 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(); >@@ -136,6 +136,9 @@ is( > 'message marked failed if tried to send SMS message for borrower with no smsalertnumber set (bug 11208)' > ); > >+# 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 <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([ >+ {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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=42553">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