From b8e404700aa8c31dc49f01416a64c2a2dfd54eb0 Mon Sep 17 00:00:00 2001
From: Galen Charlton <galen.charlton@liblime.com>
Date: Tue, 1 Jul 2008 11:53:23 -0500
Subject: [PATCH] bug 2293: better validation in overdue rules editor

If a rule should not be saved because a delay is
given but no letter or debar action is specified
for that delay, an error message is now reported.

Also:

* use "dialog alert" CSS class for confirmation
  that form was saved
* if form input is saved, retrieve the settings from the
  database again when displaying the form - this will
  help turn up any bugs where the script is not
  saving the overdue rules correctly.
* fix display of patron category in error messages

Signed-off-by: Joshua Ferraro <jmf@liblime.com>
---
 .../prog/en/modules/tools/overduerules.tmpl   |  9 +++--
 tools/overduerules.pl                         | 34 +++++++++++++++----
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tmpl b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tmpl
index 9687266d7b..dae1ca0fac 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tmpl
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tmpl
@@ -52,10 +52,13 @@ $(document).ready(function() {
                 <p>The following fields have a forbidden value. Correct them and press OK again :</p>
                 <ul>
                 <!-- TMPL_IF NAME="ERRORDELAY" -->
-                    <li>Delay <!--TMPL_VAR Name="ERRORDELAY"--> for <!--TMPL_VAR Name="bor"--> borrower category has some unexpected characters. There should be only numerical characters. </li>
+                    <li>Delay <!--TMPL_VAR Name="ERRORDELAY"--> for <!--TMPL_VAR Name="BORERR"--> borrower category has some unexpected characters. There should be only numerical characters. </li>
+                <!-- /TMPL_IF -->
+                <!-- TMPL_IF NAME="ERRORUSELESSDELAY" -->
+                    <li>No letter or debar action specified for delay <!--TMPL_VAR Name="ERRORUSELESSDELAY"--> for <!--TMPL_VAR Name="BORERR"--> borrower category.  If a delay is supplied, either a letter, debar action, or both should be specified.</li>
                 <!-- /TMPL_IF -->
                 <!-- TMPL_IF NAME="ERRORORDER" -->
-                    <li>Delay1 should be less than Delay2 which should be less than Delay3 for <!--TMPL_VAR Name="bor"--> borrower category </li>
+                    <li>Delay1 should be less than Delay2 which should be less than Delay3 for <!--TMPL_VAR Name="BORERR"--> borrower category </li>
                 <!-- /TMPL_IF -->
                 </ul>
             </div>
@@ -67,7 +70,7 @@ $(document).ready(function() {
                 <table>
                     <caption>Rules for overdue actions: <!--TMPL_IF Name="branch"--><!-- TMPL_VAR NAME="branch" --><!--TMPL_ELSE--> default library <!--/TMPL_IF-->
                     <!--TMPL_IF Name="datasaved"-->
-                        <br /><h3 align="center">INPUT SAVED</h3>
+                        <br /><div class="dialog message">INPUT SAVED</div>
                     <!--/TMPL_IF -->
                     </caption>
                     <tr>
diff --git a/tools/overduerules.pl b/tools/overduerules.pl
index 82a8ea3183..03fee7b579 100755
--- a/tools/overduerules.pl
+++ b/tools/overduerules.pl
@@ -25,6 +25,7 @@ use C4::Auth;
 use C4::Koha;
 use C4::Branch; # GetBranches
 use C4::Letters;
+use C4::Members;
 
 my $input = new CGI;
 my $dbh = C4::Context->dbh;
@@ -48,6 +49,7 @@ my $err=0;
 
 # save the values entered into tables
 my %temphash;
+my $input_saved = 0;
 if ($op eq 'save') {
     my @names=$input->param();
     my $sth_search = $dbh->prepare("SELECT count(*) AS total FROM overduerules WHERE branchcode=? AND categorycode=?");
@@ -65,21 +67,34 @@ if ($op eq 'save') {
             }
     }
     foreach my $bor (keys %temphash){
+        # get category name if we need it for an error message
+        my $bor_category = GetBorrowercategory($bor);
+        my $bor_category_name = defined($bor_category) ? $bor_category->{description} : $bor;
+
         # Do some Checking here : delay1 < delay2 <delay3 all of them being numbers
         # Raise error if not true
         if ($temphash{$bor}->{delay1}=~/[^0-9]/ and $temphash{$bor}->{delay1} ne ""){
-            $template->param("ERROR"=>1,"ERRORDELAY"=>"delay1","BORERR"=>$bor);
+            $template->param("ERROR"=>1,"ERRORDELAY"=>"delay1","BORERR"=>$bor_category_name);
             $err=1;
         } elsif ($temphash{$bor}->{delay2}=~/[^0-9]/ and $temphash{$bor}->{delay2} ne ""){
-            $template->param("ERROR"=>1,"ERRORDELAY"=>"delay2","BORERR"=>$bor);
+            $template->param("ERROR"=>1,"ERRORDELAY"=>"delay2","BORERR"=>$bor_category_name);
             $err=1;
         } elsif ($temphash{$bor}->{delay3}=~/[^0-9]/ and $temphash{$bor}->{delay3} ne ""){
-            $template->param("ERROR"=>1,"ERRORDELAY"=>"delay3","BORERR"=>$bor);
+            $template->param("ERROR"=>1,"ERRORDELAY"=>"delay3","BORERR"=>$bor_category_name);
+            $err=1;
+        } elsif ($temphash{$bor}->{delay1} and not ($temphash{$bor}->{"letter1"} or $temphash{$bor}->{"debarred1"})) {
+            $template->param("ERROR"=>1,"ERRORUSELESSDELAY"=>"delay1","BORERR"=>$bor_category_name);
+            $err=1;
+        } elsif ($temphash{$bor}->{delay2} and not ($temphash{$bor}->{"letter2"} or $temphash{$bor}->{"debarred2"})) {
+            $template->param("ERROR"=>1,"ERRORUSELESSDELAY"=>"delay2","BORERR"=>$bor_category_name);
+            $err=1;
+        } elsif ($temphash{$bor}->{delay3} and not ($temphash{$bor}->{"letter3"} or $temphash{$bor}->{"debarred3"})) {
+            $template->param("ERROR"=>1,"ERRORUSELESSDELAY"=>"delay3","BORERR"=>$bor_category_name);
             $err=1;
         }elsif ($temphash{$bor}->{delay3} and
                 ($temphash{$bor}->{delay3}<=$temphash{$bor}->{delay2} or $temphash{$bor}->{delay3}<=$temphash{$bor}->{delay1})
                 or $temphash{$bor}->{delay2} and ($temphash{$bor}->{delay2}<=$temphash{$bor}->{delay1})){
-                    $template->param("ERROR"=>1,"ERRORORDER"=>1,"BORERR"=>$bor);
+                    $template->param("ERROR"=>1,"ERRORORDER"=>1,"BORERR"=>$bor_category_name);
                         $err=1;
         }
         unless ($err){
@@ -117,7 +132,10 @@ if ($op eq 'save') {
                 }
         }
     }
-    unless ($err) {$template->param(datasaved=>1);}
+    unless ($err) {
+        $template->param(datasaved=>1);
+        $input_saved = 1;
+    }
 }
 my $branches = GetBranches();
 my @branchloop;
@@ -149,7 +167,11 @@ while (my $data=$sth->fetchrow_hashref){
                 toggle => $toggle,
                 line => $data->{'description'}
                 );
-    if (%temphash){
+    if (%temphash and not $input_saved){
+        # if we managed to save the form submission, don't
+        # reuse %temphash, but take the values from the
+        # database - this makes it easier to identify
+        # bugs where the form submission was not correctly saved
         for (my $i=1;$i<=3;$i++){
             $row{"delay$i"}=$temphash{$data->{'categorycode'}}->{"delay$i"};
             $row{"debarred$i"}=$temphash{$data->{'categorycode'}}->{"debarred$i"};
-- 
2.20.1