Attachment 58809 Details for Bug 17501 – Bug 17501: [Follow-up] QA Requests

[patch] Bug 17501: [Follow-up] QA Requests

Bug-17501-Follow-up-QA-Requests.patch (text/plain), 8.86 KB, created by Marcel de Rooy on 2017-01-11 13:15:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2017-01-11 13:15:49 UTC

Size: 8.86 KB

patch

obsolete

>From f7890d0f69351dde0901a03d5be6e5b413181c19 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Tue, 10 Jan 2017 17:11:08 +0100
>Subject: [PATCH] Bug 17501: [Follow-up] QA Requests
>Content-Type: text/plain; charset=utf-8
>
>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>
>---
> 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 de696f3..160c2d3 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 e970fbc..109593d 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 6f60b22..5aeb7a4 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 76f2cb6..fd70b13 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.1.4

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