From 15fd90dcbc98cae6febfeab59df679b761111d42 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Thu, 22 Feb 2018 11:52:14 +0100
Subject: [PATCH] Bug 19633: Use alphanumeric error codes in upload

The error codes 1 to 7 are used in Uploader.pm or tools/upload.pl.
It would be nice to use alphanumeric codes instead.
No behavior change expected.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Verify that a regular upload with tools/upload.pl still works.
[3] Rename upload_path in your koha-conf.xml. Restart Plack, flush the cache
    and try to upload to a category. Correct error message?
[4] Upload the same file twice to the same category.
    Correct error message the second time?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 19633: (QA follow-up) Really remove these ugly numbers

See BZ comment5. We now remove the numbers also from the constant names.

Test plan:
Read the changes.
Git grep "ERRCODE_"
Run t/db_dependent/Upload.t (Note: You may see one failure here; it is fixed
on bug 20727. So depends on who reaches master first.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
---
 Koha/Uploader.pm                              | 14 +++---
 .../prog/en/modules/tools/upload.tt           | 43 +++++++++++++------
 t/db_dependent/Upload.t                       |  2 +-
 tools/upload.pl                               | 10 +++--
 4 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/Koha/Uploader.pm b/Koha/Uploader.pm
index e1b34c9e0e..a4baa6a30b 100644
--- a/Koha/Uploader.pm
+++ b/Koha/Uploader.pm
@@ -58,8 +58,12 @@ Koha::Uploader - Facilitate file uploads (temporary and permanent)
 
 =cut
 
-use constant KOHA_UPLOAD => 'koha_upload';
+use constant KOHA_UPLOAD  => 'koha_upload';
 use constant BYTES_DIGEST => 2048;
+use constant ERR_EXISTS   => 'UPLERR_ALREADY_EXISTS';
+use constant ERR_PERMS    => 'UPLERR_CANNOT_WRITE';
+use constant ERR_ROOT     => 'UPLERR_NO_ROOT_DIR';
+use constant ERR_TEMP     => 'UPLERR_NO_TEMP_DIR';
 
 use Modern::Perl;
 use CGI; # no utf8 flag, since it may interfere with binary uploads
@@ -220,9 +224,9 @@ sub _create_file {
             $self->{files}->{$filename}->{errcode} ) {
         #skip
     } elsif( !$self->{temporary} && !$self->{rootdir} ) {
-        $self->{files}->{$filename}->{errcode} = 3; #no rootdir
+        $self->{files}->{$filename}->{errcode} = ERR_ROOT; #no rootdir
     } elsif( $self->{temporary} && !$self->{tmpdir} ) {
-        $self->{files}->{$filename}->{errcode} = 4; #no tempdir
+        $self->{files}->{$filename}->{errcode} = ERR_TEMP; #no tempdir
     } else {
         my $dir = $self->_dir;
         my $hashval = $self->{files}->{$filename}->{hash};
@@ -235,7 +239,7 @@ sub _create_file {
             hashvalue          => $hashval,
             uploadcategorycode => $self->{category},
         })->count ) {
-            $self->{files}->{$filename}->{errcode} = 1; #already exists
+            $self->{files}->{$filename}->{errcode} = ERR_EXISTS;
             return;
         }
 
@@ -244,7 +248,7 @@ sub _create_file {
             $fh->binmode;
             $self->{files}->{$filename}->{fh}= $fh;
         } else {
-            $self->{files}->{$filename}->{errcode} = 2; #not writable
+            $self->{files}->{$filename}->{errcode} = ERR_PERMS;
         }
     }
     return $fh;
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt
index a4e6147f6e..7972f0162b 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt
@@ -249,18 +249,6 @@
 [% MACRO jsinclude BLOCK %]
     [% Asset.js("js/tools-menu.js") %]
     [% INCLUDE 'datatables.inc' %]
