This is more of a bugfix than an enhancement (and it works perfectly).

QA comment:
 * unconditionnal warn added:
+ warn "## $total cover image biblionumber: $biblionumber file: $filename\n";

Either remove it if it's a test you've forgotten to remove, or add $ENV{DEBUG} and warn ... if you want to put a warn here.

For now, failed QA

When testing cover upload, I found it's very useful for it to report progress in apache log (as opposed to client timeout because of too big file/show connection or some random file upload limit).

I'm aware that apache error log might not be best place to put this info, but I think it's better to have it somewhere than to not have it at all.

Having said that, I could wrap if with if $ENV{DEBUG} but that won't help when users start calling me with questions like: I just uploaded covers, and I see just half of them (mostly because of wrong biblionumbers in CSV files).

Do we have some better place to put this information? action_logs comes to mind, but I'm wondering if module should be Catalog or should I add new one?

(En réponse au commentaire 5)
> When testing cover upload, I found it's very useful for it to report
> progress in apache log (as opposed to client timeout because of too big
> file/show connection or some random file upload limit).
> 
> I'm aware that apache error log might not be best place to put this info,
> but I think it's better to have it somewhere than to not have it at all.
> 
> Having said that, I could wrap if with if $ENV{DEBUG} but that won't help
> when users start calling me with questions like: I just uploaded covers, and
> I see just half of them (mostly because of wrong biblionumbers in CSV files).
I would prefer (and accept) to have this warning issued only if there is something wrong, that would be conditionnal, and be OK.

> Do we have some better place to put this information? action_logs comes to
> mind, but I'm wondering if module should be Catalog or should I add new one?

action_logs is another valid option, more usefull than warn: the user would be able to see at anytime what happened.

I have tested this using a Biblibre sandbox with the patch applied.

I added two more biblios and created the required zip file with images that had spaces in their filename. The file was uploaded and the OPAC displayed the uploaded images. In the staff client, the catalog log showed the three uploaded images with the proper filename.

I have verified that all pictures got uploaded and linked to biblio records and that modification log includes cover image name

(In reply to comment #8)
> I have tested this using a Biblibre sandbox with the patch applied.
> 
> I added two more biblios and created the required zip file with images that
> had spaces in their filename. The file was uploaded and the OPAC displayed
> the uploaded images. In the staff client, the catalog log showed the three
> uploaded images with the proper filename.
> 
> I have verified that all pictures got uploaded and linked to biblio records
> and that modification log includes cover image name

Hi Olugbenga Adara,

A lot of thanks for this great report of what you tested ! really.

QA comment: just one, but it will result in failed QA:
You must test the syspref CataloguingLog before logging action:
+                            logaction('CATALOGUING', 'MODIFY', $biblionumber, "cover image: $filename");

if (C4::Context->preference("CataloguingLog")) {
    logaction('CATALOGUING', 'MODIFY', $biblionumber, "cover image: $filename");
}

will make you earn QA ;-)

Created attachment 10105 [details] [review]
Bug 7961 - Local cover images should support CSV link files

Corrent code doesn't have support for filenames which contain spaces
or commans which breaks CSV files saved from spreadsheet similar to:

  12345, "conver image, with spaces.jpg"

This patch tweaks file parsing a bit. We are always splitting line to
only two values (to support commas as part of filename) and removing
spaces only on beginning and end of filename (to cover space after
comma in CSV example above while preserving spaces in filename).

With this change only invalid character in picture filename left
are quotes (") which are commonly used to quote strings with spaces.

Covers added will be logged in action_log, using CATALOGUING / MODIFY
action (which is shown as "Catalog" in tools > Log viewer)

Test scenario:

1. collect pictures with spaces and commas in name
2. dump file list into CSV file and add biblio number as first column
   (name of file is idlink.txt or datalink.txt)
3. create zip with CSV file and pictures
4. verify that all pictures got uploaded and linked to biblio records
5. verify that modification log includes cover image name

I asked Paul and he said we can skip sign-off for last change since it already got two sign-offs. However, currently submitted patch does not include sign-off lines (because I wrote it so it would be sheeting) so they would have to be added in QA.

Changes are well-reviewed by now, optimized over several QAings.  Code is simple change, handles split more elegantly, and scrubs for leading/trailing whitespace instead of whitespace anywhere in filename.  Marking Passed QA.

No string changes, pushed to 3.8.x will be in 3.8.2