From 884b08bb5da374b6add38a9421ab24674064234b 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 Content-Type: text/plain; charset=utf-8 See Bugzilla comment36 (QA request). Koha::UploadedFile->delete Returns number of deleted records. POD corrected. Handles a possible or fictive undef from the storage backend. 0E0 means zero (like DBI), -1 means unknown. Koha::UploadedFile->delete Returns number of deleted records (just as above), and no longer true or false depending on errors encountered. POD corrected. Koha::UploadedFile->delete_temporary Returns number of deleted records (just as above). Does no longer return true when the associated pref contains NULL or empty string. POD corrected. Signed-off-by: Marcel de Rooy As often in Koha, the return values of delete operations are ignored. The test t/db_dependent/Upload.t still passes. --- Koha/UploadedFile.pm | 8 +++++--- Koha/UploadedFiles.pm | 16 ++++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/Koha/UploadedFile.pm b/Koha/UploadedFile.pm index 160c2d3..aadc067 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). Note that this depends on the +storage backend. A value of -1 may represent 'unknown'. And keep in mind that +a deleted record does not automatically imply a deleted file. A warning may +have been raised. (TODO: Use exceptions.) =cut @@ -71,14 +74,13 @@ sub delete { my $name = $self->filename; my $file = $self->full_path; - my $retval = $self->SUPER::delete; + my $retval = $self->SUPER::delete // "0E0"; # translate (fictive) undef 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 b75aee8..4842d55 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 (depending on storage backend). An +unknown value may be represented as -1, zero as 0E0. (Note: 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 ) // "0E0"; # handle fictive undef + $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"; # in analogy with DBI (passed thru by DBIx) } my $dt = dt_from_string(); $dt->subtract( days => $days ); -- 2.1.4