-    <script type="text/javascript">
-        var errMESSAGES = [
-            "Error 0: Not in use",
-            _("This file already exists (in this category)."),
-            _("File could not be created. Check permissions."),
-            _("Your koha-conf.xml does not contain a valid upload_path."),
-            _("No temporary directory found."),
-            _("File could not be read."),
-            _("File has been deleted."),
-            _("File or upload record could not be deleted."),
-        ];
-    </script>
     [% Asset.js("js/file-upload.js") %]
     <script type="text/javascript">
         function StartUpload() {
@@ -318,13 +306,42 @@
             var str = '';
             for( var file in err ) {
                 str= str + '<p>' + file + ': ' +
-                    errMESSAGES[ err[file].code ] + '</p>';
+                    errMESSAGES( err[file].code ) + '</p>';
             }
             if( str ) {
                 $('#myalerts').html(str);
                 $('#myalerts').show();
             }
         }
+        function errMESSAGES(code) {
+            var rv;
+            switch(code) {
+                case 'UPLERR_ALREADY_EXISTS':
+                    rv = _("This file already exists (in this category).");
+                    break;
+                case 'UPLERR_CANNOT_WRITE':
+                    rv = _("File could not be created. Check permissions.");
+                    break;
+                case 'UPLERR_NO_ROOT_DIR':
+                    rv = _("Your koha-conf.xml does not contain a valid upload_path.");
+                    break;
+                case 'UPLERR_NO_TEMP_DIR':
+                    rv = _("No temporary directory found.");
+                    break;
+                case 'UPLERR_FILE_NOT_READ':
+                    rv = _("File could not be read.");
+                    break;
+                case 'UPL_FILE_DELETED': // An alert, no error
+                    rv = _("File has been deleted.");
+                    break;
+                case 'UPLERR_FILE_NOT_DELETED':
+                    rv = _("File or upload record could not be deleted.");
+                    break;
+                default:
+                    rv = code;
+            }
+            return rv;
+        }
         function CheckSearch() {
             if( $("#term").val()=="" ) {
                 alert( _("Please enter a search term.") );
diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t
index 729bb86248..802dc58da0 100644
--- a/t/db_dependent/Upload.t
+++ b/t/db_dependent/Upload.t
@@ -145,7 +145,7 @@ subtest 'Add same file in same category' => sub {
     is( $upl->count, 0, 'Upload 4 failed as expected' );
     is( $upl->result, undef, 'Result is undefined' );
     my $e = $upl->err;
-    is( $e->{file2}->{code}, 1, "Errcode 1 [already exists] reported" );
+    is( $e->{file2}->{code}, Koha::Uploader::ERR_EXISTS, "Already exists error reported" );
 };
 
 subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub {
diff --git a/tools/upload.pl b/tools/upload.pl
index 9b10b4226c..afb7c516f9 100755
--- a/tools/upload.pl
+++ b/tools/upload.pl
@@ -25,6 +25,10 @@ use C4::Auth;
 use C4::Output;
 use Koha::UploadedFiles;
 
+use constant ERR_READING     => 'UPLERR_FILE_NOT_READ';
+use constant ALERT_DELETED   => 'UPL_FILE_DELETED'; # alert, no error
+use constant ERR_NOT_DELETED => 'UPLERR_FILE_NOT_DELETED';
+
 my $input  = CGI::->new;
 my $op     = $input->param('op') // 'new';
 my $plugin = $input->param('plugin');
@@ -87,9 +91,9 @@ if ( $op eq 'new' ) {
     my $delete = $rec ? $rec->delete : undef;
     #TODO Improve error handling
     my $msg = $delete
-        ? JSON::to_json({ $fn => { code => 6 }})
+        ? JSON::to_json({ $fn => { code => ALERT_DELETED }})
         : $id
-        ? JSON::to_json({ $fn || $id, { code => 7 }})
+        ? JSON::to_json({ $fn || $id, { code => ERR_NOT_DELETED }})
         : '';
     $template->param(
         mode             => 'deleted',
@@ -104,7 +108,7 @@ if ( $op eq 'new' ) {
     if ( !$rec || !$fh ) {
         $template->param(
             mode             => 'new',
-            msg              => JSON::to_json({ $id => { code => 5 }}),
+            msg              => JSON::to_json({ $id => { code => ERR_READING }}),
         );
         output_html_with_http_headers $input, $cookie, $template->output;
     } else {
-- 
2.39.5