From e429db4746f4ea53abe4e271afd6adb43b54c5d8 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 10 Jan 2017 17:11:08 +0100 Subject: [PATCH] Bug 17501: [Follow-up] QA Requests This patch makes the following changes, as requested by QA: [1] UploadedFile->delete always calls SUPER::delete. The return value normally comes from SUPER::delete; if removing the file failed, we return false. Two warns are kept. Since delete does no longer return the filename, a few changes were needed in tools/upload.pl. [2] Method getCategories is moved to UploadedFiles. Script tools/upload.pl now only contains one call. Added a use C4::Koha. [3] Calls UploadedFiles->delete as class method. As a result I removed method delete_errors for now; may be reconsidered on a new report. [4] Adjusted three ->search calls for id and public to ->find calls. [5] If you pass no id to upload.pl when deleting, you don't get an alert. All by all, we got rid of 15 lines ! Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall --- Koha/UploadedFile.pm | 27 +++++++-------------------- Koha/UploadedFiles.pm | 26 ++++++++++++++++---------- t/db_dependent/Upload.t | 7 +++---- tools/upload.pl | 35 ++++++++++++++--------------------- 4 files changed, 40 insertions(+), 55 deletions(-) diff --git a/Koha/UploadedFile.pm b/Koha/UploadedFile.pm index de696f3411..160c2d349f 100644 --- a/Koha/UploadedFile.pm +++ b/Koha/UploadedFile.pm @@ -71,18 +71,17 @@ sub delete { my $name = $self->filename; my $file = $self->full_path; - if( $params->{keep_file} ) { - return $name if $self->SUPER::delete; - } elsif( !-e $file ) { # we will just delete the record + my $retval = $self->SUPER::delete; + return $retval if $params->{keep_file}; + + if( ! -e $file ) { warn "Removing record for $name within category ". $self->uploadcategorycode. ", but file was missing."; - return $name if $self->SUPER::delete; - } elsif( unlink($file) ) { - return $name if $self->SUPER::delete; - } else { + } elsif( ! unlink($file) ) { + $retval = 0; warn "Problem while deleting: $file"; } - return; # something went wrong + return $retval; } =head3 full_path @@ -156,18 +155,6 @@ sub temporary_directory { return File::Spec->tmpdir; } -=head3 getCategories - -getCategories returns a list of upload category codes and names - -=cut - -sub getCategories { - my ( $class ) = @_; - my $cats = C4::Koha::GetAuthorisedValues('UPLOAD'); - [ map {{ code => $_->{authorised_value}, name => $_->{lib} }} @$cats ]; -} - =head3 _type Returns name of corresponding DBIC resultset diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm index e970fbcb4b..109593d31a 100644 --- a/Koha/UploadedFiles.pm +++ b/Koha/UploadedFiles.pm @@ -19,6 +19,7 @@ package Koha::UploadedFiles; use Modern::Perl; +use C4::Koha; use Koha::UploadedFile; use parent qw(Koha::Objects); @@ -38,7 +39,7 @@ Koha::UploadedFiles - Koha::Objects class for uploaded files my @uploads = Koha::UploadedFiles->search_term({ term => '.mrc' }); # delete all uploads - Koha::UploadedFiles->new->delete; + Koha::UploadedFiles->delete; =head1 DESCRIPTION @@ -51,11 +52,10 @@ provides a wrapper around search to look for a term in multiple columns. =head2 INSTANCE METHODS -=head3 delete, delete_errors +=head3 delete Delete uploaded files. -Returns true if no errors occur. -Delete_errors returns the number of errors when deleting 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. @@ -69,15 +69,9 @@ sub delete { my $kohaobj = Koha::UploadedFile->_new_from_dbic( $row ); $err++ if !$kohaobj->delete( $params ); } - $self->{delete_errors} = $err; return $err==0; } -sub delete_errors { - my ( $self ) = @_; - return $self->{delete_errors}; -} - =head3 search_term Search_term allows you to pass a term to search in filename and hashvalue. @@ -103,6 +97,18 @@ sub search_term { =head2 CLASS METHODS +=head3 getCategories + +getCategories returns a list of upload category codes and names + +=cut + +sub getCategories { + my ( $class ) = @_; + my $cats = C4::Koha::GetAuthorisedValues('UPLOAD'); + [ map {{ code => $_->{authorised_value}, name => $_->{lib} }} @$cats ]; +} + =head3 _type Returns name of corresponding DBIC resultset diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t index 6f60b22a11..5aeb7a41bc 100644 --- a/t/db_dependent/Upload.t +++ b/t/db_dependent/Upload.t @@ -59,7 +59,7 @@ subtest 'Make a fresh start' => sub { # Passing keep_file suppresses warnings (and does not delete files) # Note that your files are not in danger, since we redirected # all files to a new empty temp folder - Koha::UploadedFiles->new->delete({ keep_file => 1 }); + Koha::UploadedFiles->delete({ keep_file => 1 }); is( Koha::UploadedFiles->count, 0, 'No records left' ); }; @@ -164,10 +164,9 @@ subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub { $upl = Koha::Uploader->new({ tmp => 1 }); $upl->cgi; my $kohaobj = Koha::UploadedFiles->find( $upl->result ); - my $name = $kohaobj->filename; $path = $kohaobj->full_path; $delete = $kohaobj->delete; - is( $delete, $name, 'Delete successful' ); + is( $delete, 1, 'Delete successful' ); isnt( -e $path, 1, 'File no longer found after delete' ); # add another record with TestBuilder, so file does not exist @@ -198,7 +197,7 @@ subtest 'Simple tests for httpheaders and getCategories' => sub { my @hdrs = $rec->httpheaders; is( @hdrs == 4 && $hdrs[1] =~ /application\/octet-stream/, 1, 'Simple test for httpheaders'); $builder->build({ source => 'AuthorisedValue', value => { category => 'UPLOAD', authorised_value => 'HAVE_AT_LEAST_ONE', lib => 'Hi there' } }); - my $cat = Koha::UploadedFile->getCategories; + my $cat = Koha::UploadedFiles->getCategories; is( @$cat >= 1, 1, 'getCategories returned at least one category' ); }; diff --git a/tools/upload.pl b/tools/upload.pl index 76f2cb64a7..fd70b13a02 100755 --- a/tools/upload.pl +++ b/tools/upload.pl @@ -23,7 +23,6 @@ use JSON; use C4::Auth; use C4::Output; -use Koha::UploadedFile; use Koha::UploadedFiles; my $input = CGI::->new; @@ -47,22 +46,20 @@ $template->param( index => $index, owner => $loggedinuser, plugin => $plugin, + uploadcategories => Koha::UploadedFiles->getCategories, ); if ( $op eq 'new' ) { $template->param( mode => 'new', - uploadcategories => Koha::UploadedFile->getCategories, ); output_html_with_http_headers $input, $cookie, $template->output; } elsif ( $op eq 'search' ) { my $uploads; if( $id ) { - my $rec = Koha::UploadedFiles->search({ - id => $id, - $plugin? ( public => 1 ) : (), - })->next; + my $rec = Koha::UploadedFiles->find( $id ); + undef $rec if $rec && $plugin && !$rec->public; push @$uploads, $rec->unblessed if $rec; } else { $uploads = Koha::UploadedFiles->search_term({ @@ -80,34 +77,30 @@ if ( $op eq 'new' ) { } elsif ( $op eq 'delete' ) { # delete only takes the id parameter - my $upload = $plugin? - Koha::UploadedFiles->search({ public => 1, id => $id })->next: - Koha::UploadedFiles->find($id); - my $fn = $upload? $upload->delete: undef; + my $rec = Koha::UploadedFiles->find($id); + undef $rec if $rec && $plugin && !$rec->public; + my $fn = $rec ? $rec->filename : ''; + my $delete = $rec ? $rec->delete : undef; #TODO Improve error handling - my $msg = $fn? - JSON::to_json({ $fn => 6 }): - JSON::to_json({ - $upload? $upload->filename: ( $id? "id $id": '[No id]' ), 7, - }); + my $msg = $delete + ? JSON::to_json({ $fn => 6 }) + : $id + ? JSON::to_json({ $fn || $id, 7 }) + : ''; $template->param( mode => 'deleted', msg => $msg, - uploadcategories => Koha::UploadedFile->getCategories, ); output_html_with_http_headers $input, $cookie, $template->output; } elsif ( $op eq 'download' ) { - my $rec = Koha::UploadedFiles->search({ - id => $id, - $plugin? ( public => 1 ) : (), - })->next; + my $rec = Koha::UploadedFiles->find( $id ); + undef $rec if $rec && $plugin && !$rec->public; my $fh = $rec? $rec->file_handle: undef; if ( !$rec || !$fh ) { $template->param( mode => 'new', msg => JSON::to_json( { $id => 5 } ), - uploadcategories => Koha::UploadedFile->getCategories, ); output_html_with_http_headers $input, $cookie, $template->output; } else { -- 2.39.5