[Koha-bugs] [Bug 6874] Attach a file to a MARC record (Was: File upload in MARC)

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6874

--- Comment #352 from Marcel de Rooy <m.de.rooy at 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.

-- 
You are receiving this mail because:
You are watching all bug changes.