From 1976ec2020a0a17a7b73ac3acac491532ef3e66d Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Wed, 7 May 2014 15:55:54 +0200
Subject: [PATCH] Bug 11742: Change return type for GetLetters

The GetLetters subroutine should return an arrayref with different
letters for a module.

Test plan:
0/ Delete your notices with module=claimacquisition, claimissues,
serial
1/ Go on the late orders page (acqui/lateorders.pl) and verify you
cannot choose a notice for claiming
2/ Create a notice with module=claimacquisition
3/ Go on the late orders page (acqui/lateorders.pl) and verify you
can choose the notice for claiming
4/ Go on the Claim serials page (serials/claims.pl) and repeat the same
thing with the a "claimissues" notice
5/ Create a new subscription (serials/subscription-add.pl) and verify
you cannot choose a notification for patrons.
6/ Create a notice with module "serial" and verify you can.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script. Additional tests done:

- copy notice ODUE, on saving you are now prompted to choose
  a new CODE for the notice
- edit new notice, try to set code back to ODUE. You are
  prompted that the code is already in use.

This will prevent people from accidentally overwriting a letter
with the same letter code.
---
 C4/Letters.pm                                 | 62 +++++--------------
 acqui/lateorders.pl                           |  8 +--
 .../prog/en/modules/serials/claims.tt         |  6 +-
 serials/claims.pl                             | 12 ++--
 serials/subscription-add.pl                   | 38 +++++++-----
 t/Letters.t                                   |  4 +-
 tools/overduerules.pl                         | 46 ++------------
 7 files changed, 57 insertions(+), 119 deletions(-)

diff --git a/C4/Letters.pm b/C4/Letters.pm
index 055c8c143c..4e0ea20367 100644
--- a/C4/Letters.pm
+++ b/C4/Letters.pm
@@ -62,58 +62,30 @@ C4::Letters - Give functions for Letters management
 
   Letters are managed through "alerts" sent by Koha on some events. All "alert" related functions are in this module too.
 
-=head2 GetLetters([$category])
+=head2 GetLetters([$module])
 
-  $letters = &GetLetters($category);
+  $letters = &GetLetters($module);
   returns informations about letters.
-  if needed, $category filters for letters given category
-  Create a letter selector with the following code
-
-=head3 in PERL SCRIPT
-
-my $letters = GetLetters($cat);
-my @letterloop;
-foreach my $thisletter (keys %$letters) {
-    my $selected = 1 if $thisletter eq $letter;
-    my %row =(
-        value => $thisletter,
-        selected => $selected,
-        lettername => $letters->{$thisletter},
-    );
-    push @letterloop, \%row;
-}
-$template->param(LETTERLOOP => \@letterloop);
-
-=head3 in TEMPLATE
-
-    <select name="letter">
-        <option value="">Default</option>
-    <!-- TMPL_LOOP name="LETTERLOOP" -->
-        <option value="<!-- TMPL_VAR name="value" -->" <!-- TMPL_IF name="selected" -->selected<!-- /TMPL_IF -->><!-- TMPL_VAR name="lettername" --></option>
-    <!-- /TMPL_LOOP -->
-    </select>
+  if needed, $module filters for letters given module
 
 =cut
 
 sub GetLetters {
+    my ($filters) = @_;
+    my $module    = $filters->{module};
+    my $dbh       = C4::Context->dbh;
+    my $letters   = $dbh->selectall_arrayref(
+        q|
+            SELECT module, code, branchcode, name
+            FROM letter
+            WHERE 1
+        |
+          . ( $module ? q| AND module = ?| : q|| )
+          . q| GROUP BY code ORDER BY name|, { Slice => {} }
+        , ( $module ? $module : () )
+    );
 
-    # returns a reference to a hash of references to ALL letters...
-    my ( $cat ) = @_;
-    my %letters;
-    my $dbh = C4::Context->dbh;
-    my $sth;
-    my $query = q{
-        SELECT * FROM letter WHERE 1
-    };
-    $query .= q{ AND module = ? } if defined $cat;
-    $query .= q{ GROUP BY code ORDER BY name};
-    $sth = $dbh->prepare($query);
-    $sth->execute((defined $cat ? $cat : ()));
-
-    while ( my $letter = $sth->fetchrow_hashref ) {
-        $letters{ $letter->{'code'} } = $letter->{'name'};
-    }
-    return \%letters;
+    return $letters;
 }
 
 # FIXME: using our here means that a Plack server will need to be
