From f1c4e388b56e2e56725de2d0f0d54dd2c325542a Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 21 Nov 2016 14:53:27 +0100 Subject: [PATCH] Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s] Content-Type: text/plain; charset=utf-8 Since delete is not part of the upload process, we will move it now to Koha::UploadedFile[s]. Deleting the file will be done in UploadedFile. The (multiple) delete method in UploadedFiles refers to the single delete. Test plan: [1] Run t/db_dependent/Upload.t The warning ("but file was missing") in the last subtest is fine; the file did not exist. Will be addressed in a follow-up. [2] Search for uploads on Tools/Upload. Clone this tab (repeat search on a new tab in your browser). [3] Delete an existing upload on the first tab. [4] Try to delete it again on the second tab. Error message? [5] Bonus points: Add an upload. Mark the file immutable with chattr +i. Try to delete the file. You should see a "Could not be deleted"-message. Signed-off-by: Marcel de Rooy --- Koha/Upload.pm | 47 ++---------------- Koha/UploadedFile.pm | 58 ++++++++++++++++++++-- Koha/UploadedFiles.pm | 22 ++++++-- .../intranet-tmpl/prog/en/modules/tools/upload.tt | 2 +- t/db_dependent/Upload.t | 34 ++++++++++--- tools/upload.pl | 19 ++++--- 6 files changed, 118 insertions(+), 64 deletions(-) diff --git a/Koha/Upload.pm b/Koha/Upload.pm index 4f4f5c5..e5a6c17 100644 --- a/Koha/Upload.pm +++ b/Koha/Upload.pm @@ -46,9 +46,6 @@ Koha::Upload - Facilitate file uploads (temporary and permanent) while( <$fh> ) { print $_; } $fh->close; - # delete an upload - my ( $fn ) = Koha::Upload->new->delete({ id => $id }); - =head1 DESCRIPTION This module is a refactored version of C4::UploadedFile but adds on top @@ -56,6 +53,9 @@ Koha::Upload - Facilitate file uploads (temporary and permanent) That report added module UploadedFiles.pm. This module contains the functionality of both. + The module has been revised to use Koha::Object[s]; the delete method + has been moved to Koha::UploadedFile[s]. + =head1 METHODS =cut @@ -192,19 +192,6 @@ sub get { return wantarray? @rv: $res; } -=head2 delete - - Returns array of deleted filenames or undef. - Since it now only accepts id as parameter, you should not expect more - than one filename. - -=cut - -sub delete { - my ( $self, $params ) = @_; - return $self->_delete( $params->{id} ); -} - =head1 CLASS METHODS =head2 getCategories @@ -262,8 +249,8 @@ sub allows_add_by { sub _init { my ( $self, $params ) = @_; - $self->{rootdir} = C4::Context->config('upload_path'); - $self->{tmpdir} = File::Spec->tmpdir; + $self->{rootdir} = Koha::UploadedFile->permanent_directory; + $self->{tmpdir} = Koha::UploadedFile->temporary_directory; $params->{tmp} = $params->{temp} if !exists $params->{tmp}; $self->{temporary} = $params->{tmp}? 1: 0; #default false @@ -402,30 +389,6 @@ sub _lookup { # Does always return an arrayref (perhaps an empty one) } -sub _delete { - my ( $self, $id ) = @_; - my $rec = Koha::UploadedFiles->find($id) || return; - my $filename = $rec->filename; - my $file = $self->_full_fname({ - permanent => $rec->permanent, - dir => $rec->dir, - hashvalue => $rec->hashvalue, - filename => $filename, - }); - - if( !-e $file ) { # we will just delete the record - # TODO Should we add a trace here for the missing file? - $rec->delete; - return $filename; - } elsif( unlink($file) ) { - $rec->delete; - return $filename; - } - $self->{files}->{ $rec->{filename} }->{errcode} = 7; - #NOTE: errcode=6 is used to report successful delete (see template) - return; -} - sub _compute { # Computes hash value when sub hook feeds the first block # For temporary files, the id is made unique with time diff --git a/Koha/UploadedFile.pm b/Koha/UploadedFile.pm index 5e573ba..01d0580 100644 --- a/Koha/UploadedFile.pm +++ b/Koha/UploadedFile.pm @@ -18,6 +18,7 @@ package Koha::UploadedFile; # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. use Modern::Perl; +use File::Spec; #use Koha::Database; @@ -41,17 +42,68 @@ Description =head3 delete -Delete uploaded file (to be extended) +Delete uploaded file. +It deletes not only the record, but also the actual file. + +Returns filename on successful delete or undef. =cut sub delete { - my ( $self, $params ) = @_; - $self->SUPER::delete( $params ); + my ( $self ) = @_; + + my $name = $self->filename; + my $file = $self->full_path; + + if( !-e $file ) { # we will just delete the record + 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 { + warn "Problem while deleting: $file"; + } + return; # something went wrong +} + +=head3 full_path + +Returns the fully qualified path name for an uploaded file. + +=cut + +sub full_path { + my ( $self ) = @_; + my $path = File::Spec->catfile( + $self->permanent? + $self->permanent_directory: $self->temporary_directory, + $self->dir, + $self->hashvalue. '_'. $self->filename, + ); + return $path; } =head2 CLASS METHODS +=head3 root_directory + +=cut + +sub permanent_directory { + my ( $class ) = @_; + return C4::Context->config('upload_path'); +} + +=head3 tmp_directory + +=cut + +sub temporary_directory { + my ( $class ) = @_; + return File::Spec->tmpdir; +} + =head3 _type Returns name of corresponding DBIC resultset diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm index 23bb52a..36eb0d2 100644 --- a/Koha/UploadedFiles.pm +++ b/Koha/UploadedFiles.pm @@ -40,15 +40,29 @@ Description =head2 INSTANCE METHODS -=head3 delete +=head3 delete, delete_errors -Delete uploaded files +Delete uploaded files. +Returns true if no errors occur. +Delete_errors returns the number of errors when deleting files. =cut sub delete { - my ( $self, $params ) = @_; - $self->SUPER::delete( $params ); + my ( $self ) = @_; + # We use the individual delete on each resultset record + my $err = 0; + while( my $row = $self->_resultset->next ) { + my $kohaobj = Koha::UploadedFile->_new_from_dbic( $row ); + $err++ if !$kohaobj->delete; + } + $self->{delete_errors} = $err; + return $err==0; +} + +sub delete_errors { + my ( $self ) = @_; + return $self->{delete_errors}; } =head2 CLASS METHODS diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt index 524c9f2..4fd5409 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt @@ -201,7 +201,7 @@ _("No temporary directory found."), _("File could not be read."), _("File has been deleted."), - _("File could not be deleted."), + _("File or upload record could not be deleted."), ]; //]]> diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t index 6c69521..239874d 100644 --- a/t/db_dependent/Upload.t +++ b/t/db_dependent/Upload.t @@ -36,6 +36,9 @@ our $uploads = [ [ { name => 'file4', cat => undef, size => 5000 }, # temp duplicate ], + [ + { name => 'file5', cat => undef, size => 7000 }, # temp duplicate + ], ]; # Redirect upload dir structure and mock File::Spec and CGI @@ -48,7 +51,7 @@ $cgimod->mock( 'new' => \&newCGI ); # Start testing subtest 'Test01' => sub { - plan tests => 7; + plan tests => 9; test01(); }; subtest 'Test02' => sub { @@ -64,7 +67,7 @@ subtest 'Test04' => sub { test04(); }; subtest 'Test05' => sub { - plan tests => 5; + plan tests => 6; test05(); }; subtest 'Test06' => sub { @@ -85,6 +88,12 @@ sub test01 { # Delete existing records (for later tests) $dbh->do( "DELETE FROM uploaded_files" ); + # Check mocked directories + is( Koha::UploadedFile->permanent_directory, $tempdir, + 'Check permanent directory' ); + is( Koha::UploadedFile->temporary_directory, $tempdir, + 'Check temporary directory' ); + my $upl = Koha::Upload->new({ category => $uploads->[$current_upload]->[0]->{cat}, }); @@ -143,11 +152,24 @@ sub test05 { # add temporary file with same name and contents, delete it is( $upl->count, 1, 'Upload 5 adds duplicate temporary file' ); my $id = $upl->result; my $r = $upl->get({ id => $id }); - my @d = $upl->delete({ id => $id }); - is( $d[0], $r->{name}, 'Delete successful' ); - is( -e $r->{path}? 1: 0, 0, 'File no longer found after delete' ); + + # testing delete via UploadedFiles (plural) + my $delete = Koha::UploadedFiles->search({ id => $id })->delete; + is( $delete, 1, 'Delete successful' ); + isnt( -e $r->{path}, 1, 'File no longer found after delete' ); is( scalar $upl->get({ id => $id }), undef, 'Record also gone' ); - is( $upl->delete({ id => $id }), undef, 'Repeated delete failed' ); + + # testing delete via UploadedFile (singular) + # Note that find returns a Koha::Object + $upl = Koha::Upload->new({ tmp => 1 }); + $upl->cgi; + $id = $upl->result; + my $kohaobj = Koha::UploadedFiles->find( $id ); + my $name = $kohaobj->filename; + my $path = $kohaobj->full_path; + $delete = $kohaobj->delete; + is( $delete, $name, 'Delete successful' ); + isnt( -e $path, 1, 'File no longer found after delete' ); } sub test06 { #some extra tests for get diff --git a/tools/upload.pl b/tools/upload.pl index 45b8d1c..ff31dd7 100755 --- a/tools/upload.pl +++ b/tools/upload.pl @@ -68,17 +68,20 @@ if ( $op eq 'new' ) { } elsif ( $op eq 'delete' ) { # delete only takes the id parameter - my $upl = Koha::Upload->new($upar); - my ($fn) = $upl->delete( { id => $id } ); - my $e = $upl->err; - my $msg = - $fn ? JSON::to_json( { $fn => 6 } ) - : $e ? JSON::to_json($e) - : undef; + my $upload = $plugin? + Koha::UploadedFiles->search({ public => 1, id => $id })->next: + Koha::UploadedFiles->find($id); + my $fn = $upload? $upload->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, + }); $template->param( mode => 'deleted', msg => $msg, - uploadcategories => $upl->getCategories, + uploadcategories => Koha::Upload->getCategories, ); output_html_with_http_headers $input, $cookie, $template->output; -- 2.1.4