@@ -, +, @@ minor improvements - Fixed various QA issues. - Added unit tests for the new module. - Improved error handling; extended pod documentation - Attached file records were not properly handled when merging --- C4/Auth.pm | 1 - Koha/Misc/Files.pm | 172 +++++++++++++++----- acqui/invoice-files.pl | 35 ++-- acqui/invoice.pl | 18 +- .../prog/en/modules/acqui/invoice-files.tt | 134 ++++++++------- .../intranet-tmpl/prog/en/modules/acqui/invoice.tt | 28 +++- t/db_dependent/Koha_Misc_Files.t | 87 ++++++++++ 7 files changed, 334 insertions(+), 141 deletions(-) create mode 100755 t/db_dependent/Koha_Misc_Files.t --- a/C4/Auth.pm +++ a/C4/Auth.pm @@ -358,7 +358,6 @@ sub get_template_and_user { EnableBorrowerFiles => C4::Context->preference('EnableBorrowerFiles'), UseKohaPlugins => C4::Context->preference('UseKohaPlugins'), UseCourseReserves => C4::Context->preference("UseCourseReserves"), - AcqEnableFiles => C4::Context->preference('AcqEnableFiles'), ); } else { --- a/Koha/Misc/Files.pm +++ a/Koha/Misc/Files.pm @@ -20,40 +20,74 @@ package Koha::Misc::Files; # along with Koha; if not, see . use Modern::Perl; -use strict; ## not needed if Modern::Perl, but perlcritic complains.. use vars qw($VERSION); -$VERSION = '0.20'; +$VERSION = '0.23'; use C4::Context; use C4::Output; use C4::Dates; -use C4::Debug; =head1 NAME -Koha::Misc::Files - module for managing miscellaneous files -associated with records from arbitrary tables +Koha::Misc::Files - module for managing miscellaneous files associated +with records from arbitrary tables + +=head1 SYNOPSIS + +use Koha::Misc::Files; + +my $mf = Koha::Misc::Files->new( tabletag => $tablename, + recordid => $recordnumber ); + +=head1 FUNCTIONS + +=over + +=item new() + +my $mf = Koha::Misc::Files->new( tabletag => $tablename, + recordid => $recordnumber ); + +Creates new Koha::Misc::Files object. Such object is essentially +a pair: in typical usage scenario, 'tabletag' parameter will be +a database table name, and 'recordid' an unique record ID number +from this table. However, this method does accept an arbitrary +string as 'tabletag', and an arbitrary integer as 'recordid'. + +Particular Koha::Misc::Files object can have one or more file records +(actuall file contents + various file metadata) associated with it. + +In case of an error (wrong parameter format) it returns undef. =cut sub new { my ( $class, %args ) = @_; - ( defined( $args{'tabletag'} ) && defined( $args{'recordid'} ) ) + my $recid = $args{'recordid'}; + my $tag = $args{'tabletag'}; + ( defined($tag) && $tag ne '' && defined($recid) && $recid =~ /^\d+$/ ) || return (); + my $self = bless( {}, $class ); - $self->{'table_tag'} = $args{'tabletag'}; - $self->{'record_id'} = $args{'recordid'}; + $self->{'table_tag'} = $tag; + $self->{'record_id'} = '' . ( 0 + $recid ); return $self; } =item GetFilesInfo() - my $mf = Koha::Misc::Files->new( tabletag => $tablename, - recordid => $recordnumber); - my $files_hashref = $mf->GetFilesInfo +my $files_descriptions = $mf->GetFilesInfo(); + +This method returns a reference to an array of hashes +containing files metadata (file_id, file_name, file_type, +file_description, file_size, date_uploaded) for all file records +associated with given $mf object, or an empty arrayref if there are +no such records yet. + +In case of an error it returns undef. =cut @@ -61,53 +95,69 @@ sub GetFilesInfo { my $self = shift; my $dbh = C4::Context->dbh; - my $query = " + my $query = ' SELECT file_id, file_name, file_type, file_description, - date_uploaded + date_uploaded, + LENGTH(file_content) AS file_size FROM misc_files WHERE table_tag = ? AND record_id = ? ORDER BY file_name, date_uploaded - "; + '; my $sth = $dbh->prepare($query); $sth->execute( $self->{'table_tag'}, $self->{'record_id'} ); return $sth->fetchall_arrayref( {} ); } =item AddFile() - my $mf = Koha::Misc::Files->new( tabletag => $tablename, - recordid => $recordnumber); - $mf->AddFile( name => $filename, type => $mimetype, description => $description, content => $content ); + +$mf->AddFile( name => $filename, type => $mimetype, + description => $description, content => $content ); + +Adds a new file (we want to store for / associate with a given +object) to the database. Parameters 'name' and 'content' are mandatory. +Note: this method would (silently) fail if there is no 'name' given +or if the 'content' provided is empty. + =cut sub AddFile { my ( $self, %args ) = @_; my $name = $args{'name'}; - my $type = $args{'type'}; + my $type = $args{'type'} // ''; my $description = $args{'description'}; my $content = $args{'content'}; - return unless ( $name && $content ); + return unless ( defined($name) && $name ne '' && defined($content) && $content ne '' ); my $dbh = C4::Context->dbh; - my $query = " + my $query = ' INSERT INTO misc_files ( table_tag, record_id, file_name, file_type, file_description, file_content ) VALUES ( ?,?,?,?,?,? ) - "; + '; my $sth = $dbh->prepare($query); $sth->execute( $self->{'table_tag'}, $self->{'record_id'}, $name, $type, $description, $content ); } =item GetFile() - my $mf = Koha::Misc::Files->new( tabletag => $tablename, - recordid => $recordnumber); - ... - my $file = $mf->GetFile( id => $file_id ); + +my $file = $mf->GetFile( id => $file_id ); + +For an individual, specific file ID this method returns a hashref +containing all metadata (file_id, table_tag, record_id, file_name, +file_type, file_description, file_content, date_uploaded), plus +an actuall contents of a file (in 'file_content'). In typical usage +scenarios, for a given $mf object, specific file IDs have to be +obtained first by GetFilesInfo() call. + +Returns undef in case when file ID specified as 'id' parameter was not +found in the database. + =cut sub GetFile { @@ -116,19 +166,21 @@ sub GetFile { my $file_id = $args{'id'}; my $dbh = C4::Context->dbh; - my $query = " + my $query = ' SELECT * FROM misc_files WHERE file_id = ? AND table_tag = ? AND record_id = ? - "; + '; my $sth = $dbh->prepare($query); $sth->execute( $file_id, $self->{'table_tag'}, $self->{'record_id'} ); return $sth->fetchrow_hashref(); } =item DelFile() - my $mf = Koha::Misc::Files->new( tabletag => $tablename, - recordid => $recordnumber); - ... - $mf->DelFile( id => $file_id ); + +$mf->DelFile( id => $file_id ); + +Deletes specific, individual file record (file contents and metadata) +from the database. + =cut sub DelFile { @@ -137,38 +189,78 @@ sub DelFile { my $file_id = $args{'id'}; my $dbh = C4::Context->dbh; - my $query = " + my $query = ' DELETE FROM misc_files WHERE file_id = ? AND table_tag = ? AND record_id = ? - "; + '; my $sth = $dbh->prepare($query); $sth->execute( $file_id, $self->{'table_tag'}, $self->{'record_id'} ); } =item DelAllFiles() - my $mf = Koha::Misc::Files->new( tabletag => $tablename, - recordid => $recordnumber); - $mf->DelAllFiles; + +$mf->DelAllFiles(); + +Deletes all file records associated with (stored for) a given $mf object. + =cut sub DelAllFiles { my ($self) = @_; my $dbh = C4::Context->dbh; - my $query = " + my $query = ' DELETE FROM misc_files WHERE table_tag = ? AND record_id = ? - "; + '; my $sth = $dbh->prepare($query); $sth->execute( $self->{'table_tag'}, $self->{'record_id'} ); } +=item MergeFileRecIds() + +$mf->MergeFileRecIds(@ids_to_be_merged); + +This method re-associates all individuall file records associated with +some "parent" records IDs (provided in @ids_to_be_merged) with the given +single $mf object (which would be treated as a "parent" destination). + +This a helper method; typically it needs to be called only in cases when +some "parent" records are being merged in the (external) 'tablename' +table. + +=cut + +sub MergeFileRecIds { + my ( $self, @ids_to_merge ) = @_; + + my $dst_recid = $self->{'record_id'}; + @ids_to_merge = map { ( $dst_recid == $_ ) ? () : ($_); } @ids_to_merge; + @ids_to_merge > 0 || return (); + + my $dbh = C4::Context->dbh; + my $query = ' + UPDATE misc_files SET record_id = ? + WHERE table_tag = ? AND record_id = ? + '; + my $sth = $dbh->prepare($query); + + for my $src_recid (@ids_to_merge) { + $sth->execute( $dst_recid, $self->{'table_tag'}, $src_recid ); + } +} + 1; + __END__ =back +=head1 SEE ALSO + +Koha::Borrower::Files + =head1 AUTHOR -Kyle M Hall -Jacek Ablewicz +Kyle M Hall Ekyle.m.hall@gmail.comE, +Jacek Ablewicz Eablewicz@gmail.comE =cut --- a/acqui/invoice-files.pl +++ a/acqui/invoice-files.pl @@ -27,21 +27,18 @@ Manage files associated with invoice =cut -use strict; -use warnings; +use Modern::Perl; use CGI; use C4::Auth; use C4::Output; use C4::Acquisition; -use C4::Debug; -use Koha::DateUtils; use Koha::Misc::Files; my $input = new CGI; my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( { - template_name => 'acqui/invoice-files.tmpl', + template_name => 'acqui/invoice-files.tt', query => $input, type => 'intranet', authnotrequired => 0, @@ -50,11 +47,12 @@ my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( } ); -my $invoiceid = $input->param('invoiceid'); +my $invoiceid = $input->param('invoiceid') // ''; my $op = $input->param('op') // ''; +my %errors; -$template->param( 'invoice_files' => 1 ); my $mf = Koha::Misc::Files->new( tabletag => 'aqinvoices', recordid => $invoiceid ); +defined($mf) || do { $op = 'none'; $errors{'invalid_parameter'} = 1; }; if ( $op eq 'download' ) { my $file_id = $input->param('file_id'); @@ -62,11 +60,11 @@ if ( $op eq 'download' ) { my $fname = $file->{'file_name'}; my $ftype = $file->{'file_type'}; - if ($input->param('view') && ($ftype =~ /^image\//i || $fname =~ /\.pdf/i)) { + if ($input->param('view') && ($ftype =~ m|^image/|i || $fname =~ /\.pdf/i)) { $fname =~ /\.pdf/i && do { $ftype='application/pdf'; }; print $input->header( -type => $ftype, - -charset => 'utf-8', + -charset => 'utf-8' ); } else { print $input->header( @@ -86,7 +84,6 @@ else { booksellerid => $details->{'booksellerid'}, datereceived => $details->{'datereceived'}, ); - my %errors; if ( $op eq 'upload' ) { my $uploaded_file = $input->upload('uploadfile'); @@ -96,23 +93,16 @@ else { my $mimetype = $input->uploadInfo($filename)->{'Content-Type'}; $errors{'empty_upload'} = 1 if ( -z $uploaded_file ); - - if (%errors) { - $template->param( errors => %errors ); - } - else { - my $file_content; - while (<$uploaded_file>) { - $file_content .= $_; - } - if ($mimetype =~ /^application\/(force-download|unknown)$/i && $filename =~ /\.pdf$/) { + unless (%errors) { + my $file_content = do { local $/; <$uploaded_file>; }; + if ($mimetype =~ /^application\/(force-download|unknown)$/i && $filename =~ /\.pdf$/i) { $mimetype = 'application/pdf'; } $mf->AddFile( name => $filename, type => $mimetype, content => $file_content, - description => $input->param('description'), + description => $input->param('description') ); } } @@ -124,9 +114,8 @@ else { } $template->param( - files => $mf->GetFilesInfo(), + files => (defined($mf)? $mf->GetFilesInfo(): undef), errors => \%errors ); - output_html_with_http_headers $input, $cookie, $template->output; } --- a/acqui/invoice.pl +++ a/acqui/invoice.pl @@ -52,6 +52,12 @@ my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( my $invoiceid = $input->param('invoiceid'); my $op = $input->param('op'); +my $invoice_files; +if ( C4::Context->preference('AcqEnableFiles') ) { + $invoice_files = Koha::Misc::Files->new( + tabletag => 'aqinvoices', recordid => $invoiceid ); +} + if ( $op && $op eq 'close' ) { CloseInvoice($invoiceid); my $referer = $input->param('referer'); @@ -87,14 +93,13 @@ elsif ( $op && $op eq 'mod' ) { } elsif ($input->param('merge')) { my @sources = $input->param('merge'); MergeInvoices($invoiceid, \@sources); + defined($invoice_files) && $invoice_files->MergeFileRecIds(@sources); } $template->param( modified => 1 ); } elsif ( $op && $op eq 'delete' ) { DelInvoice($invoiceid); - C4::Context->preference('AcqEnableFiles') - && $invoiceid && Koha::Misc::Files->new( - tabletag => 'aqinvoices', recordid => $invoiceid )->DelAllFiles(); + defined($invoice_files) && $invoice_files->DelAllFiles(); my $referer = $input->param('referer') || 'invoices.pl'; if ($referer) { print $input->redirect($referer); @@ -213,12 +218,7 @@ $template->param( budgets_loop => \@budgets_loop, ); -{ - C4::Context->preference('AcqEnableFiles') || last; - my $mf = Koha::Misc::Files->new( - tabletag => 'aqinvoices', recordid => $invoiceid ); - defined( $mf ) && $template->param( files => $mf->GetFilesInfo() ); -} +defined( $invoice_files ) && $template->param( files => $invoice_files->GetFilesInfo() ); sub get_infos { my $order = shift; --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice-files.tt +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice-files.tt @@ -4,6 +4,23 @@ Koha › Acquisitions › Invoice › Files [% INCLUDE 'doc-head-close.inc' %] +[% INCLUDE 'datatables.inc' %] + [% INCLUDE 'header.inc' %] @@ -16,67 +33,64 @@
-

Files for invoice: [% invoicenumber %]

- -

Vendor: [% suppliername %]

- - [% IF errors %] -
- [% IF errors.empty_upload %]The file you are attempting to upload has no contents.[% END %] - [% IF errors.no_file %]You did not select a file to upload.[% END %] -
- [% END %] - - [% IF ( files ) %] - - - - - - - - - - - - - - [% FOREACH f IN files %] - - - - - - - - - [% END %] - -
NameTypeDescriptionUploaded  
[% f.file_name %][% f.file_type %][% f.file_description %][% f.date_uploaded | $KohaDates %]DeleteDownload
- [% ELSE %] -
-

This invoice has no files attached.

-
+

Files for invoice: [% invoicenumber | html %]

+

Vendor: [% suppliername %]

+
+ [% IF errors %] +
+ [% IF errors.empty_upload %]The file you are attempting to upload has no contents.[% END %] + [% IF errors.no_file %]You did not select a file to upload.[% END %] + [% IF errors.invalid_parameter %]Invalid or missing script parameter.[% END %] +
+ [% END %] + [% IF files %] + + + + + + + + + + + + + + [% FOREACH f IN files %] + + + + + + + + + [% END %] - - -
- Upload New File -
    -
  1. - - - - -
  2. - -
  3. - -
-
-
- - + +
NameTypeDescriptionUploadedBytes  
[% f.file_name | html %][% f.file_type | html %][% f.file_description | html %][% f.date_uploaded | $KohaDates %][% f.file_size %]DeleteDownload
+ [% ELSE %] +
+

This invoice has no files attached.

+
+ [% END %] + [% IF invoiceid %] +
+
+
+ Upload New File +
    +
  1. + + +
  2. +
  3. +
+
+
+
+ [% END %]
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt @@ -1,3 +1,4 @@ +[% USE Koha %] [% USE KohaDates %] [% INCLUDE 'doc-head-open.inc' %] @@ -15,6 +16,17 @@ bFilter: false, sDom: "t" })); +[% IF ( (Koha.Preference('AcqEnableFiles')) && files ) %] + $("#invoice_files_table").dataTable($.extend(true, {}, dataTablesDefaults, { + "aoColumnDefs": [ + { "aTargets": [ 3 ], "sType": "natural" } + ], + bInfo: false, + bPaginate: false, + bFilter: false, + sDom: "t" + })); +[% END %] }); //]]> @@ -89,7 +101,7 @@

