From c66637670b4fa084438925d3e9a4bfa8e0275ef2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 7 Jan 2020 12:10:05 +0100 Subject: [PATCH] Bug 21684: Fix UploadedFile[s]->delete Tests were failing with: # Failed test 'Test delete via UploadedFile as well as UploadedFiles' # at t/db_dependent/Upload.t line 193. DBIx::Class::Row::delete(): Not in database at /kohadevbox/koha/Koha/Object.pm line 219 I am not sure this patch is perfect, a set of uploaded files should be deleted in a transaction, which would be rollback if something is wrong. But it will be tricky to restore the files after they have been deleted. It seems that we should deal with that with a more complicated process and should be part of a separate bug. Signed-off-by: Martin Renvoize --- Koha/UploadedFile.pm | 7 ------- Koha/UploadedFiles.pm | 8 ++++---- t/db_dependent/Upload.t | 21 ++++++++++++--------- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/Koha/UploadedFile.pm b/Koha/UploadedFile.pm index 20fc608c68..bc7d4a240f 100644 --- a/Koha/UploadedFile.pm +++ b/Koha/UploadedFile.pm @@ -75,13 +75,6 @@ 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 ) { diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm index b6804f299b..d7023ce02d 100644 --- a/Koha/UploadedFiles.pm +++ b/Koha/UploadedFiles.pm @@ -72,8 +72,8 @@ sub delete { # We use the individual delete on each resultset record my $rv = 0; while( my $row = $self->next ) { - my $delete= $row->delete( $params ); # 1, 0E0 or -1 - $rv = ( $delete < 0 || $rv < 0 ) ? -1 : ( $rv + $delete ); + my $deleted = eval { $row->delete( $params ) }; + $rv++ if $deleted && !$@; } return $rv==0 ? "0E0" : $rv; } @@ -135,8 +135,8 @@ sub delete_missing { # 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 ); + my $deleted = eval { $row->delete({ keep_file => 1 }) }; + $rv++ if $deleted && !$@; } return $rv==0 ? "0E0" : $rv; } diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t index 802dc58da0..23a50caa01 100644 --- a/t/db_dependent/Upload.t +++ b/t/db_dependent/Upload.t @@ -4,6 +4,7 @@ use Modern::Perl; use File::Temp qw/ tempdir /; use Test::More tests => 13; use Test::Warn; +use Try::Tiny; use Test::MockModule; use t::lib::Mocks; @@ -171,7 +172,7 @@ subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub { my $kohaobj = Koha::UploadedFiles->find( $upl->result ); $path = $kohaobj->full_path; $delete = $kohaobj->delete; - ok( $delete=~/^-?1$/, 'Delete successful' ); + ok( $delete, 'Delete successful' ); isnt( -e $path, 1, 'File no longer found after delete' ); # add another record with TestBuilder, so file does not exist @@ -180,16 +181,18 @@ subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub { 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' ); + ok( $delete, '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' }); $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?) + try { + $delete = $kohaobj->delete({ keep_file => 1 }); + } catch { + ok( $_->isa("DBIx::Class::Exception"), 'Repeated delete unsuccessful' ); + } }; subtest 'Test delete_missing' => sub { @@ -203,7 +206,7 @@ subtest 'Test delete_missing' => sub { is( $deleted, 2, 'Expect two records with missing files' ); isnt( Koha::UploadedFiles->find( $upload01->{id} ), undef, 'Not deleted' ); $deleted = Koha::UploadedFiles->delete_missing; - ok( $deleted =~ /^(2|-1)$/, 'Deleted two records with missing files' ); + ok( $deleted =~ /^(2)$/, 'Deleted two records with missing files' ); is( Koha::UploadedFiles->search({ id => [ $upload01->{id}, $upload02->{id} ], })->count, 0, 'Records are gone' ); @@ -300,17 +303,17 @@ subtest 'Testing delete_temporary' => sub { # Now call delete_temporary with 6, 5 and 0 t::lib::Mocks::mock_preference('UploadPurgeTemporaryFilesDays', 6 ); my $delete = Koha::UploadedFiles->delete_temporary; - ok( $delete =~ /^(-1|0E0)$/, 'Check return value with 6' ); + is( $delete, '0E0', 'Check return value with 6' ); is( Koha::UploadedFiles->search->count, 6, 'Delete with pref==6' ); # use override parameter $delete = Koha::UploadedFiles->delete_temporary({ override_pref => 5 }); - ok( $delete =~ /^(2|-1)$/, 'Check return value with 5' ); + is( $delete, 2, 'Check return value with 5' ); is( Koha::UploadedFiles->search->count, 4, 'Delete with override==5' ); t::lib::Mocks::mock_preference('UploadPurgeTemporaryFilesDays', 0 ); $delete = Koha::UploadedFiles->delete_temporary; - ok( $delete =~ /^(-1|1)$/, 'Check return value with 0' ); + is( $delete, 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