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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This commit is contained in:
Marcel de Rooy
Kyle M Hall
parent
a118dd2ba0
commit
e429db4746
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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' );
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Reference in New Issue