Bug 18300: [QA Follow-up] Fix return value inconsistency

As noted on bug report 17669, the return values of delete (both singular
and plural), delete_missing and delete_temporary should be consistent.

Removed the if-construction around full_path. We do not need it; in the
very exceptional case that full_path would be empty, we should delete
the record since the file is missing.

Adjusted POD and unit tests accordingly.

Test plan:
Run t/db_dependent/Upload.t

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: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm
index 61e9702..b6804f2 100644 (file)
--- a/Koha/UploadedFiles.pm
+++ b/Koha/UploadedFiles.pm
@@ -115,24 +115,30 @@ Deletes all records where the actual file is not found.
 
 Supports a keep_record hash parameter to do a check only.
 
-Returns the number of missing files (and/or deleted records).
+Return value: If you pass keep_record, it returns the number of records where
+the file is not found, or 0E0. Otherwise it returns a number, 0E0 or -1 just
+as delete does.
 
 =cut
 
 sub delete_missing {
     my ( $self, $params ) = @_;
     $self = Koha::UploadedFiles->new if !ref($self); # handle class call
-    my $cnt = 0;
+    my $rv = 0;
     while( my $row = $self->next ) {
-        if( my $file = $row->full_path ) {
-            next if -e $file;
-            # We are passing keep_file since we already know that the file
-            # is missing and we do not want to see the warning
-            $row->delete({ keep_file => 1 }) if !$params->{keep_record};
-            $cnt++;
+        my $file = $row->full_path;
+        next if -e $file;
+        if( $params->{keep_record} ) {
+            $rv++;
+            next;
         }
+        # We are passing keep_file since we already know that the file
+        # is missing and we do not want to see the warning
+        # Apply the same logic as in delete for the return value
+        my $delete = $row->delete({ keep_file => 1 }); # 1, 0E0 or -1
+        $rv = ( $delete < 0 || $rv < 0 ) ? -1 : ( $rv + $delete );
     }
-    return $cnt;
+    return $rv==0 ? "0E0" : $rv;
 }
 
 =head3 search_term

diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t
index 0489fee..8d77362 100644 (file)
--- a/t/db_dependent/Upload.t
+++ b/t/db_dependent/Upload.t
@@ -193,20 +193,23 @@ subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub {
 };
 
 subtest 'Test delete_missing' => sub {
-    plan tests => 4;
+    plan tests => 5;
 
     # If we add files via TestBuilder, they do not exist
     my $upload01 = $builder->build({ source => 'UploadedFile' });
     my $upload02 = $builder->build({ source => 'UploadedFile' });
     # dry run first
     my $deleted = Koha::UploadedFiles->delete_missing({ keep_record => 1 });
-    is( $deleted, 2, 'Expect two missing files' );
+    is( $deleted, 2, 'Expect two records with missing files' );
     isnt( Koha::UploadedFiles->find( $upload01->{id} ), undef, 'Not deleted' );
     $deleted = Koha::UploadedFiles->delete_missing;
-    is( $deleted, 2, 'Deleted two missing files' );
+    ok( $deleted =~ /^(2|-1)$/, 'Deleted two records with missing files' );
     is( Koha::UploadedFiles->search({
         id => [ $upload01->{id}, $upload02->{id} ],
     })->count, 0, 'Records are gone' );
+    # Repeat it
+    $deleted = Koha::UploadedFiles->delete_missing;
+    is( $deleted, "0E0", "Return value of 0E0 expected" );
 };
 
 subtest 'Call search_term with[out] private flag' => sub {