From e0379e83d6e3c6a8d1600d8b8351b9507a8aa79c Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 3 Apr 2017 21:43:48 +0200 Subject: [PATCH] Bug 18300: [QA Follow-up] Fix return value inconsistency Content-Type: text/plain; charset=utf-8 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 --- Koha/UploadedFiles.pm | 24 +++++++++++++++--------- t/db_dependent/Upload.t | 9 ++++++--- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm index 61e9702..b6804f2 100644 --- 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 8d91053..4399494 100644 --- 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 { -- 2.1.4