Description
Julian Maurice
2011-09-16 10:29:29 UTC
Created attachment 5439 [details] [review] Patch Plugin for uploading files and link it into 856$u field Created attachment 5440 [details] [review] Patch Previous patch does not contain the good bug number To test: - Create the directory where uploaded files will go mkdir koha-tmpl/uploads - Set the 2 sysprefs uploadPath: "KOHA_ROOT/koha-tmpl/uploads" uploadWebPath: "/uploads" - Set the MARC framework to use "upload.pl" plugin for 856$u - Modify an existing biblio and use the plugin on this field - Save the modified biblio, verify that the url is well displayed at catalogue/detail.pl and try to download the file. P.S.: Don't forget to give rights on upload directory Hi,
Just some remarks...
1. Template name
The uploaded file for template are:
- upload.tt
- upload_delete_file.tt
'cataloguing/value_builder/upload.pl' use:
my $template_name = $result || $delete ? "upload_delete_file.tmpl" : "upload.tmpl";
So template extensions are bad (in perl script, I think).
2. A security consideration
in 'cataloguing/value_builder/upload.pl', in the sub "plugin", the template_name could be constructed with untrusted user parameter ($result).
3. A second security consideration (near same as above)
in 'cataloguing/value_builder/upload.pl', in the sub "plugin", when a file is requested for deletion, the code is:
> unlink("$upload_path/$result")
please consider that result come from untrusted input: $input->param('result'). So if $result contains something like "../../../" any file owned by http server could be deleted...
4. And a ore general security consideration (sorry, professional deformation :-) )
About permit uploading arbitrary file... how the filename is treated ? specially the extension ? Be careful with filename like 'test.pl' on a http server that served perl code (like Koha): this could result arbitrary command execution. (same problem for all server-side executable script configured on the server, like PHP, perl, python, cgi, SSI, ...).
The minimal security is to advertise the administrator who want enable this preference to have special http server configuration for the "upload_path" (in order to disable all script execution, SSI, or others, and don't use user configuration in the directory [like .htaccess])
5. Template remark for modified files (like MARCdetail.tt)
The code added is generally like:
[% IF ( subfiel.is_url ) %]
<a href="[% subfiel.marc_value %]">[% subfiel.marc_value |html %]</a>
[% ELSIF (subfiel.is_url ) %]
[% subfiel.marc_value %]
[% ELSE %]
[% subfiel.marc_value |html %]
[% END %]
Why a second ELSIF with the same condition than before ? (subfiel.is_url)
That's all for now !
Thanks.
A couple of things: How will this handle installations that have multiple instances? I'm quite wary of allowing files to be uploaded into a webserver accessible directory, it seems there's too much that can go wrong unless you're really careful (whitelisting extensions, etc.) My preferences would be an out-of-server-tree location, and download requests go through a program that does the actual transfer. The upload.pl has no copyright header, or useful perldoc. "plugin_parameters : other parameters added when the plugin is called by the dopop function" is meaningless. Created attachment 5764 [details] [review] Patch New rebased patch with some fixes. @Sébastien: 1/ Fixed. 2/ If you are talking about this line: my $template_name = $result || $delete ? "upload_delete_file.tt" : "upload.tt"; The '||' operator have higher precedence than '?' so in reality it's equivalent to my $template_name = ($result || $delete) ? "upload_delete_file.tt" : "upload.tt"; I added parentheses to avoid confusion 3/ '../' strings are now removed from filename ($result) 4/ Added chmod 0644 on created files to avoid script execution, and removed leading dot on filename to avoid uploading files like .htaccess 5/ err... copy/paste error. The second elsif condition is 'subfiel.is_file' @Robin: Not sure to understand your first question. What do you mean when you say "multiple instances"? You can set uploadPath to be external to the server tree location, and change the apache configuration to add a directory alias. Added copyright header and a brief perldoc section. Multiple instances is when you're running more than one koha instance on a server. Having the files accessible directly through apache strikes me as problematic. Adding an alias doesn't solve that really, and I don't think chmod 0644 will either. For a simple example, what if someone uploads a PHP file and you have mod_php enabled? The patch has a lot of 'warn's in it, looks like debug code. Most of the functions are undocumented, which is bad. Putting HTML/Javascript in .pl files is something that should really really be avoided, it's violating separation of concerns (I know that other parts of Koha have that, that upsets me too :) It should be pushed into a template. I note that you search through to find a free filename. I can't tell what the base name will be (or perhaps it's provided by the uploader), but that a) risks a (very unlikely) race condition, and b) could get slow if there are many files. Perhaps include a timestamp as well? If the OPAC is set so that it requires a valid login to access, then this will still allow those files to be accessible, which is bad. (In reply to comment #8) > Multiple instances is when you're running more than one koha instance on a > server. > > Having the files accessible directly through apache strikes me as problematic. > Adding an alias doesn't solve that really, and I don't think chmod 0644 will > either. For a simple example, what if someone uploads a PHP file and you have > mod_php enabled? can't it be solved just by an apache config (like "noexec" or something like that ?) > The patch has a lot of 'warn's in it, looks like debug code. Agreed, Julian, please update unconditional warns to "$debug and warn" > Most of the functions are undocumented, which is bad. If you're speaking of upload.pl, I fully agree, Julian, please add some documentation to this script. > Putting HTML/Javascript in .pl files is something that should really really be > avoided, it's violating separation of concerns (I know that other parts of Koha > have that, that upsets me too :) It should be pushed into a template. If you speak of upload.pl, it's how it's made for all plugins and I don't think this should be changed for this one. I agree we should improve the cataloguing plugin system (& probably completely rewrite the cataloguing tool in fact), but that's outside the scope of this patch. For consistency reason I think upload.pl should continue to have this structure & embeed code & html & javascript. > I note that you search through to find a free filename. I can't tell what the > base name will be (or perhaps it's provided by the uploader), but that a) risks > a (very unlikely) race condition, and b) could get slow if there are many > files. Perhaps include a timestamp as well? I don't understand what you mean here, so won't say anything. > If the OPAC is set so that it requires a valid login to access, then this will > still allow those files to be accessible, which is bad. True, but how could we avoid this ? should we obfuscate all files URL by providing a pass-through perl script? Something like : <a href="/cgi-bin/koha/opac-retrieve.pl?filename=blabla">Download file</a> and opac-retrieve.pl: 1- check for Opac requiring login 2- send the file Note that is would also solve your php uploaded concern right ? (Julian : if Robin confirms, ask stephane_n for what he did for Electre, it's that kind of thing) * I don't think expecting people to configure apache to do that is reasonable, many of them have enough trouble with the basic stuff. * Yeah, I saw how the other plugins worked after I wrote that, so I think that's OK (but should probably be fixed in the longer term.) * Having a pass-through script is ideal. I'd be inclined to have it be like: opac-retrieve.pl?id=abc123 where abc123 is a random string, or perhaps a hash of the file. This will prevent people enumerating ID numbers, and can reduce the chance of collisions compared to using a filename (also, if you use a hash, then uploading the same file multiple times will mean that only one version needs to be stored.) In the longer term, it also allows it to have fancier permissions or what not. And, it would remove the risk of executable files being uploaded. I think it would also allow multiple Koha instances to share one file store, if it used hashes, as they wouldn't be guessable. Updating Version : This ENH will be for Koha 3.8 Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday) $ git-bz apply 6874 Bug 6874 - File upload in MARC Patch Apply? [yn] y Applying: Bug 6874: Adds the ability to upload a file for a subfield /home/claire/dev/community_repo/koha/.git/rebase-apply/patch:329: trailing whitespace. return $string; /home/claire/dev/community_repo/koha/.git/rebase-apply/patch:555: trailing whitespace. <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> /home/claire/dev/community_repo/koha/.git/rebase-apply/patch:564: trailing whitespace. var doc = opener.document; /home/claire/dev/community_repo/koha/.git/rebase-apply/patch:611: trailing whitespace. <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> /home/claire/dev/community_repo/koha/.git/rebase-apply/patch:620: trailing whitespace. var doc = opener.document; error: patch failed: catalogue/detail.pl:104 error: catalogue/detail.pl: patch does not apply error: patch failed: installer/data/mysql/sysprefs.sql:318 error: installer/data/mysql/sysprefs.sql: patch does not apply error: patch failed: kohaversion.pl:16 error: kohaversion.pl: patch does not apply error: patch failed: opac/opac-detail.pl:223 error: opac/opac-detail.pl: patch does not apply Patch failed at 0001 Bug 6874: Adds the ability to upload a file for a subfield When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Patch-69WGpm.patch Forget it, this patch have conflicts better message with 3 way merge in git bz. $ git bz apply 6874 Bug 6874 - File upload in MARC Patch Apply? [yn] y Applying: Bug 6874: Adds the ability to upload a file for a subfield Using index info to reconstruct a base tree... <stdin>:329: trailing whitespace. return $string; <stdin>:555: trailing whitespace. <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> <stdin>:564: trailing whitespace. var doc = opener.document; <stdin>:611: trailing whitespace. <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> <stdin>:620: trailing whitespace. var doc = opener.document; warning: squelched 1 whitespace error warning: 6 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging C4/Biblio.pm Auto-merging catalogue/detail.pl CONFLICT (content): Merge conflict in catalogue/detail.pl Auto-merging installer/data/mysql/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl Auto-merging kohaversion.pl CONFLICT (content): Merge conflict in kohaversion.pl Auto-merging opac/opac-detail.pl CONFLICT (content): Merge conflict in opac/opac-detail.pl Failed to merge in the changes. Patch failed at 0001 Bug 6874: Adds the ability to upload a file for a subfield When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Patch-ExUoch.patch (In reply to comment #13) Created attachment 7114 [details] [review] Reviewed patch This patch add a new table 'uploaded_files' where are stored the filename and the md5 of the file as an id. This id is stored in corresponding MARC subfield (so maybe 856$u is not the right place to store it?) and is used to download the file with the new script opac-retrieve-file.pl. That looks good, my only quibble is about the use of MD5. There's no real reason to use it any more, and while it's not a particularly critical situation, it should probably still be avoided. See also: http://marc-stevens.nl/research/md5-1block-collision/ I've tested this, and encountered a number of problems. The following issues cause the patch to fail QA: 1. The syspref does not appear in the syspref interface. 2. There's no explanation of what the syspref should be set to. 3. This patch adds C4::UploadedFiles (notice the 's') with no indication of how it relates to C4::UploadedFile (notice the lack of the 's'). It's possible there is a legitimate reason for this, but there is absolutely no documentation of the new class. The following issues do not cause the patch to fail QA, but are worth noting: 4. Uploaded files can only be accessed when the display is set to normal mode. They do not work with XSLT mode. Perhaps the uploader could be modified to insert the entire URL so that it would work with XSLT displays too? 5. The patch does not rebase cleanly due to changes in the various installer files. You might as well fix this when you address issues 1-3 above, so that the next sign-off-er doesn't need to resolve the conflicts again. 6. The patch should not modify kohaversion.pl (again, easy to fix at sign off, but you might as well do it while fixing the above). 7. Instructions to activate the upload.pl plugin for 856 (or whatever) are not displayed on update. In terms of functionality in normal mode, when the 856$u is linked to the catalog.pl plugin, this is *very* cool. Created attachment 7980 [details] [review] File upload in MARC Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly -- What's new: - SHA-1 is now used to build the file hash instead of MD5 - uploadPath syspref is now displayed in the syspref interface (module Cataloging) - documentation added to C4::UploadedFiles - Value in MARC subfield is now the full URL instead of the id (needs OPACBaseURL syspref) - Patch rebased on current master - kohaversion.pl changes removed from the patch - updatedatabase.pl now prints a short explanation on how to make the plugin working - If 'uploadPath' directory contains other directories, a directory tree is displayed to allow the user to choose in which directory he wants to store his file Hi, I only do a "visual review" of the code, but just some suggestions... In "C4/UploadedFiles.pm" - line 147: > if( -f $file_path ) { > warn "Id $id not in database, but present in filesystem, do nothing"; > return; > } Use "-e" instead of "-f": the difference is when $file_path is an existing link. -f say "not a file", but -e "a node exists" (may be a directory, a plain file, a socket, a pipe, a special block device, a symbolic link...) - line 153: > unless(open $out_fh, '>', $file_path) { > warn "Failed to open file '$file_path': $!"; > return; > } a race condition is possible between the check of file existence and the open of file. You could use sysopen function instead of file-check + open. > use IO::Handle; > unless( sysopen($out_fh, $path, O_WRONLY|O_CREAT|O_EXCL) ) { > warn "Failed to open file '$file_path': $!"; > return; > } note: the file is created only if don't exist, due to O_CREAT|O_EXCL. There is not time between check and open because all is done by system kernel. In ".../cataloguing/value_builder/upload.tt" (and others templates): please use "html" filter. It is (generally) always to use it, a prevent XSS (here only "uploaded_file" seems to be build with user-input) > The file [% uploaded_file | html %] has been successfully uploaded. In the syspref message, perhaps it should be added that the uploadPath MUST NOT be a public directory, accessible from the webserver (because else, the server could be exposed to arbitrary code execution by evil uploader). Else, the patch seems promising. Thanks Marking failed QA because of the security issues noted by Frere Marie. Once those have been resolved, I look forward to testing this. Created attachment 8234 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly -- What's new: - Use sysopen instead of open - Use html filter on 'uploaded_file' var - Change the syspref description - Rebased on master Julian, I don't think this works right. I have uploadPath set to /tmp, and I get the following result: http://screencast.com/t/x9ihzYSpWC Uploading doesn't seem to work to any directory. Am I missing something? Last time I tested it, the uploading worked, and there weren't all those radio boxes. Regards, Jared Hello, We need more documentation about using syspref and directories (which one ? relative ? absolute ?). Jared, this hierarchy of radio buttons allow you to choose in which sub-directory you want to save your file. It normally displays only directories under uploadPath and where it have permission to write. So you should only have subdirectories of /tmp. I will take a look at this. Pierre, you must enter the absolute path to directory you want to save files. This can be any directory you want, but for security reasons it should not be reachable by webserver (so a directory outside of Koha source tree should be ok) Created attachment 8468 [details] [review] File upload in MARC Patch rebased on master. Jared, I can't reproduce your problem. I've set uploadPath to '/tmp' and can only choose '(root)' (upload works) I created a directory /tmp/test, make it writable by www-data (apache user), then choice for this directory appears and upload works in both directories. Can you make another try, and give me more infos on what happens? I've tried to reproduce the bug described by Jared as well, but it did work correctly for me. Using /tmp, I had (root) and the only www-data writable directory (and it's subdirectories). Non www-data writable directories were hidden. Created attachment 10664 [details] [review] File upload in MARC Rebased on master Created attachment 10673 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> The problem I reported earlier seemed to have something to do with setting the syspref *prior* to the revised patches. I tested it on an installation which had not had the syspref set previously, and it worked great. Created attachment 10863 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> http://bugs.koha-community.org/show_bug.cgi?id=6784 (In reply to comment #29) > The problem I reported earlier seemed to have something to do with setting > the syspref *prior* to the revised patches. I tested it on an installation > which had not had the syspref set previously, and it worked great. QA comment, passing QA... all perl-critic, perl syntax, and tt-template tests pass OK $ koha-qa.pl starting perlcritic-progressive tests... OK starting 'perl -c' syntax tests... OK starting xt/tt_valid.t tests... OK starting xt/author/vaild-template.t tests... OK rebased patch on 60b6c187c46614431f109512c740a3bcf077f342 I think i've found a problem with this patch. When testing, I uploaded a file, then tried to display it, I got: not found (my URL was: /cgi-bin/koha/opac-retrieve-file.pl?id=796851af7a586fad6a19883c8ad714fa116ee9ed) Investigating in /tmp/bz6874 I saw what can be the origin of the bug: -rw-rw-r-- 1 paul paul 41461 juil. 18 11:11 Capture d'écran - 06072012 - 14:04:07.png I uploaded a file called "Capture d'écran - 06072012 - 14:04:07.png" => the encoding on the filesystem is wrong, that's probably why it's failing... You should either fix the encoding or use Text::Unaccent to remove accents sorry, failed QA Created attachment 10982 [details] [review] Bug 6874: Follow-up: fix filename encoding To test, try to upload a file with diacritics in its name and retrieve it by clicking on its link in catalogue/detail.pl page. I mark the bug "Failed QA" for a security problem (or perhaps two). in C4/UploadedFiles.pm, sub UploadFile, the regex for sanitize "../" is invalid. > if($filename =~ m#/(^|/)\.\.(/|$)# or $dir =~ m#(^|/)\.\.(/|$)#) { for $filename, a "/" was inserted at the beginning... so a filename like "../foo" is ok. That line should be corrected like: > if($filename =~ m#(^|/)\.\.(/|$)# or $dir =~ m#(^|/)\.\.(/|$)#) { And a second (possible) problem is using > $filename = decode_utf8($filename); *after* the previous check. I think it may be possible to pass the "../" check and with decode_utf8 obtain a "../". To be sure, it should be investigated, but it should be ok to have the "decode_utf8" *before* the "../" check. Created attachment 11098 [details] [review] Bug 6874: Follow-up: fix filename encoding and sanitization > for $filename, a "/" was inserted at the beginning... so a filename > like "../foo" is ok. Good catch! Fixed in this new patch > I think it may be possible to pass the "../" check and with > decode_utf8 obtain a "../". To be sure, it should be investigated, > but it should be ok to have the "decode_utf8" *before* the "../" > check. '.' and '/' are ASCII characters. I don't think any UTF-8 operations could change them. But, for safety, decode_utf8 is now before the '../' check The follow up does not fix filename encoding. When I try to upload a file with a diacritic in its name, the name is not changed. Also, as an aside, if you use the plugin after the field has already been linked to an uploaded file, you are asked if you want to delete the file, which is good, but -- after you delete the file -- you are not redirected to upload a new file. You have to close the window and reopen it by click on the plugin link. The follow-up does not intend to remove accents but to fix encoding. If the name of your uploaded file is the same as the original file, and does not contain "é"-like characters, it's ok. To be sure, you may try to download it. I'm going to submit a patch for the 2nd point. Created attachment 11634 [details] [review] Bug 6874: Allow to reupload a file immediately after deletion This patch adds a button "Upload a new file" after the "sucessful deletion" message to avoid user to close and reopen the window. (In reply to comment #37) > The follow-up does not intend to remove accents but to fix encoding. If the > name of your uploaded file is the same as the original file, and does not > contain "é"-like characters, it's ok. To be sure, you may try to download > it. I don't understand this- I thought that the filenames were supposed to be hashes of some sort? At the end, filenames are just filenames. I think this is needed if you want to access uploaded files 'manually' (not with Koha). This does not apply anymore. (In reply to comment #40) > At the end, filenames are just filenames. I think this is needed if you want > to access uploaded files 'manually' (not with Koha). I thought we switched to using SHA1 as identifier to avoid clobbering files with the same name? Created attachment 12355 [details] [review] Bug 6874: File upload in MARC Rebased on master Created attachment 12356 [details] [review] Bug 6874: Follow-up: fix filename encoding and sanitization Rebased on master Created attachment 12357 [details] [review] Bug 6874: Allow to reupload a file immediately after deletion Rebased on master Created attachment 12358 [details] [review] Bug 6874: Change filename construction method. Use <SHA-1>_<filename>. SHA-1 to avoid name clash, filename to make it easier to find a file by its name, when not using Koha (direct filesystem access). Jared, I finally choose to use both SHA-1 and name for filename. So we have the advantages of both sides, I think. Created attachment 14130 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 14131 [details] [review] Bug 6874: Follow-up: fix filename encoding and sanitization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 14132 [details] [review] Bug 6874: Allow to reupload a file immediately after deletion This patch adds a button "Upload a new file" after the "sucessful deletion" message to avoid user to close and reopen the window. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 14133 [details] [review] Bug 6874: Change filename construction method. Use <SHA-1>_<filename>. SHA-1 to avoid name clash, filename to make it easier to find a file by its name, when not using Koha (direct filesystem access). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 16343 [details] [review] Bug 6874: Follow-up: fix filename encoding and sanitization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16354 [details] [review] Bug 6874: Follow-up: fix filename encoding and sanitization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16355 [details] [review] Bug 6874: Allow to reupload a file immediately after deletion This patch adds a button "Upload a new file" after the "sucessful deletion" message to avoid user to close and reopen the window. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16356 [details] [review] Bug 6874: Change filename construction method. Use <SHA-1>_<filename>. SHA-1 to avoid name clash, filename to make it easier to find a file by its name, when not using Koha (direct filesystem access). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16357 [details] [review] QA follow-up return with explicit undef removed Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> QA comment: sorry, another failed QA, because of koha-qa/perlcritic complaining: * opac/opac-retrieve-file.pl FAIL critic FAIL Bareword file handle opened at line 35, column 5. See pages 202,204 of PBP. * cataloguing/value_builder/upload.pl FAIL critic FAIL Glob written as <...> at line 143, column 17. See page 167 of PBP. However, i've uploaded rebased patches with a 3rd perlcritic error fixed, I let you fix those 2. Once follow-up uploaded, switch back to signed off Created attachment 16370 [details] [review] Bug 6874: Follow-up for PBP fixes Created attachment 16693 [details] [review] [PASSED QA] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16694 [details] [review] [PASSED QA] Bug 6874: Follow-up: fix filename encoding and sanitization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16695 [details] [review] [PASSED QA] Bug 6874: Allow to reupload a file immediately after deletion This patch adds a button "Upload a new file" after the "sucessful deletion" message to avoid user to close and reopen the window. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16696 [details] [review] [PASSED QA] Bug 6874: Change filename construction method. Use <SHA-1>_<filename>. SHA-1 to avoid name clash, filename to make it easier to find a file by its name, when not using Koha (direct filesystem access). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16697 [details] [review] [PASSED QA] QA follow-up return with explicit undef removed Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> http://bugs.koha-community.org/show_bug.cgi?id=6874 Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16698 [details] [review] [PASSED QA] Bug 6874: Follow-up for PBP fixes Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16699 [details] [review] bug 6874 follow-up fix POD syntax Paul, isn't it kinda late where you are? ;) (In reply to comment #66) > Paul, isn't it kinda late where you are? ;) yep, but I cannot sleep, so I do some QA ;-) QA comment: I've added a follow-up to fix POD, it now passes koha-qa.pl QUESTION : trying to use the feature, I think I made something wrong or there's a problem. Here is what I do: * I've defined the UploadPath syspref to /tmp/bz6874 & created the directory * I've attached "upload.pl" plugin to 856$u * I've opened and edited an existing biblio * I click on the ... (on the right of 856) * I select a file, and put it in root of UploadPath * Koha says "The file logo2.jpg has been successfully uploaded." * I click "close" * i'm back to cataloguing, but the 856$u is empty shouldn't it contain the file I just uploaded ? Created attachment 16712 [details] [review] [PASSED QA] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16713 [details] [review] [PASSED QA] Bug 6874: Follow-up: fix filename encoding and sanitization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16714 [details] [review] [PASSED QA] Bug 6874: Allow to reupload a file immediately after deletion This patch adds a button "Upload a new file" after the "sucessful deletion" message to avoid user to close and reopen the window. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16715 [details] [review] [PASSED QA] Bug 6874: Change filename construction method. Use <SHA-1>_<filename>. SHA-1 to avoid name clash, filename to make it easier to find a file by its name, when not using Koha (direct filesystem access). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16716 [details] [review] [PASSED QA] QA follow-up return with explicit undef removed Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> http://bugs.koha-community.org/show_bug.cgi?id=6874 Created attachment 16717 [details] [review] [PASSED QA] Bug 6874: Follow-up for PBP fixes Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16718 [details] [review] [PASSED QA] bug 6874 follow-up fix POD syntax Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16719 [details] [review] [PASSED QA] Bug 6874 follow-up jquery has moved The jquery library has been removed from themelang directory and put into /intranet-tmpl/lib/jquery This patch fixes the path QA comment (continued) * the problem was caused by the move of the jquery library. Fixing the jquery path in http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16719 solves the problem, works well now passed QA ! This feature is lacking unit tests for the new C4::UploadedFiles, and at first glance does not look Plack-safe: my $upload_path = C4::Context->preference('uploadPath'); is at a file level, but $upload_path is used in subroutines. (In reply to comment #77) > and at > first glance does not look Plack-safe: > my $upload_path = C4::Context->preference('uploadPath'); > is at a file level, but $upload_path is used in subroutines. I QAed it using Plack, and it worked well. (In reply to comment #78) > (In reply to comment #77) > > and at > > first glance does not look Plack-safe: > > my $upload_path = C4::Context->preference('uploadPath'); > > is at a file level, but $upload_path is used in subroutines. > > I QAed it using Plack, and it worked well. That's good. The QA failure was due to the missing unit tests. Created attachment 17368 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Created attachment 17369 [details] [review] Bug 6874: Add dependency to Test::CGI::Multipart Oops. Forgot to reset status. Created attachment 18767 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18768 [details] [review] Bug 6874: Follow-up: fix filename encoding and sanitization Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18769 [details] [review] Bug 6874: Allow to reupload a file immediately after deletion This patch adds a button "Upload a new file" after the "sucessful deletion" message to avoid user to close and reopen the window. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18770 [details] [review] Bug 6874: Change filename construction method. Use <SHA-1>_<filename>. SHA-1 to avoid name clash, filename to make it easier to find a file by its name, when not using Koha (direct filesystem access). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18771 [details] [review] Bug 6874: QA follow-up return with explicit undef removed Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18772 [details] [review] Bug 6874: Follow-up for PBP fixes Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18773 [details] [review] bug 6874 follow-up fix POD syntax Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18774 [details] [review] Bug 6874 follow-up jquery has moved The jquery library has been removed from themelang directory and put into /intranet-tmpl/lib/jquery This patch fixes the path Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18775 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18776 [details] [review] Bug 6874: Add dependency to Test::CGI::Multipart Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18777 [details] [review] Bug 6874: Add dependency to Test::CGI::Multipart Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 (In reply to comment #83) > New cataloging plugin upload.pl and new system preference 'uploadPath'. ... > uploadPath is the absolute path on the server where the files will be > stored. It's possible to have a hierarchy of directories under this path This should not be a system preference. It should be something that's set in the koha-conf.xml. Library staff should not be modifying it, as they can't ensure that directories are set up with the right permissions. If you're running a hosted setup without permission separation, you could upload into someone else's koha space (probably not terrible, but not ideal.) Finally, it's hard to do an automated configuration of a system preference, and it's easy to do one for koha-conf.xml. > This should not be a system preference. It should be something that's set in
> the koha-conf.xml. Library staff should not be modifying it, as they can't
> ensure that directories are set up with the right permissions.
This is the same for all other system preferences that contain a path. So why
this one is different?
Moreover, I don't think a preference for a cataloguing plugin should be in
koha-conf.xml (its place is not in global system preferences either but there is
no other place)
I'm not opposed to move this pref in koha-conf.xml, I just want to understand :)
Also I would like to have someone else's opinion before writing a patch.
(In reply to comment #95) > > This should not be a system preference. It should be something that's set in > > the koha-conf.xml. Library staff should not be modifying it, as they can't > > ensure that directories are set up with the right permissions. > This is the same for all other system preferences that contain a path. So why > this one is different? At 1st glance, I had the same reaction. But there is a difference : writing ! Other systempreferences only require reading (opacusercss for example), this one will write, and there is no other syspref in this case I think. > Moreover, I don't think a preference for a cataloguing plugin should be in > koha-conf.xml (its place is not in global system preferences either but > there is no other place) I hesitate... what I think is that this preference don't need to be setup during installation, because it's an additional feature that most libraries won't use. (like for ldap for example : there are default values, but the koha-conf.xml must be edited manually if you want to enable it) > I'm not opposed to move this pref in koha-conf.xml, I just want to > understand :) > Also I would like to have someone else's opinion before writing a patch. my conclusion would be: * move the uploadPath to koha-conf.xml * add <uploadPath><uploadPath> (without any value) to default koha-conf * add a disclaimer on the plugin page if uploadPath is empty, saying something like "WARNING : your server is not properly setup, it won't work. uploadPath must be set in koha configuration file" (wait for another feedback before writing the patch) PS: will this patch make it's way one day ? :\ (In reply to comment #95) > This is the same for all other system preferences that contain a path. So why > this one is different? Most of those are URL paths and such like. The only exception I can think of (though there are probably others) is the XSLT, and that is designed to be user editable. > Moreover, I don't think a preference for a cataloguing plugin should be in > koha-conf.xml (its place is not in global system preferences either but > there is > no other place) Well, it's a preference for where your things get uploaded on the server, it's not something that's likely to change during normal configuration I think. And it's pointless being exposed to library staff as they're typically not sysadmins. Created attachment 18891 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC Rebased on master Squash of 4 signed-off patches Created attachment 18892 [details] [review] [SIGNED-OFF] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 18893 [details] [review] [SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Created attachment 18894 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml The last patch introduces a subroutine in t/lib/Mocks/Context.pm that is also introduced by Bug 10298. But there are quite similar and resolving the conflicts will be very easy. Created attachment 20059 [details] [review] [SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 20060 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Unfortunately the fourth patch, "Move uploadPath syspref to koha-conf.xml" will not apply at all because of missing sha1 information for sysprefs.sql. Could you please rebase this patchset and reupload? I'm attempting to backport to 3.12.x Okay, I accidentally messed up and got what I think is this working for master. However, I have some concerns. misc/strip_value_from_tag.pl The patches containing this were obsoleted (see first 5 ones, I think). Is this module not useful? t/lib/Mocks/Context.pm This exists in part in the non-obsoleted patches ([SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml), but I think something substituting for t::lib::Mocks would be sufficient based on my reading of the code. 3-5 lines of code are missing in the patch, so I can't reconstruct what I think is the original file. I hunted through all the obsoleted patches too. I would very much prefer the original developer/author to rebase their patch, but am willing to provide what I have reconstructed. I know that my colleagues have asked for this patch, and so I'll make sure to back port to 3.12.x too. Created attachment 20779 [details] [review] Reconstructed attempt for the upload plugin feature. This patch should obsolete all the other stuff, but since I am not the original author, but I really want this patch in, I have done my best to reconstruct it. I decided not to obsolete, in the case I that missed something. As far as I can tell: - configure the koha-conf.xml with the appropriate values - make sure the directories exist and have the correct permissions <pluginsdir>/home/mtompset/koha-dev/var/lib/plugins</pluginsdir> <enable_plugins>1</enable_plugins> <upload_path>/home/mtompset/kohaclone/koha-tmpl/uploads</upload_path> - link 856$u to the upload.pl plugin. - test. (In reply to M. Tompsett from comment #109) > As far as I can tell: > - configure the koha-conf.xml with the appropriate values > - make sure the directories exist and have the correct permissions > <pluginsdir>/home/mtompset/koha-dev/var/lib/plugins</pluginsdir> > <enable_plugins>1</enable_plugins> Please provide an updated test plan that reflects your new dependency on Koha's plugin system. Created attachment 21432 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC Rebased on master Created attachment 21433 [details] [review] [SIGNED-OFF] Bug 6874: QA follow-up Rebased on master Created attachment 21434 [details] [review] [SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles Rebased on master Created attachment 21435 [details] [review] [SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml Rebased on master Created attachment 21436 [details] [review] Bug 6874 - File upload in MARC - QA Followup Rebased on master (In reply to M. Tompsett from comment #107) > I would very much prefer the original developer/author to rebase their > patch Done ;-) Comment on attachment 20779 [details] [review] Reconstructed attempt for the upload plugin feature. Original author rebased patches. No need for my intermediate attempt. There is a missing semi-colon in the first attachment for the kohastructure.sql YIKES! " ) ENGINE=InnoDB DEFAULT CHARSET=utf8;" missing from above the uploaded_files table in kohastructure.sql Comment on attachment 21432 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC Review of attachment 21432 [details] [review]: ----------------------------------------------------------------- kohastructure.sql - missing ; on the DROP and the ") ENGINE=..." line was hijacked by the new table, but not replaced. This was obvious when I was attempting to install a UNIMARC installation to hopefully trigger cases 6, 6a, and 6b in C4::Biblio. Comment on attachment 21436 [details] [review] Bug 6874 - File upload in MARC - QA Followup Review of attachment 21436 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt @@ +4,4 @@ > <head> > <title>Upload plugin</title> > <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > + <script type="text/javascript" src="[% interface %]/lib/jquery/jquery.js"></script> This is correct here, but the upload_delete_file.tt needs the same change. Comment on attachment 21432 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC Review of attachment 21432 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/kohastructure.sql @@ +3202,5 @@ > +-- > +-- Table structure for table uploaded_files > +-- > + > +DROP TABLE IF EXISTS uploaded_files This line is missing a semi-colon. @@ +3211,1 @@ > ) ENGINE=InnoDB DEFAULT CHARSET=utf8; This line should be copied to 3201. ::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload_delete_file.tt @@ +3,5 @@ > +<html xmlns="http://www.w3.org/1999/xhtml"> > +<head> > + <title>Upload plugin</title> > + <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > + <script type="text/javascript" src="[% themelang %]/lib/jquery/jquery.js"></script> This should be [% interface %] not, [% themelang %] $ giz bz apply 6874 Had to fix up installer/data/mysql/updatedatabase.pl Had to install new dependency for this. $ koha_perl_deps.pl -m -u Listed new requirement (hence revised PerlDependencies.pm checked) $ sudo apt-get install libtest-cgi-multipart-perl Did the database upgrade by starting staff client. $ prove -v t/db_dependent/UploadedFiles.t everything passed. $ ~/qa-test-tools/koha-qa.pl -v 2 -c 10 (I forgot how many patches were applied, so I just said 10) No problems with this set of patches. Noticed POD updates unrelated to patches (C4::Biblio) Probably required to get C4::Biblio to pass QA tool tests. New files include: C4/UploadedFiles.pm cataloguing/value_builder/upload.pl koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload_delete_file.tt opac/opac-retrieve-file.pl t/db_dependent/UploadedFiles.t Did a opac-detail view, which means the "use C4::Biblio" isn't broken. There are 1,2,3,4,5,6,6a,6b,7,8,9 changes in C4::Biblio $ perldoc C4::Biblio This triggers 2,7,8,9 Refresh of opac-detail page triggers 3,4 Home -> administration -> Marc frameworks -> BKS framework structure -> tag 856 subfield structure -> edit subfields contraint -> advanced -> plugin = upload.pl Attempted to use plug in without configuring koha-conf.xml Correctly gave feedback. $ mkdir ~/uploads $ sudo chown www-data.www-data ~/uploads add in <upload_path>/home/mtompset/uploads</upload_path> Link a file into some biblio entry. Go to opac detail for it: http://demo.library.kohasystem.ca/cgi-bin/koha/opac-detail.pl?biblionumber=14206 Triggers 1,3,4,5 In order to trigger 6, 6a, and 6b, GetMarcUrls needs to be called with something other than MARC21. I couldn't figure out how to do that. I'm attempting a UNIMARC installation. I used https://www.dropbox.com/s/idw98d7jiy5e5xd/dump_unimarc.zip as mentioned by Mathieu Saby in his August 20th email "pending bugs about UNIMARC marc flavor". Linked upload.pl uploaded a dummy file for a known biblio record. went to opac and did an opac detail view of it. (6, 6a) unlinked upload.pl refreshed opac detail page (6, 6b) testing to continue later. Hi Mark, could you describe a bit what you tested on the pages? The numbers make your test plan quite cryptic to read. Comment on attachment 21432 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC Review of attachment 21432 [details] [review]: ----------------------------------------------------------------- ::: C4/Biblio.pm @@ +37,4 @@ > use C4::Charset; > use C4::Linker; > use C4::OAI::Sets; > +use C4::UploadedFiles; This is C4::Biblio Change 1. @@ +1908,4 @@ > > =head2 GetMarcUrls > > + $marcurls = GetMarcUrls($record,$marcflavour,$frameworkcode); This is C4::Biblio Change 2. @@ +1919,5 @@ > + my ( $record, $marcflavour, $frameworkcode ) = @_; > + > + my $tagslib = &GetMarcStructure(1, $frameworkcode); > + my $urltag = '856'; > + my $urlsubtag = 'u'; This block is C4::Biblio Change 3. @@ +1924,3 @@ > > my @marcurls; > + for my $field ( $record->field($urltag) ) { This is C4::Biblio Change 4. @@ +1927,5 @@ > my @notes; > for my $note ( $field->subfield('z') ) { > push @notes, { note => $note }; > } > + my @urls = $field->subfield($urlsubtag); This is C4::Biblio Change 5. @@ +1956,3 @@ > $marcurl->{'part'} = $s3 if ($link); > $marcurl->{'toc'} = 1 if ( defined($s3) && $s3 =~ /^[Tt]able/ ); > } else { This begins C4::Biblio Change 6. @@ +1956,5 @@ > $marcurl->{'part'} = $s3 if ($link); > $marcurl->{'toc'} = 1 if ( defined($s3) && $s3 =~ /^[Tt]able/ ); > } else { > + if ($tagslib->{ $urltag }->{ $urlsubtag }->{value_builder} eq "upload.pl" > + and $url =~ /id=([0-9a-f]+)/) { Inside this IF is C4::Biblio Change 6a. @@ +1963,5 @@ > + $marcurl->{'linktext'} = $field->subfield('2') > + || C4::Context->preference('URLLinkText') > + || $file->{filename}; > + $marcurl->{'MARCURL'} = $url; > + } else { Inside this ELSE is C4::Biblio Change 6b. Comment on attachment 21433 [details] [review] [SIGNED-OFF] Bug 6874: QA follow-up Review of attachment 21433 [details] [review]: ----------------------------------------------------------------- ::: C4/Biblio.pm @@ +1446,4 @@ > =head2 GetMarcPrice > > return the prices in accordance with the Marc format. > + I ignored white space while labeling changes. @@ +3395,5 @@ > =head2 CountBiblioInOrders > > +=over > + > +=item $count = &CountBiblioInOrders( $biblionumber); This is C4::Biblio change 7. @@ +3419,5 @@ > =head2 GetSubscriptionsId > > +=over > + > +=item $subscriptions = &GetSubscriptionsId($biblionumber); This is C4::Biblio change 8. @@ +3443,5 @@ > =head2 GetHolds > > +=over > + > +=item $holds = &GetHolds($biblionumber); This is C4::Biblio change 9. I hope that clarifies the complete testing of the C4::Biblio changes that I have done. Hi Mark, actually I was more thinking about some 'non-programmer' notes about testing the actual functionality :) Fear not. :) I'll be getting to less programmer oriented testing instructions. The programmer oriented instructions are a matter of proving that the code changes introduced do not cause any problems by triggering all the code points. Now, back to testing... Created attachment 21489 [details] [review] Bug 6874 - Patch kohastructure.sql issues follow up Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Set up VM from scratch (include qa-test-tools, git-bz [fishsoup branch], koha origin/master branch) $ git checkout -b bug_6874 origin/master $ git bz apply 6874 $ perl Makefile.PL $ make $ make test $ make install Remember to set up some useful data after you get a working Koha. :) Configure patch - edit koha-conf.xml and make sure to fill in <upload_path></upload_path> (e.g. <upload_path>/home/mtompset/uploads</upload_path>) - make sure to give that directory correct permissions (e.g. sudo chown www-data.www-data /home/mtompset/uploads) - attach the plug-in to the 856$u subfield (e.g. - log in to staff client - click 'Koha administration' - click 'MARC bibliographic framework' - click 'MARC structure' for a framework - Find the 856 tag - click 'Subfields' - Find the 'u' subfield - click 'Edit' - click 'Other options (choose one)' - select 'upload.pl' in the Plugin drop-down - click 'Save changes') You will want to test in both MARC21 and UNIMARC systems. $ ./koha_perl_deps -m -u $ perldoc C4::Biblio $ perldoc C4::UploadedFiles This will trigger all the functions in C4::UploadedFiles $ prove -v t/db_dependent/UploadedFiles.t Once you have a bilio with 856$u linked up to a file, go to the same biblio on the OPAC. (e.g. http://demo.library.kohasystem.ca/cgi-bin/koha/opac-detail.pl?biblionumber=14206) Then unlink the upload.pl plugin and refresh the OPAC. This latter step is necessary to test a secondary code point under UNIMARC. In the staff client, find a biblio. Add it to your cart. Click the cart at the top of the page. This will test opac-retrieve-file.pl fully: While at the OPAC detail page for the item added - Click the 'Online resources:' URL. 1) It should work. - Delete the file from the filesystem. (e.g. sudo rm /home/mtompset/uploads/*) - Click the 'Online resources:' URL. 2) It should fail. - Open up MySQL and delete the record from the uploaded_files table. 3) It should fail. - Make sure to clear the 856$u entry for that biblio manually and save. opac-detail.pl change is tested just by viewing the details of the biblio with an 856$u link. opac-basket.pl change is tested by adding the item to your cart and clicking the "Cart" button. To test the template files, it is a matter of doing a lot of upload, delete, upload, whatever type tests in the staff client. This has not been fully tested, but there are some issues that I want to test to the point of being able to say what is wrong and how I was expecting it to be handled. I'm beginning to run short of testing time today again. However, I am pretty confident that I have tested the following thoroughly: C4/Biblio.pm C4/Installer/PerlDependencies.pm C4/UploadedFiles.pm basket/basket.pl etc/koha-conf.xml installer/data/mysql/kohastructure.sql installer/data/mysql/updatedatabase.pl opac/opac-basket.pl opac/opac-detail.pl opac/opac-retrieve-file.pl t/db_dependent/UploadedFiles.t Patches with updatedatabase.pl in them need to be rebased. I have already attached an extra patch to correct issues in kohastructure.sql I will continue my testing later of: catalogue/MARCdetail.pl catalogue/detail.pl cataloguing/value_builder/upload.pl koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload_delete_file.tt initial state: blank, no records in uploaded_files, no files on filesystem. click plug in icon - expected window pops up. click submit query with nothing clicked. - nothing happens, refreshes as expected. click submit query with root selected - nothing happens, refreshes as expected. click browse, but cancel - nothing happens as expected click browse and choose a file - file name appears as expected click submit query with root not selected - "Error: Failed to upload file. See logs for details." current state: file is uploaded, but database is blank. checked error logs jquery.js issue and "Use of uninitialized $dir" error. click Close. delete file from file system. make sure the jquery.js line in upload.tt has interface not themelang click plug in icon - expected window pops up click browse and choose a file - file name appears as expected click submit query with root not selected - "Error: Failed to upload file. See logs for details." ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ FIX REQUESTED: I was expecting to have a refresh of the page with perhaps a "You forgot to select a location" in red, just above/beside/below the "Choose where to upload file". I was also expect to NOT have the file uploaded. This disconnect between the file existing and the record not existing in the database leads to weird problems. ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ click Close. delete file from file system. click plug in icon - expected window pops up click browse and choose a file - file name appears as expected click root - NOTE: I was lazy and didn't create subdirectories in this testing. click submit query - happy it worked kind of message click close - URL is filled in nicely. Not finished fully testing, but I thought I'd mention this problem since I found it. Comment on attachment 21432 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC Review of attachment 21432 [details] [review]: ----------------------------------------------------------------- ::: cataloguing/value_builder/upload.pl @@ +50,5 @@ > + var id = document.getElementById(index).value; > + if(id.match(/id=([0-9a-f]+)/)){ > + id = RegExp.\$1; > + } > + window.open(\"../cataloguing/plugin_launcher.pl?plugin_name=upload.pl&index=\"+index+\"&id=\"+id, 'upload', 'width=600,height=400,toolbar=false,scrollbars=no'); var newin=... (this line) newin.focus(); ::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt @@ +3,5 @@ > +<html xmlns="http://www.w3.org/1999/xhtml"> > +<head> > + <title>Upload plugin</title> > + <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > + <script type="text/javascript" src="[% themelang %]/lib/jquery/jquery.js"></script> This should be interface, not themelang. Reading through the comments of M.Tompset I think this is failed QA. There are issues and further testing should be postponed until those are fixed. Julian, could you please take a look and provide follow-ups where needed? Created attachment 21543 [details] [review] Bug 6874 - Focus and jquery.js fix Clicking the plug in show cause the popup window to refocus. The patch to the jquery.js script was wrong. Changed [% themelang %] to [% interface %]. Seeing as I can't sign off on my own patches. Could you squash these into a single patch, re-test, and re-post, Julian? Thanks. I'll continue testing once reposted. :) As they haven't been signed off you could squash your patches into one - but don't squash them with Julian's. Created attachment 21546 [details] [review] Bug 6874 - kohastructure.sql & jquery.js fix with refocus Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. Squashed my patches together as per Katrin Fischer's request. Created attachment 21635 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. First patch needs to be rebased. uploaddatabase.pl patch issues. Still going ahead with testing. Testing with Javascript enabled. click plugin icon click Submit Query with nothing filled in. - Was only told about the file location not filled in. I would expected to be told neither were filled in, if I get told this. Selected a file click Submit Query - expected to be told location not filled it. As expected. closed pop up window. click plugin icon click location, but not select a file. - Refreshed blank. A valid behaviour, but confusing since now we have the file location validation. selected a file and chose a location click Submit Query - File uploaded as expected. click close - URL is filled in nicely, as expected. click plugin icon - Would you like to delete window pops up. Okay behaviour. click cancel - windows closes no changes as expected. click plugin icon click delete - told it was deleted successfully, as expected. URL is blanked, as expected. click close repeat steps to get back to delete popup and click delete upload a new file -- after a few cycles of trying to repeat the nothing, browse, root, browse & root selections, I realized I would really like a way to clear the file selected. Though, perhaps this isn't the real need. I did try testing with Javascript off, but it is nearly impossible to work with different browers and scripting off. I was able to trigger your non-javascript red message under IE 10, but not Firefox 24 since the plugin icon refused to popup a window. The problem with it disabled was that then the 'close' doesn't work with javascript disabled, and the URL isn't filled in, but the file is uploaded. On a positive note it continues to behave as if there was no file uploaded, which is what is expected from a blank 856$u field. However, there are table entries and files sitting on the server now. SUGGESTIONS FOR IMPROVEMENTS: - If we validate the location, could we also validate if the filename is set? - Could we add a 'Cancel' which just closes the window to this? Not just a 'Submit Query' button. - Could we change 'Submit Query' to 'Upload File'? - When nothing was selected, got a refresh, which is okay, but strange in light of validation message logic that was added. - Javascript off generates bizarre (but not unstable) state of file uploaded and table pointing to it, but textbox empty, under IE 10. I couldn't even open the new popup windows under FireFox 24. I think if we document this requires Javascript, this is passable. This is getting really close to useable. Thank you for working on this, Julian. :) Created attachment 21798 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 21799 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Comment on attachment 21546 [details] [review] Bug 6874 - kohastructure.sql & jquery.js fix with refocus squashed my patches into one on the end. Created attachment 21800 [details] [review] [SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 21801 [details] [review] [SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 21802 [details] [review] [SIGNED OFF] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 21803 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Test Plan for my patch: - click plugin icon (after all the necessary configuration and setup). No red message is given. - Turn off javascript - click 'Upload file' Should get pretty red message about both things not selected. - go through combinations of one or the other, but not both, and clicking upload Should get pretty red message each time with appropriate message. - Turn on javascript - go through combinations of none, one, and then the other, javascript alerts should warn appropriately. - after linking the 856$u field to a preset URL, click the plugin icon, and delete the file. Comment on attachment 21434 [details] [review] [SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles Review of attachment 21434 [details] [review]: ----------------------------------------------------------------- ::: C4/Installer/PerlDependencies.pm @@ +692,5 @@ > + 'Test::CGI::Multipart' => { > + 'usage' => 'Tests', > + 'required' => '0', > + 'min_ver' => '0.0.3', > + }, This needs a rebase now. :( I also have conflict in sysprefs: CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql I'm applying on 3.2.x branch, but since it's removed in another patch, it would probably be better to drop all changes to sysprefs from this patchset instead of modifying it and than reverting it back. Shoot... My librarian colleague came up with a scenario that breaks this. 1) Upload and set everything for a 856$u. 2) Clone the subfield. 3) Click the plugin icon. This is where the problem comes in, because now the two subfields you can end up deleting the file for the current 856$u, but the other 856$u still has a reference to the deleted file in it. Any ideas? While we're waiting for a design suggestion to solve the test case I posted, could the original author please rebase his first two signed off patches? Thank you. :) I have some other related questions: - should validate URLs, since not all 856$u urls will be uploaded file urls. - external urls will generate the same apparent error deleting as the cloning scenario I mentioned in comment #154. - is telling a user to check errors logs reasonable? They may not have shell access to the system. - If a valid external url is detected, then a different response is needed than if we discover a url to a deleted file. - How will the user know to delete the text field before clicking the plugin icon in order to continue onto the upload screen to put a different file in place? The signed off patches need a rebase. Feedback on these questions would be useful. I'm leaving the status at In Discussion, because even if my patch was signed off, the problem with respect to external urls and deleted files, due to a clone, would still exist. And that seems like enough to merit not just running for QA's approval. (In reply to M. Tompsett from comment #154) > Shoot... My librarian colleague came up with a scenario that breaks this. > > 1) Upload and set everything for a 856$u. > 2) Clone the subfield. > 3) Click the plugin icon. > This is where the problem comes in, because now the two subfields you can > end up deleting the file for the current 856$u, but the other 856$u still > has a reference to the deleted file in it. I think it's not a big deal: the user just get an invalid URL. That's exactly the same thing as when the librarian fills an invalid URL. Also note that the script misc/cronjobs/check-url-quick.pl will detect the problem. (In reply to M. Tompsett from comment #156) > I have some other related questions: > - should validate URLs, since not all 856$u urls will be uploaded file urls. Not sure I understand well the question, but isn't misc/cronjobs/check-url-quick.pl what you're asking ? > - external urls will generate the same apparent error deleting as the > cloning scenario I mentioned in comment #154. see my previous answer. > - is telling a user to check errors logs reasonable? They may not have shell > access to the system. No, it isn't > - If a valid external url is detected, then a different response is needed > than if we discover a url to a deleted file. I think check-url-quick.pl achieve the goal, and it's better because it detects when an URL becomes invalid at any time. > - How will the user know to delete the text field before clicking the plugin > icon in order to continue onto the upload screen to put a different file in > place? I don't understand this one. > The signed off patches need a rebase. Feedback on these questions would be > useful. I'm leaving the status at In Discussion, because even if my patch > was signed off, the problem with respect to external urls and deleted files, > due to a clone, would still exist. And that seems like enough to merit not > just running for QA's approval. Are my answer OK and is it back to go to signed-off ? (I have not tested the patches still applies cleanly) (In reply to Paul Poulain from comment #158) > (In reply to M. Tompsett from comment #156) > > I have some other related questions: > > - should validate URLs, since not all 856$u urls will be uploaded file urls. > Not sure I understand well the question, but isn't > misc/cronjobs/check-url-quick.pl what you're asking ? No. if ( is this one of our uploaded files ) { continue processing as expected } else { give big warning, and then go immediately to the upload, not the delete page } > > - is telling a user to check errors logs reasonable? They may not have shell > > access to the system. > No, it isn't Then we need to fix that. However, I think that is in part of the signed off patches. > > - If a valid external url is detected, then a different response is needed > > than if we discover a url to a deleted file. > I think check-url-quick.pl achieve the goal, and it's better because it > detects when an URL becomes invalid at any time. This isn't about an invalid URL. Frankly, I don't care about the validity as much as I care about the process flow. If I have a url in the 856$u field and click the plugin icon, I do not expect to attempt to delete an external url (not our hosted files that were uploaded). > > - How will the user know to delete the text field before clicking the plugin > > icon in order to continue onto the upload screen to put a different file in > > place? > I don't understand this one. Test this. Upload a file. Clone the field. Now there are two fields pointing to the same uploaded file. Click the plug in icon, and delete the file, but then cancel. Now there are two fields: one blanked (because you just deleted), and the other with a dangling reference. Now, click the plugin icon for the non-empty 856$u field. It's going to prompt to delete. The user will click "Delete", because there is no way to go to upload otherwise, and an error will be generated (because the file was already deleted). And there is no meaningful feedback suggesting to clear the field before clicking the plugin icon, so the user will be frustrated because they can't seem to upload a file anymore, because they overlook the obvious idea of clearing the field. > Are my answer OK and is it back to go to signed-off ? > (I have not tested the patches still applies cleanly) If I have clarified what I mean, and you do test and see the process flow, then you'll understand that with a couple more validations the process can be made smoother. Created attachment 23588 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 23589 [details] [review] [SIGNED OFF] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Comment on attachment 21432 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC I rebased it. Created attachment 23590 [details] [review] [SIGNED OFF] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Comment on attachment 21433 [details] [review] [SIGNED-OFF] Bug 6874: QA follow-up I rebased this. Created attachment 23591 [details] [review] [SIGNED OFF] Bug 6874: Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Comment on attachment 21434 [details] [review] [SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles I rebased this. I've almost got something nice working. I fixed DelUploadedFile and added DanglingEntry. Hopefully something will be finished in 24 hours. :) Created attachment 23607 [details] [review] [SIGNED OFF] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 23608 [details] [review] [SIGNED OFF] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 23609 [details] [review] [SIGNED OFF] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 23610 [details] [review] [SIGNED OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 23611 [details] [review] [SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 23612 [details] [review] [SIGNED OFF] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 23613 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles Created attachment 26418 [details] [review] [SIGNED OFF] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26419 [details] [review] [SIGNED OFF] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26420 [details] [review] [SIGNED OFF] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26421 [details] [review] [SIGNED OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26422 [details] [review] [SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26423 [details] [review] [SIGNED OFF] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26424 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. I rebase'd to master. The last patch still needs sign off. :) Someone asked about how to test and why I have my name on so many things. I thought perhaps posting this might help people find the test plan and some background details. > Also, why many commits have only your sign? > Why is your sign at all? Because I didn’t write all the first part. I tested it, because we wanted the same functionality. > It's also difficult to test something without a clear test plan. Yes, yes, it is. I started... Comment 109. Comment 123. Katrin makes similar comments regarding the test plan being difficult or lacking in Comment 128. Comment 131. Comment 142. Which all cumulated into what you are looking for: Comment 175 I only wrote the last patch, which is not signed off. And in my mind, it is the only thing stopping this from reaching QA. > Could you add a description of what to test, before and after patch Before, Koha does not host files, and 856$u needs to point to some external source. After, Koha’s tag editor lets you upload a file and have a URL automatically generated to point to that newly hosted file. I hope this helps someone decide to take the plunge and test this. :) (In reply to M. Tompsett from comment #184) > I hope this helps someone decide to take the plunge and test this. :) Looking back at the history of this patch, I definitely admire the perseverance of Julian and Mark to get this into Koha. Created attachment 26878 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26879 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26880 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26881 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26882 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26883 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26884 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Given that master has had things added to it. I rebased against master. The only thing that really needs sign off is the last patch (kohastructure.sql, jquery.js, refocus, and more). You can find a mega test plan in comment 175. Hi, testing Found a problem, at first I have an empty OPACbaseURL, file upload worked. Then I set base url, now when I upload a file, the original file name is not appended at the end of the 'new' name, I got something like "/cgi-bin/koha/opac-retrieve-file.pl?id=775f48aaf5f7a44fa534dae4e7dc876ea7191115", with a missing "_filename.ext" But file is created, and I can delete it, but url does not work Other little things, koha-qa FAIL C4/UploadedFiles.pm FAIL cataloguing/value_builder/upload.pl ... FAIL opac/opac-retrieve-file.pl with error FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9) Just an old version of licence on files, update http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Created attachment 27114 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. (In reply to Bernardo Gonzalez Kriegel from comment #194) So just to clarify. You tried to use this without OPACBaseURL set in the first place? And it seemed to upload fine. Then, you set the OPACBaseURL, and after logging out and back in, when you tried to check on the file it busted? I just patched the license stuff. I'll attempt to recreate your problem tomorrow. Created attachment 27125 [details] [review] Bug 6874 - Fix file URL This patch prepends 'http://' to file upload URL, because this patch series assumes (wrongly) that OAPCBaseURL is filled WITH that :) (In reply to M. Tompsett from comment #196) > (In reply to Bernardo Gonzalez Kriegel from comment #194) > > So just to clarify. You tried to use this without OPACBaseURL set in the > first place? And it seemed to upload fine. Then, you set the OPACBaseURL, > and after logging out and back in, when you tried to check on the file it > busted? > > I just patched the license stuff. I'll attempt to recreate your problem > tomorrow. Mark, found it! Your OPACBaseURL must be filled with "http://", that's it And it's wrong, or at last it's not suggested when you fill the preference ("The OPAC is located at http:// ... ") Look my followup, use it or not, fix as you like and squash with previous patch Regards :) (In reply to Bernardo Gonzalez Kriegel from comment #197) > Created attachment 27125 [details] [review] [review] > Bug 6874 - Fix file URL > > This patch prepends 'http://' to file upload URL, > because this patch series assumes (wrongly) that > OAPCBaseURL is filled WITH that :) /me points to https:// and frowns. Perhaps we need a dependency on bug 5010 to solve OPAC http vs. https issues? (In reply to Robin Sheat from comment #199) > > /me points to https:// and frowns. But... is strange to put OPAC under https It's possible of course, but my followup only inserts it if it's not there. It can be modified to recongnize ^http only... Mark, it's just an idea, ta Sorry, pressed wrong key. Mark, It's an idea, don't fully know if http(s):// is the expected beginning of base url. But that explained the wrong behavior I got. (In reply to Bernardo Gonzalez Kriegel from comment #201) > But... is strange to put OPAC under https No it's not. In fact, these days we pretty much won't deploy anything without it. People's passwords and personal information goes over it, after all. Even more so when it's LDAPified to a corporate authentication server. (In reply to Robin Sheat from comment #203) > No it's not. In fact, these days we pretty much won't deploy anything > without it. People's passwords and personal information goes over it, after > all. Even more so when it's LDAPified to a corporate authentication server. Good point, you are right. Putting 'https://' on OPACBaseURL brakes something? Is it safe to use it that way? (In reply to Bernardo Gonzalez Kriegel from comment #204) > Putting 'https://' on OPACBaseURL brakes something? > Is it safe to use it that way? We tend to not put the https:// part in, and it seems to work, though I bet there are corner cases that don't. I don't know what would happen if you included the protocol. Really, the whole syspref should be redesigned so that it requires it, so it can be set properly. I'm not sure what the best solution here is, perhaps make it work so that it'll add http:// unless the syspref =~ m|^\w+://| (after all, you might want to host your Koha on gopher://... :) (In reply to Robin Sheat from comment #205) >(after all, you might > want to host your Koha on gopher://... :) YES! Oh man. I haven't heard of that protocol for like decades, since university. Actually, Bernardo Gonzalez Kriegel, putting 'http://' or 'https://' into the OPACBaseURL can break things, because some places would then generate http://http://, because we are totally inconsistent in our appending in code vs. appending in templates. That kind of mistake busts really well in FireFox. Created attachment 27154 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, particularly the OPAC system preference: OpacProtocol. This is used to generate the http or https for the OPACBaseURL. This now checks for proper configuration of the system preference. I have attached my rework which includes a dependency on bug 5010. I have set the status to ASSIGNED, because I have not tested it yet, and did not feel like asking anyone else to sign off until I have revisited a full testing. Created attachment 27155 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, particularly the OPAC system preference: OpacProtocol. This is used to generate the http or https for the OPACBaseURL. This now checks for proper configuration of the system preference. Further testing has shown an problem with the id generation. You can't upload a file with identical contents to different directories. I'll add a patch for that. Created attachment 27225 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 30362 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 30363 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 30364 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 30365 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 30366 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 30367 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 30368 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 30369 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 30370 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, particularly the OPAC system preference: OpacProtocol. This is used to generate the http or https for the OPACBaseURL. This now checks for proper configuration of the system preference. Created attachment 30371 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. I rebased this. However, I am feeling, I may have to bite the bullet and just fix all the hardcoded http references in Koha across the board for bug 5010, so that OPACBaseURL can include https:// or http:// nicely. Created attachment 32384 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 32385 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 32386 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 32387 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 32388 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 32389 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 32390 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 32391 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 32392 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010. This ensured that http or https is in the OPACBaseURL. This now checks for proper configuration of the system preference. Created attachment 32393 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. I recently worked on database documentation for 3.18 and noticed that we now have a table misc_files in the database which currently stores scanned invoices from the acquisition module (bug 3050) If I saw it correctly, this patch set will add a table 'uploaded_files'. It looks to me like the new table was thought up to store different kinds of files. Not sure if a merge of both features would make sense, but it might be worth investigating in order to avoid having various ways of achieving similar things. We also choose to keep things separate for reasons. We _might_ also choose... I see now that one main difference is that we upload files to the file system here while misc_files seems to store the files directly in the database. Looked at the database some more and we got lots of tables where we store files - but no feature yet where we store the files outside the database as far as I can tell. So I think the idea of merging with misc_files is irrelevant - please don't let me keep you from signing off! fatal: sha1 information is lacking or useless (catalogue/detail.pl). Okay, I'll try to work on this. Created attachment 36198 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36199 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36200 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36201 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36202 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36203 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36204 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36205 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36206 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36207 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36208 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 36209 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 36210 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, particularly the OPAC system preference: OpacProtocol. This is used to generate the http or https for the OPACBaseURL. This now checks for proper configuration of the system preference. Created attachment 36211 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 36213 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36214 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36215 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36216 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36217 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36218 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36219 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 36220 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 36221 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, particularly the OPAC system preference: OpacProtocol. This is used to generate the http or https for the OPACBaseURL. This now checks for proper configuration of the system preference. Created attachment 36222 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 36223 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36224 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36225 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36226 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36227 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36228 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 36229 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 36230 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 36231 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Created attachment 36232 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. I have moved this to assigned, like I did in comment #208, because I have yet to retest the rebase, not to mention, I had issues rebasing, so some fixing jumped from one patch into another. I wish to retest as a result. The test plan is in comment #175, with later comments perhaps containing some other things to look at. Created attachment 37681 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37682 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37683 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37684 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37685 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37686 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37687 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37688 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37689 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37690 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37691 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 37692 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 37693 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Created attachment 37694 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 37696 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37697 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37698 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37699 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37700 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37701 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 37702 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 37703 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 37704 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Created attachment 37705 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 37706 [details] [review] Bug 6874: Catch warnings for clean tests. I hate noisy tests, so I cleaned them up. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles One more round of tests tomorrow, and this should be ready. :) Should the status be "Needs signoff"? (In reply to Magnus Enger from comment #298) > Should the status be "Needs signoff"? Oops... Yes, but so does bug 5010, on which this dependent. Created attachment 38911 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38912 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38913 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38914 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38915 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38916 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38917 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 38918 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 38919 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Created attachment 38920 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 38921 [details] [review] Bug 6874: Catch warnings for clean tests. I hate noisy tests, so I cleaned them up. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles One more round of tests tomorrow, and this should be ready. :) Created attachment 38922 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38923 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38924 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38925 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38926 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38927 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38928 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38929 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38930 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38931 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38932 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Created attachment 38933 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Created attachment 38934 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Created attachment 38935 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Created attachment 38936 [details] [review] Bug 6874: Catch warnings for clean tests. I hate noisy tests, so I cleaned them up. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles One more round of tests tomorrow, and this should be ready. :) Created attachment 39113 [details] [review] [SIGNED-OFF] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39114 [details] [review] [SIGNED-OFF] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39115 [details] [review] [SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39116 [details] [review] [SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39117 [details] [review] [SIGNED-OFF] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39118 [details] [review] [SIGNED-OFF] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39119 [details] [review] [SIGNED-OFF] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39120 [details] [review] [SIGNED-OFF] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39121 [details] [review] [SIGNED-OFF] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39122 [details] [review] [SIGNED-OFF] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39123 [details] [review] [SIGNED-OFF] Bug 6874: Catch warnings for clean tests. I hate noisy tests, so I cleaned them up. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles One more round of tests tomorrow, and this should be ready. :) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Tested on top of 5010 Followed mostly mega test plan, seems to work :) Can upload, delete, modify, etc Test runs Ok No koha-qa errors I view this as valuable addition, dangling since 2011! As with 5010, I consider this can be pushed (for 3.22) and we can fix anything wrong later. QA: Like to do this one next :) But if someone else is faster.. Please bear with me; still working on this one.. Created attachment 39626 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Slightly amended/simplified the patch during QA: The changes to GetMarcUrls are not really needed, and would have needed some attention. Another link text can be supplied by the plugin too. This also reduces the need for changes in basket.pl, MARCdetail.pl, detail.pl, opac-basket.pl and opac-detail.pl. Created attachment 39627 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39628 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39629 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39630 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39631 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39632 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39633 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39634 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39635 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39636 [details] [review] Bug 6874: Catch warnings for clean tests. I hate noisy tests, so I cleaned them up. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles One more round of tests tomorrow, and this should be ready. :) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Tested on top of 5010 Followed mostly mega test plan, seems to work :) Can upload, delete, modify, etc Test runs Ok No koha-qa errors I view this as valuable addition, dangling since 2011! As with 5010, I consider this can be pushed (for 3.22) and we can fix anything wrong later. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39637 [details] [review] Bug 6874: [QA Follow-up] Adjustments for opac-retrieve Adds httpheaders sub to UploadedFiles; this will be extended on a new report. (Idea is to add configurable headers for file extensions.) Trivial unit test added. Small cosmetic changes to opac-retrieve-file. Test plan: Run test UploadedFiles.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 39638 [details] [review] Bug 6874: [QA Follow-up] File test, plugin style, finddirs With reference to comment19 about using -e or -f, I adjusted a POD line and a warning. Note that the two -f occurrences in the module now are correct (if it is not a plain file, something is wrong..) The plugin contains an obsolete plugin_parameters and empty Blur, Focus code. Converted it to new style rightaway. Moved finddirs to module. Slightly adjusted since uploadpath is passed in as base already. Added unit test. Test plan: Run t/db../UploadedFiles.t. Run t/db../FrameworkPlugins.t -incl catalouging/value_builder/upload.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> QA Comment Bug 6874: First, thanks for your work, Julian and Marc (and others testing it). This really had a long time.. And it is a nice feature. But what should QA be without some comments :) No blockers for me, especially taking into consideration the long history. [1] The new dependency Test/CGI/Multipart.pm is marked as not required. But you do unconditionally use it in the unit test. Should it be required? [2] Pass a nonexistent id to opac-retrieve-file; this results in Premature end of script headers: opac-retrieve-file.pl (Error 500) We should do some more here (instead of just an exit). I would prefer at least a 404 error here instead of a 500. [3] The changes to GetMarcUrls are not really needed. They are only done at UNIMARC side; so we create some inconsistency. And imo the change is questionable. Using tagslib and an additional framework parameter seems overkill. You also do not use $text in your code. An easier and more consistent implementation would be: let the plugin provide a link text in the corresponding subfield. I slightly amended the first patch in this respect. [4] With reference to Jareds comment (dating from 2012): [Quote] This patch adds C4::UploadedFiles (notice the 's') with no indication of how it relates to C4::UploadedFile (notice the lack of the 's'). It's possible there is a legitimate reason for this, but there is absolutely no documentation of the new class. [End of quote] I would not want to block this development for this reason now, since we are three years later and much effort went into it already. But it is far from ideal to now have UploadedFile.pm and UploadedFiles.pm. (Too bad that this comment seems to be ignored later on.) The new modules does have far better test coverage and actually implements a TODO from the old module. The overlap between both modules should be worked on. [5] The plugin still contains old stuff like plugin_parameters. (While still working on converting plugins, I decided to do this one rightaway.) See last follow-up. [6] The design of the form is rather poor. This c/should be improved on a new followup report. (Additionally, if I uploaded a file successfully, there is no need to say that it was fine and give me a Close button. Just close the form and put the URL in my field. An error is something else.) [7] The directory structure on the upload form shows a *tree* of all writable folders and subfolders within the upload dir. It works now, but I have my doubts if we really want it like that. It could be a huge tree. It may have security implications too. I would rather opt for a combo box with some categories and leave the exact location to the upload manager. (Perhaps start a new subdir after 1000 uploads or whatever. Note that we leave the filename to the upload script too.) I will open a new report for it. [8] Some interesting extensions (imo) would be: Move upload to Tools (adjust the plugin to provide an additional interface). Provide a Manage upload too with some additional settings like quota, maximum size, etc. Add upload permissions. Add configurable headers for opac-retrieve (to view files inline etc.) I will start a few reports :-) [9] Personally, I would have renamed the column dir into subdir or relpath or something to indicate that it is no full path. Also for the root dir I would leave it empty instead of putting / into it. [10] Translation issues: I see several alerts in the template that cannot be translated. Please adjust (new report). Note to RM: I would ask for some additional consideration/clemency when deciding on this patch set. As you can see, the number of points listed could have justified a Failed QA too, but there is a lot of work in here. If you push it, we could also mark the new feature as experimental while some improvements are on the way. Opened umbrella report (bug 14294) for follow-up reports. Coming back here in a minute.. Hold on. Created attachment 41275 [details] [review] Bug 6874: File upload in MARC New cataloging plugin upload.pl and new system preference 'uploadPath'. upload.pl provide a way to upload files on the server and store a link (url) to it in MARC uploadPath is the absolute path on the server where the files will be stored. It's possible to have a hierarchy of directories under this path and the plugin will allow to choose in which directory to store the file. Stored value in MARC subfield looks like this: <OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file> So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this plugin to work correctly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Slightly amended/simplified the patch during QA: The changes to GetMarcUrls are not really needed, and would have needed some attention. Another link text can be supplied by the plugin too. This also reduces the need for changes in basket.pl, MARCdetail.pl, detail.pl, opac-basket.pl and opac-detail.pl. Created attachment 41276 [details] [review] Bug 6874: QA follow-up Squash of the following commits: return with explicit undef removed Follow-up for PBP fixes follow-up fix POD syntax follow-up jquery has moved Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: removed POD changes in Biblio.pm while rebasing. Created attachment 41277 [details] [review] Bug 6874: Add unit tests for C4::UploadedFiles Add unit tests for C4::UploadedFiles and move a variable declaration at subroutine level instead of file level. Add dependency to Test::CGI::Multipart Still works, and the newly-provided unit tests have good test coverage: C4/UploadedFiles.pm 90.7 65.0 66.7 100.0 100.0 0.2 86.4 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41278 [details] [review] Bug 6874: Move uploadPath syspref to koha-conf.xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41279 [details] [review] Bug 6874 - File upload in MARC - QA Followup * Renames uploadPath to upload_path to follow the standard naming conventions in koha-conf which use underscores rather than camel case * Remove reference to intranet-tmpl and replace with [% interface %] required to pass qa Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41280 [details] [review] Bug 6874: Force user to select a directory This patch adds a javascript and a server-side checks Test plan: 1/ Choose a file, leave the radios not selected and submit 2/ You have a javascript alert which prevents form to be submitted 3/ Disable javascript and repeat step 1 4/ Form is submitted but form is redisplayed with an error message telling you to choose a directory. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41281 [details] [review] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more Two problems were discovered while doing a fresh install of Koha. These problems in the kohastructure.sql file are addressed with this patch. Clicking the plug-in icon should cause the popup window to refocus. This adds the refocus code to the upload.pl file. The path to the jquery.js script was wrong in the upload_delete_file.tt file. Changed [% themelang %] to [% interface %]. If a user clones 856$u after uploading a file, deletes the file, and then clicks the plugin icon on the first 856$u, this will go immediately to the upload screen with an informative error message. After some validation was added, it was extended to include other cases. This serves to patch 6874 to a state where sign off should be possible. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41282 [details] [review] Bug 6874 - License Text Update This updates the license text for - C4/UploadedFiles.pm - cataloguing/value_builder/upload.pl - opac/opac-retrieve-file.pl to GPL v3 as per the http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence text. TEST PLAN --------- 1) Don't apply this patch as part of 6874. 2) Run the koha qa test tool. -- It will FAIL for those three file. 3) Apply this patch 4) Run the koha qa test tool. -- License problem is corrected, no FAILs. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41283 [details] [review] Bug 6874 - Clean up file URL generation This adds a dependency on bug 5010, in that the protocol is assumed in the OpacBaseURL system preference. It also adds improved error handling. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41284 [details] [review] Bug 6874 - id based on contents, filename, and directory. This patch corrects the inability to upload the same file to a different directory, or even a renamed file to the same directory. By including the filename and directory as part of the $id SHA generation, only identical files in the same directory with the same file name will generate the same $id. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41285 [details] [review] Bug 6874: Catch warnings for clean tests. I hate noisy tests, so I cleaned them up. MEGA TEST PLAN -------------- git checkout -b bug_6874 origin/master git bz apply 6874 -- yes, it should all apply smoothly. I intentionally restored an old backup from a couple months ago to force the upgrade. staff client: -- upgrade -- login Koha administration MARC Bibliographic framework MARC structure (for BKS) Search for 856 Subfields Edit (for u) Other options Plugin: upload.pl Save changes Search the catalog tab {choose a word, any word. I used fancy, because I knew we had a cookbook} click first link Edit Edit record 8 (that's the tab name) click the name for 856 scroll down to u and click the plugin icon -- Correctly got a configuration error message. At a command prompt: vi ~/koha-dev/etc/koha-conf.xml /enable_plug -- add in: <upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path> :wq mkdir {appropriate path used} -- I even added some subdirectories sudo chown -R -v www-data.www-data {appropriate path used} Back in staff client: click the plugin icon again -- this time an upload screen pops up click the 'Upload file' button -- message about no file or destination browse for a file, select one click the 'Upload file' button -- message about no destination click the 'Cancel' button click the plugin icon click a destination radio button click the 'Upload file' button -- message about no file browse for a file, select one click the 'Upload file' button -- message about success click the 'close' button. -- the text box has been filled in with a nice URL click the plugin icon click cancel click the plugin icon click delete -- the test box should be blanked, and a success message given click Close -- reupload a file properly click the plugin icon click delete click upload file browse for a file, select a destination, click upload file click close on success message click 'Clone this subfield' click the second plugin icon click delete click close click the first plugin icon -- Nice error message about a URL which points to nothing. click cancel In the second 856$u type in a URL (eg. www.google.com) click the second plugin icon -- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it. click save (we need to save the bibliographic record) In OPAC: search for the same word (I used fancy) find the entry you just updated with links click the two links. The dangling entry should give you a 500 error, and the other link should work just fine. get back to the detail page In staff client: edit edit record 8 change the 856$u to a valid file in the first link. save In OPAC: refresh the detail page, and click the first link again this time it should get downloaded nicely. From a command line: prove -v t/db_dependent/UploadedFiles.t perldoc C4::Biblio perldoc C4::UploadedFiles One more round of tests tomorrow, and this should be ready. :) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Tested on top of 5010 Followed mostly mega test plan, seems to work :) Can upload, delete, modify, etc Test runs Ok No koha-qa errors I view this as valuable addition, dangling since 2011! As with 5010, I consider this can be pushed (for 3.22) and we can fix anything wrong later. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41286 [details] [review] Bug 6874: [QA Follow-up] Adjustments for opac-retrieve Adds httpheaders sub to UploadedFiles; this will be extended on a new report. (Idea is to add configurable headers for file extensions.) Trivial unit test added. Small cosmetic changes to opac-retrieve-file. Test plan: Run test UploadedFiles.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 41287 [details] [review] Bug 6874: [QA Follow-up] Last adjustments for future developments 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> Created attachment 41288 [details] [review] Bug 6874: [QA Follow-up] Last adjustments for future developments 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> To get this moving again, I rebased and made some additional changes with reference to the last QA comment: For point 7 I at last removed the dir structure and provide a category. We can build further improvement on that. See the last commit message. (This QA followup got out of hand :) but I leave that to the RM) I still favor pushing it (with some special consideration..) Note that bugs 14588 and 14589 are slightly related and can follow this one. For point 4 (one candidate blocker), bug 14321 (WIP) will merge the two modules into Koha::Upload. I already have a prototype in test. This will also include stripping the progress from the server side to client side ajax (improved progress bar). This logic can be used in several uploading scripts like this plugin but also stage-marc-import etc. Hope to submit that in one or two weeks. Back to PQA Created attachment 41448 [details] [review] Bug 6874: (QA followup) Warnings in about page This patch introduces checks on the about.pl page so the user is warned for a deficient configuration. Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> Created attachment 41449 [details] [review] Bug 6874: (QA followup) upload_path missing in koha-conf.xml.in Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> Created attachment 41450 [details] [review] Bug 6874: (RM followup) DBIx updates Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Feature pushed to master. Congratulations to everyone involved! Note: this is an early push of a working feature. It needs testing and followup bugs will be filled (at least by me) that will be blockers for the release. (In reply to Tomás Cohen Arazi from comment #373) > Feature pushed to master. > > Congratulations to everyone involved! > > Note: this is an early push of a working feature. It needs testing > and followup bugs will be filled (at least by me) that will be blockers > for the release. Patches for bug 14321 forthcoming ! Created attachment 41783 [details] [review] Bug 6874: (QA followup) fix templates Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> |