Bug 6874: [QA Follow-up] Last adjustments for future developments
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Thu, 28 May 2015 09:54:56 +0000 (11:54 +0200)
committerTomas Cohen Arazi <tomascohen@unc.edu.ar>
Fri, 7 Aug 2015 18:23:36 +0000 (15:23 -0300)
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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

C4/UploadedFiles.pm
cataloguing/value_builder/upload.pl
installer/data/mysql/atomicupdate/06874_lastrevision.sql [new file with mode: 0644]
installer/data/mysql/kohastructure.sql
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt
t/db_dependent/UploadedFiles.t

index c03acbf..f426b60 100644 (file)
@@ -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
index 2cebadf..72bed79 100755 (executable)
@@ -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
 
 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           = "
     <script type=\"text/javascript\">
-        function Focus$function_name(subfield_managed) {
-            return 1;
-        }
-
-        function Blur$function_name(subfield_managed) {
-            return 1;
-        }
-
-        function Clic$function_name(index) {
+        function Click$function_name(event) {
+            var index = event.data.id;
             var id = document.getElementById(index).value;
             var IsFileUploadUrl=0;
             if (id.match(/opac-retrieve-file/)) {
@@ -55,16 +44,15 @@ sub plugin_javascript {
             }
             var newin=window.open(\"../cataloguing/plugin_launcher.pl?plugin_name=upload.pl&index=\"+index+\"&id=\"+id+\"&from_popup=0\"+\"&IsFileUploadUrl=\"+IsFileUploadUrl, 'upload', 'width=600,height=400,toolbar=false,scrollbars=no');
             newin.focus();
-
         }
     </script>
 ";
+    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 (file)
index 0000000..81b37fe
--- /dev/null
@@ -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;
index 1210ca4..13b6a6a 100644 (file)
@@ -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;
 
 --
index e08e424..87f0051 100644 (file)
@@ -7,36 +7,21 @@
     <script type="text/javascript" src="[% interface %]/lib/jquery/jquery.js"></script>
     <link rel="stylesheet" type="text/css" href="[% themelang %]/css/staff-global.css" />
     <script type="text/javascript">
-      function ValidateForm() {
-        var filename = document.forms["UploadForm"]["uploaded_file"].value;
-        var selected = 0;
-        var value;
-        $('form').each(function() {
-          value = $(this).find('input[type="radio"][name="dir"]:checked').val();
-          if (value) {
-            selected = 1;
-          }
-        });
-        if (!filename && !selected) {
-          alert("Please select a file and its destination.");
-          return false;
-        }
-        else if (!filename) {
-          alert("Please select a file to upload.");
-          return false;
-        }
-        else if (!selected) {
-          alert("Please select a file destination.");
-          return false;
+        function ValidateForm() {
+            var filename = document.forms["UploadForm"]["uploaded_file"].value;
+            if (!filename) {
+                alert("Please select a file to upload.");
+                return false;
+            }
+            return true;
         }
-        else {
-          return true;
-        }
-      }
     </script>
 
 </head>
 <body>
+
+<div id="doc3" class="yui-t2"><div id="bd"><div id="yui-main">
+
 [% IF ( success ) %]
 
     <script type="text/javascript">
         <p>Error: Failed to upload file. See logs for details.</p>
         <p><input type="button" value="close" onclick="window.close();" /></p>
     [% ELSE %]
-        [%# This block display recursively a directory tree in variable 'dirs' %]
-        [% BLOCK list_dirs %]
-            [% IF dirs.size %]
-                <ul>
-                    [% FOREACH dir IN dirs %]
-                        <li style="list-style-type:none">
-                            <input type="radio" name="dir" id="[% dir.value %]" value="[% dir.value %]">
-                                <label for="[% dir.value %]">
-                                    [% IF (dir.name == '/') %]
-                                        <em>(root)</em>
-                                    [% ELSE %]
-                                        [% dir.name %]
-                                    [% END %]
-                                </label>
-                            </input>
-                            [% INCLUDE list_dirs dirs=dir.dirs %]
-                        </li>
-                    [% END %]
-                </ul>
-            [% END %]
-        [% END %]
-
         [% IF (error_upload_path_not_configured) %]
           <h2>Configuration error</h2>
           <p>Configuration variable 'upload_path' is not configured.</p>
           [% IF (dangling) %]
               <p class="error">Error: The URL has no file to retrieve.</p>
           [% END %]
-          <h2>Please select the file to upload : </h2>
+
+          <h2>Please select the file to upload:</h2>
           <form name="UploadForm" method="post" enctype="multipart/form-data" action="/cgi-bin/koha/cataloguing/plugin_launcher.pl" onsubmit="return ValidateForm()">
-              [% filefield %]
-              <h3>Choose where to upload the file</h3>
-              [% INCLUDE list_dirs dirs = dirs_tree %]
               <input type="hidden" name="from_popup" value="1" />
               <input type="hidden" name="plugin_name" value="upload.pl" />
               <input type="hidden" name="index" value="[% index %]" />
-              <input type="submit" value="Upload file">
-              <input type="button" value="Cancel" onclick="window.close();" />
+
+              <div>[% filefield %]</div>
+              <p/>
+              <div>
+                  <label for="uploadcategory">Category: </label>
+                  [% IF uploadcategories %]
+                      <select id="uploadcategory" name="uploadcategory">
+                      [% FOREACH cat IN uploadcategories %]
+                          <option value="[% cat.code %]">[% cat.name %]</option>
+                      [% END %]
+                      </select>
+                  [% ELSE %]
+                      <input type="hidden" name="uploadcategory" value="CATALOGUING" />
+                  [% END %]
+              </div>
+              <p/>
+              <fieldset class="action">
+                  <input type="submit" value="Upload">
+                  <input type="button" value="Cancel" onclick="window.close();" />
+              </fieldset>
           </form>
         [% END %]
     [% END %]
 
 [% END %]
 
+</div></div></div>
+
 </body>
 </html>
index 908f9f9..f933546 100644 (file)
@@ -30,7 +30,7 @@ ok($id, "File uploaded, id is $id");
 
 my $file = C4::UploadedFiles::GetUploadedFile($id);
 isa_ok($file, 'HASH', "GetUploadedFiles($id)");
-foreach my $key (qw(id filename filepath dir)) {
+foreach my $key (qw(hashvalue filename filepath dir)) {
     ok(exists $file->{$key}, "GetUploadedFile($id)->{$key} exists");
 }
 
@@ -47,7 +47,7 @@ close $fh;
 
 my $DelResult;
 is(C4::UploadedFiles::DelUploadedFile($id),1, "DelUploadedFile($id) returned 1 as expected.");
-warning_like { $DelResult=C4::UploadedFiles::DelUploadedFile($id); } qr/file for id=/, "Expected warning for deleting Dangling Entry.";
+warning_like { $DelResult=C4::UploadedFiles::DelUploadedFile($id); } qr/file for hash/, "Expected warning for deleting Dangling Entry.";
 is($DelResult,-1, "DelUploadedFile($id) returned -1 as expected.");
 ok(! -e $file->{filepath}, "File $file->{filepath} does not exist anymore");