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.

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)

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.

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.

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)

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.

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.

(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

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.

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.

(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?

Jared, I finally choose to use both SHA-1 and name for filename. So we have the advantages of both sides, I think.

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

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 ?

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.

Oops. Forgot to reset status.

(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.

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.

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.

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.

(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...

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?

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.

Squashed my patches together as per Katrin Fischer's request.

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. :)

Comment on attachment 21546 [details] [review]
Bug 6874 - kohastructure.sql & jquery.js fix with refocus

squashed my patches into one on the end.

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.

Comment on attachment 21432 [details] [review]
[SIGNED-OFF] Bug 6874: File upload in MARC

I rebased it.

Comment on attachment 21433 [details] [review]
[SIGNED-OFF] Bug 6874: QA follow-up

I rebased this.

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. :)

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

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.

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

(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.

(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.

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.

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.

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.

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.