[Bug 7961] New: Local cover images should support CSV link files
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Priority: P5 - low Change sponsored?: --- Bug ID: 7961 Assignee: gmcharlt@gmail.com Summary: Local cover images should support CSV link files QA Contact: koha.sekjal@gmail.com Severity: enhancement Classification: Unclassified OS: All Reporter: dpavlin@rot13.org Hardware: All Status: NEW Version: master Component: Cataloging Product: Koha -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 --- Comment #1 from Dobrica Pavlinusic <dpavlin@rot13.org> --- Created attachment 9190 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9190&action=edit 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. Also added is simple debug warn which displays number of cover image uploaded, biblionumber and filename which is very useful when uploading huge zip files with covers, because progress can be monitored in log. 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 3. create zip with CSV file and pictures 4. verify that all pictures got uploaded and linked to biblio records -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|gmcharlt@gmail.com |dpavlin@rot13.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=1633 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9190|0 |1 is obsolete| | --- Comment #2 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 9796 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9796&action=edit 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. Also added is simple debug warn which displays number of cover image uploaded, biblionumber and filename which is very useful when uploading huge zip files with covers, because progress can be monitored in log. 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 3. create zip with CSV file and pictures 4. verify that all pictures got uploaded and linked to biblio records Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Note that the file name of the CSV should be idlink.txt. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Severity|enhancement |normal --- Comment #3 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This is more of a bugfix than an enhancement (and it works perfectly). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |paul.poulain@biblibre.com --- Comment #4 from Paul Poulain <paul.poulain@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 --- Comment #5 from Dobrica Pavlinusic <dpavlin@rot13.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 --- Comment #6 from Paul Poulain <paul.poulain@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9796|0 |1 is obsolete| | --- Comment #7 from Dobrica Pavlinusic <dpavlin@rot13.org> --- Created attachment 9960 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9960&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Olugbenga Adara <gbengaadara@yahoo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |gbengaadara@yahoo.com --- Comment #8 from Olugbenga Adara <gbengaadara@yahoo.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 --- Comment #9 from Paul Poulain <paul.poulain@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #10 from Paul Poulain <paul.poulain@biblibre.com> --- 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 ;-) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9960|0 |1 is obsolete| | --- Comment #11 from Dobrica Pavlinusic <dpavlin@rot13.org> --- Created attachment 10105 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10105&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 --- Comment #12 from Dobrica Pavlinusic <dpavlin@rot13.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #13 from Ian Walls <koha.sekjal@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master Version|master |rel_3_8 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7961 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |chris@bigballofwax.co.nz --- Comment #14 from Chris Cormack <chris@bigballofwax.co.nz> --- No string changes, pushed to 3.8.x will be in 3.8.2 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org