diff --git a/acqui/lateorders.pl b/acqui/lateorders.pl
index 1a64a4cb72..3c96e11691 100755
--- a/acqui/lateorders.pl
+++ b/acqui/lateorders.pl
@@ -159,17 +159,13 @@ foreach (@lateorders){
 	$total += $_->{subtotal};
 }
 
-my @letters;
-my $letters=GetLetters("claimacquisition");
-foreach (keys %$letters){
-	push @letters, {code=>$_,name=>$letters->{$_}};
-}
-$template->param(letters=>\@letters) if (@letters);
+my $letters = GetLetters({ module => "claimacquisition" });
 
 $template->param(ERROR_LOOP => \@errors) if (@errors);
 $template->param(
 	lateorders => \@lateorders,
 	delay => $delay,
+    letters => $letters,
     estimateddeliverydatefrom => $estimateddeliverydatefrom,
     estimateddeliverydateto   => $estimateddeliverydateto,
 	total => $total,
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt
index 34c6d8cfe0..de9102e22d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt
@@ -177,12 +177,12 @@
 
     <h1>Claims</h1>
 
-[% IF ( letter ) %][% UNLESS ( missingissues ) %][% IF ( supplierid ) %] <div class="dialog alert">No missing issues found.</div>[% ELSE %]<div class="dialog message">Please choose a vendor.</div>[% END %][% END %][% END %]
+[% IF letters %][% UNLESS ( missingissues ) %][% IF ( supplierid ) %] <div class="dialog alert">No missing issues found.</div>[% ELSE %]<div class="dialog message">Please choose a vendor.</div>[% END %][% END %][% END %]
 	
 	     [% IF ( SHOWCONFIRMATION ) %]
      <div class="dialog alert">Your notification has been sent.</div>
      [% END %]
-[% UNLESS ( letter ) %]<div class="dialog alert">No claims notice defined. <a href="/cgi-bin/koha/tools/letter.pl">Please define one</a>.</div>[% END %]
+[% UNLESS letters %]<div class="dialog alert">No claims notice defined. <a href="/cgi-bin/koha/tools/letter.pl">Please define one</a>.</div>[% END %]
     <form id="claims" name="claims" action="claims.pl" method="post">
     <fieldset>
 	    <label for="supplierid">Vendor: </label>
@@ -331,7 +331,7 @@
             <span class="exportSelected"><a id="ExportSelected" href="/cgi-bin/koha/serials/claims.pl">Download selected claims</a></span>
         [% END %]
 
-[% IF ( letter ) %]
+[% IF ( letters ) %]
         <fieldset class="action"> <label for="letter_code">Select notice:</label>
             <select name="letter_code" id="letter_code">
                 [% FOREACH letter IN letters %]
diff --git a/serials/claims.pl b/serials/claims.pl
index c826804add..13167d727f 100755
--- a/serials/claims.pl
+++ b/serials/claims.pl
@@ -58,14 +58,9 @@ for my $s (@{$supplierlist} ) {
     }
 }
 
-my $letters = GetLetters('claimissues');
-my @letters;
-foreach (keys %{$letters}){
-    push @letters ,{code=>$_,name=> $letters->{$_}};
-}
+my $letters = GetLetters({ module => 'claimissues' });
 
-my $letter=((scalar(@letters)>1) || ($letters[0]->{name}||$letters[0]->{code}));
-my  @missingissues;
+my @missingissues;
 my @supplierinfo;
 if ($supplierid) {
     @missingissues = GetLateOrMissingIssues($supplierid,$serialid,$order);
@@ -85,7 +80,7 @@ if($op && $op eq 'preview'){
         ### $cntupdate SHOULD be equal to scalar(@$serialnums)
     }
 }
