Attachment 59350 Details for Bug 17501 – Bug 17501: Additional polishing (POD, unit tests)

[patch] Bug 17501: Additional polishing (POD, unit tests)

Bug-17501-Additional-polishing-POD-unit-tests.patch (text/plain), 10.84 KB, created by Jonathan Druart on 2017-01-20 11:41:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2017-01-20 11:41:00 UTC

Size: 10.84 KB

patch

obsolete

>From 6ea0cbca858f6e92548523cd46ac5927bc370826 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Thu, 24 Nov 2016 14:13:22 +0100
>Subject: [PATCH] Bug 17501: Additional polishing (POD, unit tests)
>
>This patch adds some documentation lines.
>And mainly rearrangs the tests in Upload.t. The 'basic CRUD testing' is
>not needed separately any more. A new test catches the "file missing"
>warn.
>
>Test plan:
>[1] Run perldoc on UploadedFile[s].pm
>[2] Run t/db_dependent/Upload.t
>
>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/UploadedFile.pm    |  34 ++++++++++---
> Koha/UploadedFiles.pm   |  17 +++++--
> t/db_dependent/Upload.t | 129 ++++++++++++++++++++++--------------------------
> 3 files changed, 100 insertions(+), 80 deletions(-)
>
>diff --git a/Koha/UploadedFile.pm b/Koha/UploadedFile.pm
>index 085037d..de696f3 100644
>--- a/Koha/UploadedFile.pm
>+++ b/Koha/UploadedFile.pm
>@@ -20,8 +20,6 @@ package Koha::UploadedFile;
> use Modern::Perl;
> use File::Spec;
> 
>-#use Koha::Database;
>-
> use parent qw(Koha::Object);
> 
> =head1 NAME
>@@ -30,11 +28,28 @@ Koha::UploadedFile - Koha::Object class for single uploaded file
> 
> =head1 SYNOPSIS
> 
>-use Koha::UploadedFile;
>+    use Koha::UploadedFile;
>+
>+    # store record in uploaded_files
>+    my $upload = Koha::UploadedFile->new({ [columns and values] });
>+
>+    # get a file handle on an uploaded_file
>+    my $fh = $upload->file_handle;
>+
>+    # get full path
>+    my $path = $upload->full_path;
>+
>+    # delete uploaded file
>+    $upload->delete;
> 
> =head1 DESCRIPTION
> 
>-Description
>+Allows regular CRUD operations on uploaded_files via Koha::Object / DBIx.
>+
>+The delete method also takes care of deleting files. The full_path method
>+returns a fully qualified path for an upload.
>+
>+Additional methods include: file_handle, httpheaders.
> 
> =head1 METHODS
> 
>@@ -103,8 +118,9 @@ sub file_handle {
> 
> =head3 httpheaders
> 
>-    httpheaders returns http headers for a retrievable upload
>-    Will be extended by report 14282
>+httpheaders returns http headers for a retrievable upload.
>+
>+Will be extended by report 14282
> 
> =cut
> 
>@@ -120,6 +136,8 @@ sub httpheaders {
> 
> =head3 permanent_directory
> 
>+Returns root directory for permanent storage
>+
> =cut
> 
> sub permanent_directory {
>@@ -129,6 +147,8 @@ sub permanent_directory {
> 
> =head3 tmp_directory
> 
>+Returns root directory for temporary storage
>+
> =cut
> 
> sub temporary_directory {
>@@ -138,7 +158,7 @@ sub temporary_directory {
> 
> =head3 getCategories
> 
>-    getCategories returns a list of upload category codes and names
>+getCategories returns a list of upload category codes and names
> 
> =cut
> 
>diff --git a/Koha/UploadedFiles.pm b/Koha/UploadedFiles.pm
>index 7587dfa..e970fbc 100644
>--- a/Koha/UploadedFiles.pm
>+++ b/Koha/UploadedFiles.pm
>@@ -19,7 +19,6 @@ package Koha::UploadedFiles;
> 
> use Modern::Perl;
> 
>-#use Koha::Database;
> use Koha::UploadedFile;
> 
> use parent qw(Koha::Objects);
>@@ -30,11 +29,23 @@ Koha::UploadedFiles - Koha::Objects class for uploaded files
> 
> =head1 SYNOPSIS
> 
>-use Koha::UploadedFiles;
>+    use Koha::UploadedFiles;
>+
>+    # get one upload
>+    my $upload01 = Koha::UploadedFiles->find( $id );
>+
>+    # get some uploads
>+    my @uploads = Koha::UploadedFiles->search_term({ term => '.mrc' });
>+
>+    # delete all uploads
>+    Koha::UploadedFiles->new->delete;
> 
> =head1 DESCRIPTION
> 
>-Description
>+Allows regular CRUD operations on uploaded_files via Koha::Objects / DBIx.
>+
>+The delete method also takes care of deleting files. The search_term method
>+provides a wrapper around search to look for a term in multiple columns.
> 
> =head1 METHODS
> 
>diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t
>index 49ec239..6f60b22 100644
>--- a/t/db_dependent/Upload.t
>+++ b/t/db_dependent/Upload.t
>@@ -2,7 +2,8 @@
> 
> use Modern::Perl;
> use File::Temp qw/ tempdir /;
>-use Test::More tests => 9;
>+use Test::More tests => 10;
>+use Test::Warn;
> 
> use Test::MockModule;
> use t::lib::Mocks;
>@@ -16,6 +17,7 @@ use Koha::Uploader;
> 
> my $schema  = Koha::Database->new->schema;
> $schema->storage->txn_begin;
>+my $builder = t::lib::TestBuilder->new;
> 
> our $current_upload = 0;
> our $uploads = [
>@@ -50,50 +52,29 @@ my $cgimod = Test::MockModule->new( 'CGI' );
> $cgimod->mock( 'new' => \&newCGI );
> 
> # Start testing
>-subtest 'Test01' => sub {
>-    plan tests => 11;
>-    test01();
>-};
>-subtest 'Test02' => sub {
>-    plan tests => 5;
>-    test02();
>-};
>-subtest 'Test03' => sub {
>-    plan tests => 2;
>-    test03();
>-};
>-subtest 'Test04' => sub {
>-    plan tests => 3;
>-    test04();
>-};
>-subtest 'Test05' => sub {
>-    plan tests => 6;
>-    test05();
>-};
>-subtest 'Test06' => sub {
>-    plan tests => 3;
>-    test06();
>-};
>-subtest 'Test07' => sub {
>-    plan tests => 2;
>-    test07();
>-};
>-subtest 'Test08: allows_add_by' => sub {
>-    plan tests => 4;
>-    test08();
>-};
>-$schema->storage->txn_rollback;
>+subtest 'Make a fresh start' => sub {
>+    plan tests => 1;
> 
>-sub test01 {
>     # Delete existing records (for later tests)
>-    # Passing keep_file suppresses warnings
>+    # 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 });
>+    is( Koha::UploadedFiles->count, 0, 'No records left' );
>+};
>+
>+subtest 'permanent_directory and temporary_directory' => sub {
>+    plan tests => 2;
> 
>     # Check mocked directories
>     is( Koha::UploadedFile->permanent_directory, $tempdir,
>         'Check permanent directory' );
>     is( Koha::UploadedFile->temporary_directory, $tempdir,
>         'Check temporary directory' );
>+};
>+
>+subtest 'Add two uploads in category A' => sub {
>+    plan tests => 9;
> 
>     my $upl = Koha::Uploader->new({
>         category => $uploads->[$current_upload]->[0]->{cat},
>@@ -115,9 +96,11 @@ sub test01 {
>     is( $rec->filename, 'file2', 'Check file name 2' );
>     is( $rec->filesize, 8000, 'Check size of file2' );
>     is( $rec->public, undef, 'Check public undefined' );
>-}
>+};
>+
>+subtest 'Add another upload, check file_handle' => sub {
>+    plan tests => 5;
> 
>-sub test02 {
>     my $upl = Koha::Uploader->new({
>         category => $uploads->[$current_upload]->[0]->{cat},
>         public => 1,
>@@ -135,17 +118,21 @@ sub test02 {
>     $rec->filename( 'doesprobablynotexist' )->store;
>     is( $rec->file_handle, undef, 'Sabotage with file handle' );
>     $rec->filename( $orgname )->store;
>-}
>+};
>+
>+subtest 'Add temporary upload' => sub {
>+    plan tests => 2;
> 
>-sub test03 {
>     my $upl = Koha::Uploader->new({ tmp => 1 }); #temporary
>     my $cgi= $upl->cgi;
>     is( $upl->count, 1, 'Upload 3 includes one temporary file' );
>     my $rec = Koha::UploadedFiles->find( $upl->result );
>     is( $rec->uploadcategorycode =~ /_upload$/, 1, 'Check category temp file' );
>-}
>+};
>+
>+subtest 'Add same file in same category' => sub {
>+    plan tests => 3;
> 
>-sub test04 { # Fail on a file already there
>     my $upl = Koha::Uploader->new({
>         category => $uploads->[$current_upload]->[0]->{cat},
>     });
>@@ -154,12 +141,15 @@ sub test04 { # Fail on a file already there
>     is( $upl->result, undef, 'Result is undefined' );
>     my $e = $upl->err;
>     is( $e->{file2}, 1, "Errcode 1 [already exists] reported" );
>-}
>+};
>+
>+subtest 'Test delete via UploadedFile as well as UploadedFiles' => sub {
>+    plan tests => 8;
> 
>-sub test05 { # add temporary file with same name and contents, delete it
>+    # add temporary file with same name and contents (file4)
>     my $upl = Koha::Uploader->new({ tmp => 1 });
>     my $cgi= $upl->cgi;
>-    is( $upl->count, 1, 'Upload 5 adds duplicate temporary file' );
>+    is( $upl->count, 1, 'Add duplicate temporary file (file4)' );
>     my $id = $upl->result;
>     my $path = Koha::UploadedFiles->find( $id )->full_path;
> 
>@@ -179,9 +169,19 @@ sub test05 { # add temporary file with same name and contents, delete it
>     $delete = $kohaobj->delete;
>     is( $delete, $name, 'Delete successful' );
>     isnt( -e $path, 1, 'File no longer found after delete' );
>-}
> 
>-sub test06 { #search_term with[out] private flag
>+    # add another record with TestBuilder, so file does not exist
>+    # catch warning
>+    my $upload01 = $builder->build({ source => 'UploadedFile' });
>+    warning_like { Koha::UploadedFiles->find( $upload01->{id} )->delete; }
>+        qr/file was missing/,
>+        'delete warns when file is missing';
>+    is( Koha::UploadedFiles->count, 4, 'Back to four uploads now' );
>+};
>+
>+subtest 'Call search_term with[out] private flag' => sub {
>+    plan tests => 3;
>+
>     my @recs = Koha::UploadedFiles->search_term({ term => 'file' });
>     is( @recs, 1, 'Returns only one public result' );
>     is( $recs[0]->filename, 'file3', 'Should be file3' );
>@@ -189,20 +189,22 @@ sub test06 { #search_term with[out] private flag
>     is( Koha::UploadedFiles->search_term({
>         term => 'file', include_private => 1,
>     })->count, 4, 'Returns now four results' );
>-}
>+};
>+
>+subtest 'Simple tests for httpheaders and getCategories' => sub {
>+    plan tests => 2;
> 
>-sub test07 { #simple test for httpheaders and getCategories
>     my $rec = Koha::UploadedFiles->search_term({ term => 'file' })->next;
>     my @hdrs = $rec->httpheaders;
>     is( @hdrs == 4 && $hdrs[1] =~ /application\/octet-stream/, 1, 'Simple test for httpheaders');
>-    my $builder = t::lib::TestBuilder->new;
>     $builder->build({ source => 'AuthorisedValue', value => { category => 'UPLOAD', authorised_value => 'HAVE_AT_LEAST_ONE', lib => 'Hi there' } });
>     my $cat = Koha::UploadedFile->getCategories;
>     is( @$cat >= 1, 1, 'getCategories returned at least one category' );
>-}
>+};
>+
>+subtest 'Testing allows_add_by' => sub {
>+    plan tests => 4;
> 
>-sub test08 { # allows_add_by
>-    my $builder = t::lib::TestBuilder->new;
>     my $patron = $builder->build({
>         source => 'Borrower',
>         value  => { flags => 0 }, #no permissions
>@@ -236,25 +238,12 @@ sub test08 { # allows_add_by
>     });
>     is( Koha::Uploader->allows_add_by( $patron->{userid} ),
>         1, 'Patron is still allowed to add uploaded files' );
>-}
>-
>-# Additional tests for Koha::UploadedFiles
>-# TODO Rearrange the tests after this migration
>-subtest 'Some basic CRUD testing' => sub {
>-    plan tests => 2;
>-
>-    # Test find and attribute id, delete and search
>-    my $builder = t::lib::TestBuilder->new;
>-    my $upload01 = $builder->build({ source => 'UploadedFile' });
>-    my $found = Koha::UploadedFiles->find( $upload01->{id} );
>-    is( $found->id, $upload01->{id}, 'Koha::Object returns id' );
>-    $found->delete({ keep_file => 1 }); #note that it does not exist
>-    $found = Koha::UploadedFiles->search(
>-        { id => $upload01->{id} },
>-    );
>-    is( $found->count, 0, 'Delete seems successful' );
> };
> 
>+# The end
>+$schema->storage->txn_rollback;
>+
>+# Helper routine
> sub newCGI {
>     my ( $class, $hook ) = @_;
>     my $read = 0;
>-- 
>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