Bugzilla – Attachment 61819 Details for
Bug 17669
Add purging temporary uploads to cleanup_database
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 17669: [QA Follow-up] More consistency in return values of delete
Bug-17669-QA-Follow-up-More-consistency-in-return-.patch (text/plain), 7.66 KB, created by
Marcel de Rooy
on 2017-04-04 12:45:07 UTC
(
hide
)
Description:
Bug 17669: [QA Follow-up] More consistency in return values of delete
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2017-04-04 12:45:07 UTC
Size:
7.66 KB
patch
obsolete
>From b9217e91134dc1139e59725f813dcce920d49702 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >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 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 <m.de.rooy@rijksmuseum.nl> >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. >--- > 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 160c2d3..1e62ab6 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 b75aee8..b64df0a 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 d752426..6af3fce 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.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 17669
:
58176
|
58177
|
58178
|
58179
|
58810
|
58811
|
58812
|
61239
|
61240
|
61241
|
61242
|
61243
|
61465
|
61466
|
61467
|
61468
|
61469
|
61727
|
61729
|
61730
|
61742
|
61743
|
61805
|
61809
|
61818
|
61819
|
62486
|
62487
|
62488
|
62489
|
62490
|
62491
|
62492
|
62493
|
62494
|
62495