Bugzilla – Attachment 41287 Details for
Bug 6874
Attach files to bibliographic records
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 6874: [QA Follow-up] Last adjustments for future developments
Bug-6874-QA-Follow-up-Last-adjustments-for-future-.patch (text/plain), 18.92 KB, created by
Marcel de Rooy
on 2015-07-31 17:07:04 UTC
(
hide
)
Description:
Bug 6874: [QA Follow-up] Last adjustments for future developments
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2015-07-31 17:07:04 UTC
Size:
18.92 KB
patch
obsolete
>From 5610ee932a741b16cb9fbfbe413176c710681c38 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >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] Upload new file. Check the record in the table. Delete it again; check. >[2] Run t/db../UploadedFiles.t. >[3] Run t/db../FrameworkPlugins.t -incl cataloguing/value_builder/upload.pl > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >--- > 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 = " > <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 >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 @@ > <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"> >@@ -63,28 +48,6 @@ > <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> >@@ -102,21 +65,39 @@ > [% 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> >diff --git a/t/db_dependent/UploadedFiles.t b/t/db_dependent/UploadedFiles.t >index 908f9f9..f933546 100644 >--- a/t/db_dependent/UploadedFiles.t >+++ b/t/db_dependent/UploadedFiles.t >@@ -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"); > >-- >1.7.10.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 6874
:
5439
|
5440
|
5764
|
7114
|
7980
|
8234
|
8468
|
10664
|
10673
|
10863
|
10982
|
11098
|
11634
|
12355
|
12356
|
12357
|
12358
|
14130
|
14131
|
14132
|
14133
|
16343
|
16354
|
16355
|
16356
|
16357
|
16370
|
16693
|
16694
|
16695
|
16696
|
16697
|
16698
|
16699
|
16712
|
16713
|
16714
|
16715
|
16716
|
16717
|
16718
|
16719
|
17368
|
17369
|
18767
|
18768
|
18769
|
18770
|
18771
|
18772
|
18773
|
18774
|
18775
|
18776
|
18777
|
18891
|
18892
|
18893
|
18894
|
20059
|
20060
|
20779
|
21432
|
21433
|
21434
|
21435
|
21436
|
21489
|
21543
|
21546
|
21635
|
21798
|
21799
|
21800
|
21801
|
21802
|
21803
|
23588
|
23589
|
23590
|
23591
|
23607
|
23608
|
23609
|
23610
|
23611
|
23612
|
23613
|
26418
|
26419
|
26420
|
26421
|
26422
|
26423
|
26424
|
26878
|
26879
|
26880
|
26881
|
26882
|
26883
|
26884
|
27114
|
27125
|
27154
|
27155
|
27225
|
30362
|
30363
|
30364
|
30365
|
30366
|
30367
|
30368
|
30369
|
30370
|
30371
|
32384
|
32385
|
32386
|
32387
|
32388
|
32389
|
32390
|
32391
|
32392
|
32393
|
36198
|
36199
|
36200
|
36201
|
36202
|
36203
|
36204
|
36205
|
36206
|
36207
|
36208
|
36209
|
36210
|
36211
|
36213
|
36214
|
36215
|
36216
|
36217
|
36218
|
36219
|
36220
|
36221
|
36222
|
36223
|
36224
|
36225
|
36226
|
36227
|
36228
|
36229
|
36230
|
36231
|
36232
|
37681
|
37682
|
37683
|
37684
|
37685
|
37686
|
37687
|
37688
|
37689
|
37690
|
37691
|
37692
|
37693
|
37694
|
37696
|
37697
|
37698
|
37699
|
37700
|
37701
|
37702
|
37703
|
37704
|
37705
|
37706
|
38911
|
38912
|
38913
|
38914
|
38915
|
38916
|
38917
|
38918
|
38919
|
38920
|
38921
|
38922
|
38923
|
38924
|
38925
|
38926
|
38927
|
38928
|
38929
|
38930
|
38931
|
38932
|
38933
|
38934
|
38935
|
38936
|
39113
|
39114
|
39115
|
39116
|
39117
|
39118
|
39119
|
39120
|
39121
|
39122
|
39123
|
39626
|
39627
|
39628
|
39629
|
39630
|
39631
|
39632
|
39633
|
39634
|
39635
|
39636
|
39637
|
39638
|
41275
|
41276
|
41277
|
41278
|
41279
|
41280
|
41281
|
41282
|
41283
|
41284
|
41285
|
41286
|
41287
|
41288
|
41448
|
41449
|
41450
|
41783