-$template->param('letters'=>\@letters,'letter'=>$letter);
+
 $template->param(
         order =>$order,
         suploop => $supplierlist,
@@ -99,6 +94,7 @@ $template->param(
         supplierloop => \@supplierinfo,
         branchloop   => $branchloop,
         csv_profiles => C4::Csv::GetCsvProfiles( "sql" ),
+        letters => $letters,
         (uc(C4::Context->preference("marcflavour"))) => 1
         );
 output_html_with_http_headers $input, $cookie, $template->output;
diff --git a/serials/subscription-add.pl b/serials/subscription-add.pl
index e31b8f27da..b17fc608d0 100755
--- a/serials/subscription-add.pl
+++ b/serials/subscription-add.pl
@@ -88,7 +88,6 @@ if ($op eq 'modify' || $op eq 'dup' || $op eq 'modsubscription') {
       if (!defined $subs->{letter}) {
           $subs->{letter}= q{};
       }
-    letter_loop($subs->{'letter'}, $template);
     my $nextexpected = GetNextExpected($subscriptionid);
     $nextexpected->{'isfirstissue'} = $nextexpected->{planneddate} eq $firstissuedate ;
     $subs->{nextacquidate} = $nextexpected->{planneddate}  if($op eq 'modify');
@@ -123,6 +122,10 @@ if ($op eq 'modify' || $op eq 'dup' || $op eq 'modsubscription') {
         my @fields_id = map { fieldid => $_ }, split '\|', $dont_copy_fields;
         $template->param( dont_export_field_loop => \@fields_id );
     }
+
+    my $letters = get_letter_loop( $subs->{letter} );
+    $template->param( letterloop => $letters );
+
 }
 
 my $onlymine =
@@ -152,7 +155,8 @@ $template->param(branchloop => $branchloop,
 );
 # prepare template variables common to all $op conditions:
 if ($op!~/^mod/) {
-    letter_loop(q{}, $template);
+    my $letters = get_letter_loop();
+    $template->param( letterloop => $letters );
 }
 
 if ($op eq 'addsubscription') {
@@ -172,7 +176,10 @@ if ($op eq 'addsubscription') {
         }
     $template->param(subtype => \@sub_type_data);
 
-    letter_loop( '', $template ) if ($op ne 'modsubscription' && $op ne 'dup' && $op ne 'modify');
+    if ( $op ne 'modsubscription' && $op ne 'dup' && $op ne 'modify' ) {
+        my $letters = get_letter_loop();
+        $template->param( letterloop => $letters );
+    }
 
     my $new_biblionumber = $query->param('biblionumber_for_new_subscription');
     if (defined $new_biblionumber) {
@@ -225,19 +232,18 @@ if ($op eq 'addsubscription') {
     output_html_with_http_headers $query, $cookie, $template->output;
 }
 
-sub letter_loop {
-    my ($selected_letter, $templte) = @_;
-    my $letters = GetLetters('serial');
-    my $letterloop;
-    foreach my $thisletter (keys %{$letters}) {
-        push @{$letterloop}, {
-            value => $thisletter,
-            selected => $thisletter eq $selected_letter,
-            lettername => $letters->{$thisletter},
-        };
-    }
-    $templte->param(letterloop => $letterloop);
-    return;
+sub get_letter_loop {
+    my ($selected_lettercode) = @_;
+    my $letters = GetLetters({ module => 'serial' });
+    return [
+        map {
+            {
+                value      => $_->{code},
+                lettername => $_->{name},
+                ( $_->{code} eq $selected_lettercode ? ( selected => 1 ) : () ),
+            }
+          } @$letters
+    ];
 }
 
 sub _get_sub_length {
diff --git a/t/Letters.t b/t/Letters.t
index 7cbf6c8e86..0dc4ff5bda 100755
--- a/t/Letters.t
+++ b/t/Letters.t
@@ -31,7 +31,9 @@ $dbh->{mock_add_resultset} = $mock_letters;
 
 my $letters = C4::Letters::GetLetters();
 
-is( $letters->{ISBN}, 'book', 'HASH ref of ISBN is book' );
+my ( $ISBN_letter ) = grep {$_->{code} eq 'ISBN'} @$letters;
+is( $ISBN_letter->{name}, 'book', 'letter name for "ISBN" letter is book' );
+is( scalar( @$letters ), 2, 'GetLetters returns the 2 inserted letters' );
 
 # Regression test for bug 10843
 # $dt->add takes a scalar, not undef
diff --git a/tools/overduerules.pl b/tools/overduerules.pl
index 810b1fd9e4..11dd9bd0bc 100755
--- a/tools/overduerules.pl
+++ b/tools/overduerules.pl
@@ -201,9 +201,7 @@ if ($op eq 'save') {
 }
 my $branchloop = GetBranchesLoop($branch);
 
-my $letters = GetLetters("circulation");
-
-my $countletters = keys %{$letters};
+my $letters = GetLetters({ module => "circulation" });
 
 my @line_loop;
 
@@ -222,25 +220,7 @@ for my $data (@categories) {
             );
             $row{delay}=$temphash{$data->{'categorycode'}}->{"delay$i"};
             $row{debarred}=$temphash{$data->{'categorycode'}}->{"debarred$i"};
-            if ($countletters){
-                my @letterloop;
-                foreach my $thisletter (sort { $letters->{$a} cmp $letters->{$b} } keys %$letters) {
-                    my $selected;
-                    if ( $temphash{$data->{categorycode}}->{"letter$i"} &&
-                        $thisletter eq $temphash{$data->{'categorycode'}}->{"letter$i"}) {
-                        $selected = 1;
-                    }
-                    my %letterrow =(value => $thisletter,
-                                    selected => $selected,
-                                    lettername => $letters->{$thisletter},
-                                    );
-                    push @letterloop, \%letterrow;
-                }
-                $row{letterloop}=\@letterloop;
-            } else {
-                $row{noletter}=1;
-                $row{letter}=$temphash{$data->{'categorycode'}}->{"letter$i"};
-            }
+            $row{selected_lettercode} = $temphash{ $data->{categorycode} }->{"letter$i"};
             my @selected_mtts = @{ GetOverdueMessageTransportTypes( $branch, $data->{'categorycode'}, $i) };
             my @mtts;
             for my $mtt ( @$message_transport_types ) {
@@ -268,24 +248,9 @@ for my $data (@categories) {
                 overduename => $data->{'categorycode'},
                 line        => $data->{'description'}
             );
-            if ($countletters){
-                my @letterloop;
-                foreach my $thisletter (sort { $letters->{$a} cmp $letters->{$b} } keys %$letters) {
-                    my $selected;
-                    if ($dat->{"letter$i"} && $thisletter eq $dat->{"letter$i"}) {
-                        $selected = 1;
-                    }
-                    my %letterrow =(value => $thisletter,
-                                    selected => $selected,
-                                    lettername => $letters->{$thisletter},
-                                    );
-                    push @letterloop, \%letterrow;
-                }
-                $row{letterloop}=\@letterloop;
-            } else {
-                $row{noletter}=1;
-                if ($dat->{"letter$i"}){$row{letter}=$dat->{"letter$i"};}
-            }
+
+            $row{selected_lettercode} = $dat->{"letter$i"};
+
             if ($dat->{"delay$i"}){$row{delay}=$dat->{"delay$i"};}
             if ($dat->{"debarred$i"}){$row{debarred}=$dat->{"debarred$i"};}
             my @selected_mtts = @{ GetOverdueMessageTransportTypes( $branch, $data->{'categorycode'}, $i) };
@@ -333,5 +298,6 @@ $template->param(
     branch => $branch,
     tabs => \@tabs,
     message_transport_types => $message_transport_types,
+    letters => $letters,
 );
 output_html_with_http_headers $input, $cookie, $template->output;
-- 
2.39.5