---- Reported by nengard@gmail.com 2010-02-11 14:15:16 ---- When choosing to upload a file that isn't a CSV file the inventory tool just refreshes - no errors presented. There should be some errors in place so that that librarians knows why things failed. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:23 UTC --- This bug was previously known as _bug_ 4162 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4162 Actual time not defined. Setting to 0.0
I tried uploading a .png file, the result looks interesting. This could certainly be improved.
Created attachment 29659 [details] [review] Bug 4162 The inventory tool lacks validation for barcodes The inventory tool had no form of validating on what was assumed to be a valid barcode number. To solve this, an extra loop to read before processing was added. This allows to validate length (<=20 which is the current size of the barcode field in the items table) and content (assumed that 0-9a-zA-Z was good enough). The template was modified to accomodate validation messages related to the length and content errors. TEST PLAN --------- 1) Attempt to select a file which does not contain barcodes and is not a text file. -- a horrible lack of validation and spamminess ensues. 2) Apply patch 3) Create three files. a) One containing valid barcodes on each line -- this file should trigger no errors. b) A copy of the first with an extra line of >20 characters (e.g. The Quick Red Fox Jumped Over The Brown Fence) -- this file should trigger the singular error message case. c) A copy of the second with the last line duplicated -- this file should trigger the plural error message case. 4) Attempt each of the three files. 5) Run koha-qa tools.
> content (assumed that 0-9a-zA-Z was good enough). As we don't enforce any restrictions on the barcode in circ and cataloguing, I think we can't do that here. I have seen libraries using special characters like ß and - in their barcodes and this would make it impossible for them to use the inventory tool. The length restriction seems like a good idea - should be ok to check just the first line of the file maybe?
Created attachment 29660 [details] [review] Bug 4162 The inventory tool lacks validation for barcodes The inventory tool had no form of validating on what was assumed to be a valid barcode number. To solve this, an extra loop to read before processing was added. This allows to validate length and content (assumed that [:print:] was good enough). The template was modified to accomodate validation messages related to the length and content errors. TEST PLAN --------- 1) Attempt to select a file which does not contain barcodes and is not a text file. -- a horrible lack of validation and spamminess ensues. 2) Apply patch 3) Create three files. a) One containing valid barcodes on each line -- this file should trigger no errors. b) A copy of the first with an extra line of >20 characters (e.g. The Quick Red Fox Jumped Over The Brown Fence^A^B^C) -- this file should trigger the singular error message case. ^A^B^C are actually CTRL-A,CTRL-B,CTRL-C, and it is left as an exercise to the reader to add them to the line. c) A copy of the second with the last line duplicated -- this file should trigger the plural error message case. 4) Attempt each of the three files. 5) Run koha-qa tools.
Created attachment 30548 [details] [review] [Signed-off] Bug 4162 The inventory tool lacks validation for barcodes The inventory tool had no form of validating on what was assumed to be a valid barcode number. To solve this, an extra loop to read before processing was added. This allows to validate length and content (assumed that [:print:] was good enough). The template was modified to accomodate validation messages related to the length and content errors. TEST PLAN --------- 1) Attempt to select a file which does not contain barcodes and is not a text file. -- a horrible lack of validation and spamminess ensues. 2) Apply patch 3) Create three files. a) One containing valid barcodes on each line -- this file should trigger no errors. b) A copy of the first with an extra line of >20 characters (e.g. The Quick Red Fox Jumped Over The Brown Fence^A^B^C) -- this file should trigger the singular error message case. ^A^B^C are actually CTRL-A,CTRL-B,CTRL-C, and it is left as an exercise to the reader to add them to the line. c) A copy of the second with the last line duplicated -- this file should trigger the plural error message case. 4) Attempt each of the three files. 5) Run koha-qa tools. Followed test plan 1-4. Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch>
I am sorry, but there is a problem with this - Umlauts are considered unprintable characters? My test barcode was Ümlauts and exists like that as a barcode in the database.
I am attempting to find a suitable regular expression to replace the [:print:] code sections with.
Created attachment 30841 [details] [review] Bug 4162 The inventory tool lacks validation for barcodes The inventory tool had no form of validating on what was assumed to be a valid barcode number. To solve this, an extra loop to read before processing was added. This allows to validate length and content. By using a check of \p{Print}, this includes Unicode characters such as umlauts, but excludes unusual control characters. The template was modified to accomodate validation messages related to the length and content errors. Additionally, it says how many "barcodes" were read. Barcodes are supposed to be on separate lines. TEST PLAN --------- 1) Attempt to select a file which does not contain barcodes and is not a text file. -- a horrible lack of validation and spamminess ensues. 2) Apply patch 3) Create three files. a) One containing valid barcodes on each line -- this file should trigger no errors. Attempt a valid barcode with an umlaut. b) A copy of the first with an extra line of >20 characters (e.g. The Quick Red Fox Jumped Over The Brown Fence^A^B^C) -- this file should trigger the singular error message case. ^A^B^C are actually CTRL-A,CTRL-B,CTRL-C, and it is left as an exercise to the reader to add them to the line. c) A copy of the second with the last line duplicated -- this file should trigger the plural error message case. 4) Attempt each of the three files. 5) Run koha-qa tools.
Created attachment 30880 [details] [review] Bug 4162 The inventory tool lacks validation for barcodes The inventory tool had no form of validating on what was assumed to be a valid barcode number. To solve this, an extra loop to read before processing was added. This allows to validate length and content. By using a check of \p{Print}, this includes Unicode characters such as umlauts, but excludes unusual control characters. The template was modified to accomodate validation messages related to the length and content errors. Additionally, it says how many "barcodes" were read. Barcodes are supposed to be on separate lines. TEST PLAN --------- 1) Attempt to select a file which does not contain barcodes and is not a text file. -- a horrible lack of validation and spamminess ensues. 2) Apply patch 3) Create three files. a) One containing valid barcodes on each line -- this file should trigger no errors. Attempt a valid barcode with an umlaut. b) A copy of the first with an extra line of >20 characters (e.g. The Quick Red Fox Jumped Over The Brown Fence^A^B^C) -- this file should trigger the singular error message case. ^A^B^C are actually CTRL-A,CTRL-B,CTRL-C, and it is left as an exercise to the reader to add them to the line. c) A copy of the second with the last line duplicated -- this file should trigger the plural error message case. 4) Attempt each of the three files. 5) Run koha-qa tools. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> I have a feeling that the column size could be better fetched from Koha::Database. But it is an improvement in functionality signing off
Created attachment 30891 [details] [review] [PASSED QA] Bug 4162 The inventory tool lacks validation for barcodes The inventory tool had no form of validating on what was assumed to be a valid barcode number. To solve this, an extra loop to read before processing was added. This allows to validate length and content. By using a check of \p{Print}, this includes Unicode characters such as umlauts, but excludes unusual control characters. The template was modified to accomodate validation messages related to the length and content errors. Additionally, it says how many "barcodes" were read. Barcodes are supposed to be on separate lines. TEST PLAN --------- 1) Attempt to select a file which does not contain barcodes and is not a text file. -- a horrible lack of validation and spamminess ensues. 2) Apply patch 3) Create three files. a) One containing valid barcodes on each line -- this file should trigger no errors. Attempt a valid barcode with an umlaut. b) A copy of the first with an extra line of >20 characters (e.g. The Quick Red Fox Jumped Over The Brown Fence^A^B^C) -- this file should trigger the singular error message case. ^A^B^C are actually CTRL-A,CTRL-B,CTRL-C, and it is left as an exercise to the reader to add them to the line. c) A copy of the second with the last line duplicated -- this file should trigger the plural error message case. 4) Attempt each of the three files. 5) Run koha-qa tools. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> I have a feeling that the column size could be better fetched from Koha::Database. But it is an improvement in functionality signing off Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described, no problems found. Passes tests and QA script. Another thing for another day: Empty lines are counted for the potential barcodes.
(In reply to Katrin Fischer from comment #10) > Another thing for another day: > Empty lines are counted for the potential barcodes. Oops... That's because the $lines_read++; (line 173) is before the next unless $barcode; (line 175)! DOH!
If you want to provide a follow-up here or on a separate bug I can test again.
Created attachment 30899 [details] [review] Bug 4162 - Followup: only count potential barcodes The $lines_read++; was before the code which skipped empty lines. By moving it after the next unless $barcode; code, the $lines_read variable only counts potential barcodes, and does not include empty lines. TEST PLAN --------- 1) Apply patch. 2) Attempt a valid barcode file with empty lines. -- The potential barcode count should exclude the empty lines.
Changed status to signed off, because Katrin suggested treating it like a QA followup in our discussion on the #koha IRC channel.
Created attachment 30922 [details] [review] [PASSED QA] Bug 4162 - Followup: only count potential barcodes The $lines_read++; was before the code which skipped empty lines. By moving it after the next unless $barcode; code, the $lines_read variable only counts potential barcodes, and does not include empty lines. TEST PLAN --------- 1) Apply patch. 2) Attempt a valid barcode file with empty lines. -- The potential barcode count should exclude the empty lines. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Empty lines are now no longer counted as potential barcodes. Passes tests and QA script.
Patches pushed to master. Good job Mark!