Bug 7961 - Local cover images should support CSV link files
Summary: Local cover images should support CSV link files
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-14 09:33 UTC by Dobrica Pavlinusic
Modified: 2013-12-05 19:59 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7961 - Local cover images should support CSV link files (2.44 KB, patch)
2012-04-14 09:51 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7961 - Local cover images should support CSV link files (2.60 KB, patch)
2012-05-26 11:42 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7961 - Local cover images should support CSV link files (2.58 KB, patch)
2012-06-06 15:40 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7961 - Local cover images should support CSV link files (2.69 KB, patch)
2012-06-10 13:37 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2012-04-14 09:33:54 UTC

    
Comment 1 Dobrica Pavlinusic 2012-04-14 09:51:33 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-05-26 11:42:48 UTC Comment hidden (obsolete)
Comment 3 Jared Camins-Esakov 2012-05-26 11:49:09 UTC
This is more of a bugfix than an enhancement (and it works perfectly).
Comment 4 Paul Poulain 2012-05-28 15:59:38 UTC
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
Comment 5 Dobrica Pavlinusic 2012-05-28 18:20:28 UTC
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?
Comment 6 Paul Poulain 2012-05-29 02:35:58 UTC
(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.
Comment 7 Dobrica Pavlinusic 2012-06-06 15:40:16 UTC Comment hidden (obsolete)
Comment 8 Olugbenga Adara 2012-06-10 10:13:24 UTC
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
Comment 9 Paul Poulain 2012-06-10 10:17:11 UTC
(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.
Comment 10 Paul Poulain 2012-06-10 12:21:04 UTC
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 ;-)
Comment 11 Dobrica Pavlinusic 2012-06-10 13:37:20 UTC
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
Comment 12 Dobrica Pavlinusic 2012-06-11 09:34:30 UTC
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.
Comment 13 Ian Walls 2012-06-15 17:36:38 UTC
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.
Comment 14 Chris Cormack 2012-06-20 09:26:18 UTC
No string changes, pushed to 3.8.x will be in 3.8.2