http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6874
--- Comment #352 from Marcel de Rooy <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