Go to receipt page - [% IF ( AcqEnableFiles ) %]| Manage invoice files[% END %] + [% IF Koha.Preference('AcqEnableFiles') %]| Manage invoice files[% END %]

Invoice details

[% IF orders_loop.size %] @@ -170,10 +182,10 @@ [% ELSE %]

No orders yet

[% END %] - [% IF ( AcqEnableFiles && files ) %] -
+ [% IF ( (Koha.Preference('AcqEnableFiles')) && files ) %] +

Files attached to invoice

- +
@@ -185,10 +197,10 @@ [% FOREACH f IN files %] - - - - + + + + [% END %] --- a/t/db_dependent/Koha_Misc_Files.t +++ a/t/db_dependent/Koha_Misc_Files.t @@ -0,0 +1,87 @@ +#!/usr/bin/perl + +# Unit tests for Koha::Misc::Files +# Author: Jacek Ablewicz, abl@biblos.pk.edu.pl + +use Modern::Perl; +use C4::Context; +use Test::More tests => 27; + +BEGIN { + use_ok('Koha::Misc::Files'); +} + +my $dbh = C4::Context->dbh; +$dbh->{AutoCommit} = 0; +$dbh->{RaiseError} = 1; + +## new() parameter handling check +is(Koha::Misc::Files->new(recordid => 12), undef, "new() param check test/1"); +is(Koha::Misc::Files->new(recordid => 'aa123', tabletag => 'ttag_a'), undef, "new() param check test/2"); + +## create some test objects with arbitrary (tabletag, recordid) pairs +my $mf_a_123 = Koha::Misc::Files->new(recordid => '123', tabletag => 'tst_table_a'); +my $mf_a_124 = Koha::Misc::Files->new(recordid => '124', tabletag => 'tst_table_a'); +my $mf_b_221 = Koha::Misc::Files->new(recordid => '221', tabletag => 'tst_table_b'); +is(ref($mf_a_123), "Koha::Misc::Files", "new() returned object type"); + +## GetFilesInfo() initial tests (dummy AddFile() / parameter handling checks) +is(ref($mf_a_123->GetFilesInfo()), 'ARRAY', "GetFilesInfo() return type"); +is(scalar @{$mf_a_123->GetFilesInfo()}, 0, "GetFilesInfo() empty/non-empty result/1"); +$mf_a_123->AddFile(name => '', type => 'text/plain', content => "aaabbcc"); +is(scalar @{$mf_a_123->GetFilesInfo()}, 0, "GetFilesInfo() empty/non-empty result/2"); + +## AddFile(); add 5 sample file records for 3 test objects +$mf_a_123->AddFile(name => 'File_name_1.txt', type => 'text/plain', + content => "file contents\n1111\n", description => "File #1 sample description"); +$mf_a_123->AddFile(name => 'File_name_2.txt', type => 'text/plain', + content => "file contents\n2222\n", description => "File #2 sample description"); +$mf_a_124->AddFile(name => 'File_name_3.txt', content => "file contents\n3333\n", type => 'text/whatever'); +$mf_a_124->AddFile(name => 'File_name_4.txt', content => "file contents\n4444\n"); +$mf_b_221->AddFile(name => 'File_name_5.txt', content => "file contents\n5555\n"); + +## check GetFilesInfo() results for added files +my $files_a_123_infos = $mf_a_123->GetFilesInfo(); +is(scalar @$files_a_123_infos, 2, "GetFilesInfo() result count/1"); +is(scalar @{$mf_b_221->GetFilesInfo()}, 1, "GetFilesInfo() result count/2"); +is(ref($files_a_123_infos->[0]), 'HASH', "GetFilesInfo() item file result type"); +is($files_a_123_infos->[0]->{file_name}, 'File_name_1.txt', "GetFilesInfo() result check/1"); +is($files_a_123_infos->[1]->{file_name}, 'File_name_2.txt', "GetFilesInfo() result check/2"); +is($files_a_123_infos->[1]->{file_type}, 'text/plain', "GetFilesInfo() result check/3"); +is($files_a_123_infos->[1]->{file_size}, 19, "GetFilesInfo() result check/4"); +is($files_a_123_infos->[1]->{file_description}, 'File #2 sample description', "GetFilesInfo() result check/5"); + +## GetFile() result checks +is($mf_a_123->GetFile(), undef, "GetFile() result check/1"); +is($mf_a_123->GetFile(id => 0), undef, "GetFile() result check/2"); + +my $a123_file_1 = $mf_a_123->GetFile(id => $files_a_123_infos->[0]->{file_id}); +is(ref($a123_file_1), 'HASH', "GetFile() result check/3"); +is($a123_file_1->{file_id}, $files_a_123_infos->[0]->{file_id}, "GetFile() result check/4"); +is($a123_file_1->{file_content}, "file contents\n1111\n", "GetFile() result check/5"); + +## MergeFileRecIds() tests +$mf_a_123->MergeFileRecIds(123,221); +$files_a_123_infos = $mf_a_123->GetFilesInfo(); +is(scalar @$files_a_123_infos, 2, "GetFilesInfo() result count after dummy MergeFileRecIds()"); +$mf_a_123->MergeFileRecIds(124); +$files_a_123_infos = $mf_a_123->GetFilesInfo(); +is(scalar @$files_a_123_infos, 4, "GetFilesInfo() result count after MergeFileRecIds()/1"); +is(scalar @{$mf_a_124->GetFilesInfo()}, 0, "GetFilesInfo() result count after MergeFileRecIds()/2"); +is($files_a_123_infos->[-1]->{file_name}, 'File_name_4.txt', "GetFilesInfo() result check after MergeFileRecIds()"); + +## DelFile() test +$mf_a_123->DelFile(id => $files_a_123_infos->[-1]->{file_id}); +$files_a_123_infos = $mf_a_123->GetFilesInfo(); +is(scalar @$files_a_123_infos, 3, "GetFilesInfo() result count after DelFile()"); + +## DelAllFiles() tests +$mf_a_123->DelAllFiles(); +$files_a_123_infos = $mf_a_123->GetFilesInfo(); +is(scalar @$files_a_123_infos, 0, "GetFilesInfo() result count after DelAllFiles()/1"); +$mf_b_221->DelAllFiles(); +is(scalar @{$mf_b_221->GetFilesInfo()}, 0, "GetFilesInfo() result count after DelAllFiles()/2"); + +$dbh->rollback; + +1; --
Name
[% f.file_name %][% f.file_type %][% f.file_description %][% f.date_uploaded | $KohaDates %][% f.file_name | html %][% f.file_type | html %][% f.file_description | html %][% f.date_uploaded | $KohaDates %]