From b7510c59f5bbbeafca4bcf664edfa71db57c6c1c Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Mon, 14 Sep 2015 11:35:56 +0100
Subject: [PATCH] Bug 12426: Simplify the code adding a new subroutine
 GetMessage

The C4::Letters module does not have a GetMessage subroutine, which
could be quite useful.
This patch adds it and simplifies the code added by the previous patch.
It also adds a few tests and fixes POD typos.

Note that ResendNotice only resends failed messages. This will avoid to
resend already sent messages (using an url from the browser history for
instance).
---
 C4/Letters.pm                                      | 41 +++++++++++++++++-----
 .../prog/en/modules/members/notices.tt             |  2 +-
 members/notices.pl                                 | 27 ++++++--------
 t/db_dependent/Letters.t                           | 13 ++++---
 4 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/C4/Letters.pm b/C4/Letters.pm
index 7929506..f0d7aa3 100644
--- a/C4/Letters.pm
+++ b/C4/Letters.pm
@@ -1060,12 +1060,29 @@ sub GetMessageTransportTypes {
     return $mtts;
 }
 
+=head2 GetMessage
+
+    my $message = C4::Letters::Message($message_id);
+
+=cut
+
+sub GetMessage {
+    my ( $message_id ) = @_;
+    return unless $message_id;
+    my $dbh = C4::Context->dbh;
+    return $dbh->selectrow_hashref(q|
+        SELECT message_id, borrowernumber, subject, content, metadata, letter_code, message_transport_type, status, time_queued, to_address, from_address, content_type
+        FROM message_queue
+        WHERE message_id = ?
+    |, {}, $message_id );
+}
+
 =head2 ResendMessage
 
-  Attempt to resend a message.
-  
-  my $message_id = C4::Letters::ResendMessage(123);
-  
+  Attempt to resend a message which has failed previously.
+
+  my $has_been_resent = C4::Letters::ResendMessage($message_id);
+
   Updates the message to 'pending' status so that
   it will be resent later on.
 
@@ -1075,11 +1092,17 @@ sub GetMessageTransportTypes {
 
 sub ResendMessage {
     my $message_id = shift;
-    
-    return ((C4::Letters::_set_message_status( {
-                message_id => $message_id,
-                status => 'pending',
-         } ) > 0) ? 1:0);
+    return unless $message_id;
+
+    my $message = GetMessage( $message_id );
+    return unless $message;
+    if ( $message->{status} eq 'failed' ) {
+        return ((C4::Letters::_set_message_status( {
+                    message_id => $message_id,
+                    status => 'pending',
+             } ) > 0) ? 1:0);
+    }
+    return 0;
 }
 
 =head2 _add_attachements
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 d46438e..189209c 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
@@ -70,7 +70,7 @@
 		<td>
             [% IF ( QUEUED_MESSAGE.status == 'sent' ) %]sent
             [% ELSIF ( QUEUED_MESSAGE.status == 'pending' ) %]pending
-            [% ELSIF ( QUEUED_MESSAGE.status == 'failed' ) %]failed <div class="notice"><a href="/cgi-bin/koha/members/notices.pl?borrowernumber=[% borrowernumber %]&resendnotice=[% QUEUED_MESSAGE.message_id %]" title="Attempt to resend the notice">Resend</a></div>
+            [% ELSIF ( QUEUED_MESSAGE.status == 'failed' ) %]failed <div class="notice"><a href="/cgi-bin/koha/members/notices.pl?borrowernumber=[% borrowernumber %]&amp;op=resend_notice&amp;message_id=[% QUEUED_MESSAGE.message_id %]" title="Attempt to resend the notice">Resend</a></div>
             [% ELSIF ( QUEUED_MESSAGE.status == 'deleted' ) %]deleted
             [% ELSE %][% QUEUED_MESSAGE.status %][% END %]
         </td>
diff --git a/members/notices.pl b/members/notices.pl
index dc021ed..2fdbe88 100755
--- a/members/notices.pl
+++ b/members/notices.pl
@@ -46,29 +46,24 @@ my ($template, $loggedinuser, $cookie)
 				debug => 1,
 				});
 
+my $op = $input->param('op') || q||;
+
 $template->param( $borrower );
 my ($picture, $dberror) = GetPatronImage($borrower->{'borrowernumber'});
 $template->param( picture => 1 ) if $picture;
 
-# Getting the messages
-my $queued_messages = C4::Letters::GetQueuedMessages({borrowernumber => $borrowernumber});
-
-# Bug 12426 - Allow resending of messages in Notices tab
-if ($input->param('resendnotice')) {
-    foreach my $message (@$queued_messages){
-        # resendnotice must be in this borrower's queue - we don't want to make it
-        # possible to change any message just by changing resendnotice id.
-        if ($message->{message_id} == $input->param('resendnotice')) {
-            # We also only want to resend messages in failed status
-            last unless $message->{status} eq "failed";
-
-            # Modify the message in $queued_message to have its new pending status
-            $message->{status} = 'pending' if (C4::Letters::ResendMessage($message->{message_id}));
-            last;
-        }
+# Allow resending of messages in Notices tab
+if ( $op eq 'resend_notice' ) {
+    my $message_id = $input->param('message_id');
+    my $message = C4::Letters::GetMessage( $message_id );
+    if ( $message->{borrowernumber} = $borrowernumber ) {
+        C4::Letters::ResendMessage( $message_id );
     }
 }
 
+# Getting the messages
+my $queued_messages = C4::Letters::GetQueuedMessages({borrowernumber => $borrowernumber});
+
 if (C4::Context->preference('ExtendedPatronAttributes')) {
     my $attributes = GetBorrowerAttributes($borrowernumber);
     $template->param(
diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t
index 49b1320..78cb458 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 => 66;
+use Test::More tests => 69;
 use Test::MockModule;
 use Test::Warn;
 
@@ -137,9 +137,14 @@ is(
 );
 
 # ResendMessage
-C4::Letters::ResendMessage($messages->[0]->{message_id});
-$messages = C4::Letters::GetQueuedMessages({ borrowernumber => $borrowernumber });
-is($messages->[0]->{status},'pending', 'ResendMessage sets status to pending correctly (bug 12426)');
+my $resent = C4::Letters::ResendMessage($messages->[0]->{message_id});
+my $message = C4::Letters::GetMessage( $messages->[0]->{message_id});
+is( $resent, 1, 'The message should have been resent' );
+is($message->{status},'pending', 'ResendMessage sets status to pending correctly (bug 12426)');
+$resent = C4::Letters::ResendMessage($messages->[0]->{message_id});
+is( $resent, 0, 'The message should not have been resent again' );
+$resent = C4::Letters::ResendMessage();
+is( $resent, undef, 'ResendMessage should return undef if not message_id given' );
 
 # GetLetters
 my $letters = C4::Letters::GetLetters();
-- 
2.1.0