From 3c0b0aeebc66ebab688cbd9dcc71ab1654b704f7 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 28 May 2015 11:54:56 +0200 Subject: [PATCH] Bug 6874: [QA Follow-up] Last adjustments for future developments Content-Type: text/plain; charset=utf-8 This patch does: [1] Some trivial template changes. Modified some comments (POD lines). [2] Converted plugin to new style. [3] Table updates: renames id to hashvalue, adds a autoincrement id, adds filesize, timestamp, owner and category. RM: This db rev is in a separate sql file in atomicupdate. [4] Code references to computed hash renamed to hashvalue instead of id. [5] Removed some code pertaining to exposing upload dir structure. The user now may choose a category; the uploader takes care of storage. The list of upload categories is now taken from authorised values; this might become a separate table in the future. (If there are none, the upload process creates one default fallback.) We can add e.g. permissions later, subdir structure, etc. (So dir will not necessarily be category anymore.) Test plan: [1] Run the db revision. [2] Upload new file. Check the record in the table. Delete it again; check. [3] Run t/db../UploadedFiles.t. [4] Run t/db../FrameworkPlugins.t -incl cataloguing/value_builder/upload.pl Signed-off-by: Marcel de Rooy --- C4/UploadedFiles.pm | 72 ++++++++++------ cataloguing/value_builder/upload.pl | 89 +++++--------------- .../data/mysql/atomicupdate/06874_lastrevision.sql | 11 +++ installer/data/mysql/kohastructure.sql | 10 ++- .../en/modules/cataloguing/value_builder/upload.tt | 87 ++++++++----------- t/db_dependent/UploadedFiles.t | 4 +- 6 files changed, 120 insertions(+), 153 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/06874_lastrevision.sql diff --git a/C4/UploadedFiles.pm b/C4/UploadedFiles.pm index c03acbf..f426b60 100644 --- a/C4/UploadedFiles.pm +++ b/C4/UploadedFiles.pm @@ -57,12 +57,16 @@ use Fcntl; use Encode; use C4::Context; +use C4::Koha; sub _get_file_path { - my ($id, $dirname, $filename) = @_; + my ($hash, $dirname, $filename) = @_; my $upload_path = C4::Context->config('upload_path'); - my $filepath = "$upload_path/$dirname/${id}_$filename"; + if( !-d "$upload_path/$dirname" ) { + mkdir "$upload_path/$dirname"; + } + my $filepath = "$upload_path/$dirname/${hash}_$filename"; $filepath =~ s|/+|/|g; return $filepath; @@ -90,21 +94,21 @@ It returns undef if file is not found =cut sub GetUploadedFile { - my ($id) = @_; + my ( $hashvalue ) = @_; - return unless $id; + return unless $hashvalue; my $dbh = C4::Context->dbh; my $query = qq{ - SELECT id, filename, dir + SELECT hashvalue, filename, dir FROM uploaded_files - WHERE id = ? + WHERE hashvalue = ? }; my $sth = $dbh->prepare($query); - $sth->execute($id); + $sth->execute( $hashvalue ); my $file = $sth->fetchrow_hashref; if ($file) { - $file->{filepath} = _get_file_path($file->{id}, $file->{dir}, + $file->{filepath} = _get_file_path($file->{hashvalue}, $file->{dir}, $file->{filename}); } @@ -134,7 +138,6 @@ $cgi->upload('uploaded_file')->handle; sub UploadFile { my ($filename, $dir, $handle) = @_; - $filename = decode_utf8($filename); if($filename =~ m#(^|/)\.\.(/|$)# or $dir =~ m#(^|/)\.\.(/|$)#) { warn "Filename or dirname contains '..'. Aborting upload"; @@ -152,15 +155,15 @@ sub UploadFile { $sha->add($data); $sha->add($filename); $sha->add($dir); - my $id = $sha->hexdigest; + my $hash = $sha->hexdigest; # Test if this id already exist - my $file = GetUploadedFile($id); + my $file = GetUploadedFile($hash); if ($file) { - return $file->{id}; + return $file->{hashvalue}; } - my $file_path = _get_file_path($id, $dir, $filename); + my $file_path = _get_file_path($hash, $dir, $filename); my $out_fh; # Create the file only if it doesn't exist @@ -170,16 +173,18 @@ sub UploadFile { } print $out_fh $data; + my $size= tell($out_fh); close $out_fh; my $dbh = C4::Context->dbh; my $query = qq{ - INSERT INTO uploaded_files (id, filename, dir) - VALUES (?,?, ?); + INSERT INTO uploaded_files (hashvalue, filename, filesize, dir, categorycode, owner) VALUES (?,?,?,?,?,?); }; my $sth = $dbh->prepare($query); - if($sth->execute($id, $filename, $dir)) { - return $id; + my $uid= C4::Context->userenv? C4::Context->userenv->{number}: undef; + # uid is null in unit test + if($sth->execute($hash, $filename, $size, $dir, $dir, $uid)) { + return $hash; } return; @@ -192,7 +197,7 @@ sub UploadFile { Determine if a entry is dangling. Returns: 2 == no db entry - 1 == no file + 1 == no plain file 0 == both a file and db entry. -1 == N/A (undef id / non-file-upload URL) @@ -230,9 +235,9 @@ sub DanglingEntry { =head2 DelUploadedFile - C4::UploadedFiles::DelUploadedFile($id); + C4::UploadedFiles::DelUploadedFile( $hash ); -Remove a previously uploaded file, given its id. +Remove a previously uploaded file, given its hash value. Returns: 1 == file deleted 0 == file not deleted @@ -241,16 +246,16 @@ Returns: 1 == file deleted =cut sub DelUploadedFile { - my ($id) = @_; + my ( $hashval ) = @_; my $retval; - if ($id) { - my $file = GetUploadedFile($id); + if ( $hashval ) { + my $file = GetUploadedFile( $hashval ); if($file) { my $file_path = $file->{filepath}; my $file_deleted = 0; unless( -f $file_path ) { - warn "Id $file->{id} is in database but not in filesystem, removing id from database"; + warn "Id $file->{hashvalue} is in database but no plain file found, removing id from database"; $file_deleted = 1; } else { if(unlink $file_path) { @@ -265,10 +270,10 @@ sub DelUploadedFile { my $dbh = C4::Context->dbh; my $query = qq{ DELETE FROM uploaded_files - WHERE id = ? + WHERE hashvalue = ? }; my $sth = $dbh->prepare($query); - my $numrows = $sth->execute($id); + my $numrows = $sth->execute( $hashval ); # if either a DB entry or file was deleted, # then clearly we have a deletion. if ($numrows>0 || $file_deleted==1) { @@ -279,17 +284,28 @@ sub DelUploadedFile { } } else { - warn "There was no file for id=($id)"; + warn "There was no file for hash $hashval."; $retval = -1; } } else { - warn "DelUploadFile called with no id."; + warn "DelUploadFile called without hash value."; $retval = -1; } return $retval; } +=head2 getCategories + + getCategories returns a list of upload category codes and names + +=cut + +sub getCategories { + my $cats = C4::Koha::GetAuthorisedValues('UPLOAD'); + [ map {{ code => $_->{authorised_value}, name => $_->{lib} }} @$cats ]; +} + =head2 httpheaders httpheaders returns http headers for a retrievable upload diff --git a/cataloguing/value_builder/upload.pl b/cataloguing/value_builder/upload.pl index 2cebadf..72bed79 100755 --- a/cataloguing/value_builder/upload.pl +++ b/cataloguing/value_builder/upload.pl @@ -1,5 +1,7 @@ #!/usr/bin/perl +# Converted to new plugin style (Bug 6874/See also 13437) + # This file is part of Koha. # # Copyright (C) 2011-2012 BibLibre @@ -19,32 +21,19 @@ use Modern::Perl; use CGI qw/-utf8/; -use File::Basename; use C4::Auth; use C4::Context; use C4::Output; use C4::UploadedFiles; -sub plugin_parameters { - my ( $dbh, $record, $tagslib, $i, $tabloop ) = @_; - return ""; -} - -sub plugin_javascript { - my ( $dbh, $record, $tagslib, $field_number, $tabloop ) = @_; - my $function_name = $field_number; +my $builder = sub { + my ( $params ) = @_; + my $function_name = $params->{id}; my $res = " "; + return $res; +}; - return ( $function_name, $res ); -} - -sub plugin { - my ($input) = @_; +my $launcher = sub { + my ( $params ) = @_; + my $input = $params->{cgi}; my $index = $input->param('index'); my $id = $input->param('id'); my $delete = $input->param('delete'); @@ -95,7 +83,7 @@ sub plugin { } # Dealing with the uploaded file - my $dir = $input->param('dir'); + my $dir = $input->param('uploadcategory'); if ($uploaded_file and $dir) { my $fh = $input->upload('uploaded_file'); @@ -132,16 +120,9 @@ sub plugin { -name => 'uploaded_file', -size => 50, ); - - my $dirs_tree = [ { - name => '/', - value => '/', - dirs => finddirs($upload_path) - } ]; - $template->param( - dirs_tree => $dirs_tree, - filefield => $filefield + filefield => $filefield, + uploadcategories => C4::UploadedFiles::getCategories(), ); } else { $template->param( error_upload_path_not_configured => 1 ); @@ -165,47 +146,19 @@ sub plugin { ); output_html_with_http_headers $input, $cookie, $template->output; -} - -# Build a hierarchy of directories -sub finddirs { - my $base = shift; - my $upload_path = C4::Context->config('upload_path'); - my $found = 0; - my @dirs; - my @files = glob("$base/*"); - foreach (@files) { - if (-d $_ and -w $_) { - my $lastdirname = basename($_); - my $dirname = $_; - $dirname =~ s/^$upload_path//g; - push @dirs, { - value => $dirname, - name => $lastdirname, - dirs => finddirs($_) - }; - $found = 1; - }; - } - return \@dirs; -} +}; -1; +return { builder => $builder, launcher => $launcher }; +1; __END__ =head1 upload.pl -This plugin allow to upload files on the server and reference it in a marc +This plugin allows to upload files on the server and reference it in a marc field. -Two system preference are used: - -=over 4 - -=item * upload_path: the real absolute path where files will be stored - -=item * OPACBaseURL: for building URLs to be stored in MARC +It uses config variable upload_path and pref OPACBaseURL. -=back +=cut diff --git a/installer/data/mysql/atomicupdate/06874_lastrevision.sql b/installer/data/mysql/atomicupdate/06874_lastrevision.sql new file mode 100644 index 0000000..81b37fe --- /dev/null +++ b/installer/data/mysql/atomicupdate/06874_lastrevision.sql @@ -0,0 +1,11 @@ +-- table adjustments for uploaded_files +ALTER TABLE uploaded_files + CHANGE COLUMN id hashvalue char(40) NOT NULL, + DROP PRIMARY KEY; +ALTER TABLE uploaded_files + ADD COLUMN id int NOT NULL AUTO_INCREMENT FIRST, + ADD CONSTRAINT PRIMARY KEY (id), + ADD COLUMN filesize int, + ADD COLUMN dtcreated timestamp, + ADD COLUMN categorycode tinytext, + ADD COLUMN owner int; diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 1210ca4..13b6a6a 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3366,9 +3366,15 @@ CREATE TABLE IF NOT EXISTS `borrower_modifications` ( DROP TABLE IF EXISTS uploaded_files; CREATE TABLE uploaded_files ( - id CHAR(40) NOT NULL PRIMARY KEY, + id int(11) NOT NULL AUTO_INCREMENT, + hashvalue CHAR(40) NOT NULL, filename TEXT NOT NULL, - dir TEXT NOT NULL + dir TEXT NOT NULL, + filesize int(11), + dtcreated timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, + categorycode tinytext, + owner int(11), + PRIMARY KEY (id) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; -- diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt index e08e424..87f0051 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt @@ -7,36 +7,21 @@ + +
+ [% IF ( success ) %]