Bugzilla – Attachment 59346 Details for
Bug 17501
Koha Objects for uploaded files
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]
Bug-17501-Move-KohaUploaddelete-to-KohaUploadedFil.patch (text/plain), 11.01 KB, created by
Jonathan Druart
on 2017-01-20 11:40:36 UTC
(
hide
)
Description:
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2017-01-20 11:40:36 UTC
Size:
11.01 KB
patch
obsolete
>From 1d330ff586457b10ff65193b29af21c21c4b7fde Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Mon, 21 Nov 2016 14:53:27 +0100 >Subject: [PATCH] Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s] > >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> >--- > 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 acf767c..f89ef03 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."), > ]; > //]]> > </script> >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
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 17501
:
57751
|
57752
|
57753
|
57754
|
57755
|
57756
|
57757
|
57758
|
57759
|
58613
|
58614
|
58615
|
58616
|
58617
|
58618
|
58619
|
58620
|
58621
|
58809
|
59342
|
59343
|
59344
|
59345
| 59346 |
59347
|
59348
|
59349
|
59350
|
59351