Attachment 37484 Details for Bug 8000 – Bug 8000 - Correct inability to know where message was sent

[patch] Bug 8000 - Correct inability to know where message was sent

Bug-8000---Correct-inability-to-know-where-message.patch (text/plain), 6.45 KB, created by Mark Tompsett on 2015-04-03 17:48:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Tompsett

Created: 2015-04-03 17:48:22 UTC

Size: 6.45 KB

patch

obsolete

>From 2825cc3b2631bcbd1205a368d92321ef3a9f3ff9 Mon Sep 17 00:00:00 2001
>From: Mark Tompsett <mtompset@hotmail.com>
>Date: Thu, 6 Mar 2014 22:26:23 -0500
>Subject: [PATCH] Bug 8000 - Correct inability to know where message was sent
>
>The added column in the previous patch is used to determine where
>the message was actually sent when using email.
>
>This include a modification to the C4/Letters.pm libraries to
>appropriate track and update the column and revising the test
>file to check that the system preference actually does override
>or not the destination.
>---
> C4/Letters.pm            | 27 ++++++++++++++++++---------
> t/db_dependent/Letters.t | 32 +++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 10 deletions(-)
>
>diff --git a/C4/Letters.pm b/C4/Letters.pm
>index 62f49ba..b13ebb2 100644
>--- a/C4/Letters.pm
>+++ b/C4/Letters.pm
>@@ -1016,7 +1016,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, sentto_address
> FROM message_queue
> ENDSQL
> 
>@@ -1145,17 +1145,19 @@ sub _send_message_by_email {
> 
>     my $member = C4::Members::GetMember( 'borrowernumber' => $message->{'borrowernumber'} );
>     my $to_address = $message->{'to_address'};
>+    if (!$to_address && $member) {
>+        $to_address = C4::Members::GetNoticeEmailAddress( $message->{'borrowernumber'} );
>+    }
>     my $sendAllEmailsTo = C4::Context->preference('SendAllEmailsTo');
>     $to_address = $sendAllEmailsTo if ($sendAllEmailsTo && $sendAllEmailsTo =~ /@/ ); # some validation.  This could be improved.
>     unless ($to_address) {
>-        unless ($member) {
>+        if (!$member) {
>             warn "FAIL: No 'to_address' and INVALID borrowernumber ($message->{borrowernumber})";
>             _set_message_status( { message_id => $message->{'message_id'},
>                                    status     => 'failed' } );
>             return;
>         }
>-        $to_address = C4::Members::GetNoticeEmailAddress( $message->{'borrowernumber'} );
>-        unless ($to_address) {  
>+        else {
>             # 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'},
>@@ -1199,8 +1201,9 @@ 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' } );
>+        _set_message_status( { message_id     => $message->{'message_id'},
>+                               status         => 'sent',
>+                               sentto_address => $to_address } );
>         return 1;
>     } else {
>         _set_message_status( { message_id => $message->{'message_id'},
>@@ -1284,11 +1287,17 @@ sub _set_message_status {
>         return unless exists $params->{ $required_parameter };
>     }
> 
>-    my $dbh = C4::Context->dbh();
>     my $statement = 'UPDATE message_queue SET status= ? WHERE message_id = ?';
>+    my @values;
>+    push @values,$params->{'status'};
>+    if (exists($params->{'sentto_address'})) {
>+        $statement = 'UPDATE message_queue SET status=?, sentto_address=? WHERE message_id = ?';
>+        push @values,$params->{'sentto_address'};
>+    }
>+    push @values,$params->{'message_id'};
>+    my $dbh = C4::Context->dbh();
>     my $sth = $dbh->prepare( $statement );
>-    my $result = $sth->execute( $params->{'status'},
>-                                $params->{'message_id'} );
>+    my $result = $sth->execute( @values );
>     return $result;
> }
> 
>diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t
>index b0143e2..a42ade4 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 => 60;
>+use Test::More tests => 64;
> use Test::MockModule;
> use Test::Warn;
> 
>@@ -107,7 +107,12 @@ $my_message->{message_transport_type} = 'sms';
> $message_id = C4::Letters::EnqueueLetter($my_message);
> ok(defined $message_id && $message_id > 0, 'new message successfully queued');
> 
>+# This is will force C4::Context->preference() to read the DB.
>+C4::Context->disable_syspref_cache();
>+
> # used to trigger _send_message_by_email in C4::Letter
>+# first override.
>+$dbh->do("UPDATE systempreferences SET value='override\@example.com' WHERE variable='SendAllEmailsTo';");
> my $message_id2 = C4::Letters::EnqueueLetter({
>     borrowernumber         => $borrowernumber,
>     message_transport_type => 'email',
>@@ -146,11 +151,36 @@ is($messages_processed, 2, 'all queued messages processed');
> $messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber });
> is(scalar(@$messages), 2, 'two messages stored for the borrower');
> 
>+# used to trigger _send_message_by_email in C4::Letter
>+# second no override.
>+$dbh->do("UPDATE systempreferences SET value='' WHERE variable='SendAllEmailsTo';");
>+my $message_id3 = C4::Letters::EnqueueLetter({
>+    borrowernumber         => $borrowernumber,
>+    message_transport_type => 'email',
>+    to_address             => 'to@example.com',
>+    from_address           => 'from@example.com',
>+    letter => {
>+        content      => 'a message',
>+        title        => 'message title',
>+        metadata     => 'metadata',
>+        code         => 'TEST_MESSAGE',
>+        content_type => 'text/plain',
>+    },
>+});
>+
>+$messages_processed = C4::Letters::SendQueuedMessages();
>+is($messages_processed, 1, 'third queued message processed');
>+
>+$messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber });
>+is(scalar(@$messages), 3, 'three messages stored for the borrower');
>+
> is(
>     $messages->[0]->{status},
>     'failed',
>     'message marked failed if tried to send SMS message for borrower with no smsalertnumber set (bug 11208)'
> );
>+is ( $messages->[1]->{sentto_address},'override@example.com','Second message sent to override.');
>+is ( $messages->[2]->{sentto_address},'to@example.com','Third message sent to default.');
> 
> # GetLetters
> my $letters = C4::Letters::GetLetters();
>-- 
>1.9.1

Actions: View | Diff | Splinter Review

Attachments on bug 8000: 21976 | 22315 | 24784 | 25917 | 25919 | 25920 | 25923 | 25924 | 25925 | 25926 | 25927 | 25928 | 25939 | 25950 | 25951 | 26446 | 37463 | 37464 | 37465 | 37466 | 37467 | 37468 | 37469 | 37472 | 37473 | 37474 | 37475 | 37476 | 37477 | 37478 | 37481 | 37482 | 37483 | 37484 | 37485 | 37486 | 37487 | 39985 | 39986 | 39987 | 39988 | 39989 | 39990 | 39991 | 39992 | 40215 | 40216 | 40217 | 40218 | 40219 | 40220 | 40221 | 40222 | 40223 | 40224 | 40225 | 40226 | 40227 | 40228 | 40229 | 40230 | 46354 | 46355 | 46356 | 46357 | 46358 | 46359 | 46360 | 46361 | 46362 | 49991 | 49992 | 49993 | 49998 | 49999 | 50000 | 55940 | 58434 | 62863 | 67155 | 70086 | 70087 | 70484 | 74110 | 74111 | 74848 | 80370 | 81487 | 81488 | 81589 | 81590 | 81591 | 84801 | 84802 | 84803 | 84804 | 84805 | 86861 | 86862 | 86863 | 86864 | 86865 | 88874 | 88875 | 88876 | 88877 | 88878 | 88879