Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Mon, 21 Nov 2016 13:53:27 +0000 (14:53 +0100)
committerKyle M Hall <kyle@bywatersolutions.com>
Fri, 20 Jan 2017 14:20:04 +0000 (14:20 +0000)
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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Koha/Upload.pm
Koha/UploadedFile.pm
Koha/UploadedFiles.pm
koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt
t/db_dependent/Upload.t
tools/upload.pl

index 4f4f5c5..e5a6c17 100644 (file)
@@ -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
index 5e573ba..01d0580 100644 (file)
@@ -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
index 23bb52a..36eb0d2 100644 (file)
@@ -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
index acf767c..f89ef03 100644 (file)
         _("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."),
     ];
 //]]>
 </script>
index 6c69521..239874d 100644 (file)
@@ -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
index 45b8d1c..ff31dd7 100755 (executable)
@@ -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;