From aee0682d60ddf1f21c5802001d8af91b43102e51 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 3 Apr 2017 19:14:52 +0200 Subject: [PATCH] Bug 17669: [QA Follow-up] More consistency in return values of delete See Bugzilla comment36 (QA request). Koha::UploadedFile->delete Returns 1, 0E0 or -1 (unknown). Koha::UploadedFiles->delete and delete_temporary Returns number of deleted records, 0E0 or -1. POD lines are corrected accordingly. Unit tests adjusted too. Signed-off-by: Marcel de Rooy Added a note that Koha::Object->delete itself is not completely consistent with Koha::Objects->delete (DBIx). The singular may return 1 when it gets 0E0 from DBIx, while the plural one may return 0E0. But should be handled somewhere else. Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall --- Koha/UploadedFile.pm | 13 +++++++++++-- Koha/UploadedFiles.pm | 16 ++++++++++------ t/db_dependent/Upload.t | 28 ++++++++++++++++++++-------- 3 files changed, 41 insertions(+), 16 deletions(-) diff --git a/Koha/UploadedFile.pm b/Koha/UploadedFile.pm index 160c2d349f..1e62ab68a3 100644 --- a/Koha/UploadedFile.pm +++ b/Koha/UploadedFile.pm @@ -61,7 +61,10 @@ Delete uploaded file. It deletes not only the record, but also the actual file (unless you pass the keep_file parameter). -Returns filename on successful delete or undef. +Returns number of deleted records (1 or 0E0), or -1 for unknown. +Please keep in mind that a deleted record does not automatically imply a +deleted file; a warning may have been raised. +(TODO: Use exceptions.) =cut @@ -72,13 +75,19 @@ sub delete { my $file = $self->full_path; my $retval = $self->SUPER::delete; + if( !defined($retval) ) { # undef is Unknown (-1) + $retval = -1; + } elsif( $retval eq '0' ) { # 0 => 0E0 + $retval = "0E0"; + } elsif( $retval !~ /^(0E0|1)$/ ) { # Unknown too + $retval = -1; + } return $retval if $params->{keep_file}; if( ! -e $file ) { warn "Removing record for $name within category ". $self->uploadcategorycode. ", but file was missing."; } elsif( ! unlink($file) ) { - $retval = 0; warn "Problem while deleting: $file"; } return $retval; diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm index b75aee8927..b64df0a15a 100644 --- a/Koha/UploadedFiles.pm +++ b/Koha/UploadedFiles.pm @@ -57,21 +57,25 @@ provides a wrapper around search to look for a term in multiple columns. =head3 delete Delete uploaded files. -Returns true if no errors occur. (So, false may mean partial success.) Parameter keep_file may be used to delete records, but keep files. +Returns the number of deleted records, 0E0 or -1 (Unknown). +Please note that the number of deleted records is not automatically the same +as the number of files. + =cut sub delete { my ( $self, $params ) = @_; $self = Koha::UploadedFiles->new if !ref($self); # handle class call # We use the individual delete on each resultset record - my $err = 0; + my $rv = 0; while( my $row = $self->next ) { - $err++ if !$row->delete( $params ); + my $delete= $row->delete( $params ); # 1, 0E0 or -1 + $rv = ( $delete < 0 || $rv < 0 ) ? -1 : ( $rv + $delete ); } - return $err==0; + return $rv==0 ? "0E0" : $rv; } =head3 delete_temporary @@ -80,7 +84,7 @@ Delete_temporary is called by cleanup_database and only removes temporary uploads older than [pref UploadPurgeTemporaryFilesDays] days. It is possible to override the pref with the override_pref parameter. -Returns true if no errors occur. (Even when no files had to be deleted.) +Return value: see delete. =cut @@ -90,7 +94,7 @@ sub delete_temporary { if( exists $params->{override_pref} ) { $days = $params->{override_pref}; } elsif( !defined($days) || $days eq '' ) { # allow 0, not NULL or "" - return 1; + return "0E0"; } my $dt = dt_from_string(); $dt->subtract( days => $days ); diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t index d752426d91..6af3fce3ee 100644 --- a/t/db_dependent/Upload.t +++ b/t/db_dependent/Upload.t @@ -149,7 +149,7 @@ subtest 'Add same file in same category' => sub { }; subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub { - plan tests => 8; + plan tests => 10; # add temporary file with same name and contents (file4) my $upl = Koha::Uploader->new({ tmp => 1 }); @@ -160,7 +160,7 @@ subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub { # testing delete via UploadedFiles (plural) my $delete = Koha::UploadedFiles->search({ id => $id })->delete; - is( $delete, 1, 'Delete successful' ); + isnt( $delete, "0E0", 'Delete successful' ); isnt( -e $path, 1, 'File no longer found after delete' ); is( Koha::UploadedFiles->find( $id ), undef, 'Record also gone' ); @@ -171,16 +171,25 @@ subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub { my $kohaobj = Koha::UploadedFiles->find( $upl->result ); $path = $kohaobj->full_path; $delete = $kohaobj->delete; - is( $delete, 1, 'Delete successful' ); + ok( $delete=~/^-?1$/, 'Delete successful' ); isnt( -e $path, 1, 'File no longer found after delete' ); # add another record with TestBuilder, so file does not exist # catch warning my $upload01 = $builder->build({ source => 'UploadedFile' }); - warning_like { Koha::UploadedFiles->find( $upload01->{id} )->delete; } + warning_like { $delete = Koha::UploadedFiles->find( $upload01->{id} )->delete; } qr/file was missing/, 'delete warns when file is missing'; + ok( $delete=~/^-?1$/, 'Deleting record was successful' ); is( Koha::UploadedFiles->count, 4, 'Back to four uploads now' ); + + # add another one with TestBuilder and delete twice (file does not exist) + $upload01 = $builder->build({ source => 'UploadedFile' }); + my $kohaobj = Koha::UploadedFiles->find( $upload01->{id} ); + $delete = $kohaobj->delete({ keep_file => 1 }); + $delete = $kohaobj->delete({ keep_file => 1 }); + ok( $delete =~ /^(0E0|-1)$/, 'Repeated delete unsuccessful' ); + # NOTE: Koha::Object->delete does not return 0E0 (yet?) }; subtest 'Call search_term with[out] private flag' => sub { @@ -245,7 +254,7 @@ subtest 'Testing allows_add_by' => sub { }; subtest 'Testing delete_temporary' => sub { - plan tests => 6; + plan tests => 9; # Add two temporary files: result should be 3 + 3 Koha::Uploader->new({ tmp => 1 })->cgi; # add file6 and file7 @@ -270,15 +279,18 @@ subtest 'Testing delete_temporary' => sub { # Now call delete_temporary with 6, 5 and 0 t::lib::Mocks::mock_preference('UploadPurgeTemporaryFilesDays', 6 ); - Koha::UploadedFiles->delete_temporary; + my $delete = Koha::UploadedFiles->delete_temporary; + ok( $delete =~ /^(-1|0E0)$/, 'Check return value with 6' ); is( Koha::UploadedFiles->search->count, 6, 'Delete with pref==6' ); # use override parameter - Koha::UploadedFiles->delete_temporary({ override_pref => 5 }); + $delete = Koha::UploadedFiles->delete_temporary({ override_pref => 5 }); + ok( $delete =~ /^(2|-1)$/, 'Check return value with 5' ); is( Koha::UploadedFiles->search->count, 4, 'Delete with override==5' ); t::lib::Mocks::mock_preference('UploadPurgeTemporaryFilesDays', 0 ); - Koha::UploadedFiles->delete_temporary; + $delete = Koha::UploadedFiles->delete_temporary; + ok( $delete =~ /^(-1|1)$/, 'Check return value with 0' ); is( Koha::UploadedFiles->search->count, 3, 'Delete with pref==0 makes 3' ); is( Koha::UploadedFiles->search({ permanent => 1 })->count, 3, 'Still 3 permanent uploads' ); -- 2.39.5