Bug 4162 - Inventory Tool Fails Silently
Summary: Inventory Tool Fails Silently
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: PC All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-11 02:15 UTC by Nicole C. Engard
Modified: 2015-06-04 23:33 UTC (History)
5 users (show)

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


Attachments
Bug 4162 The inventory tool lacks validation for barcodes (3.99 KB, patch)
2014-07-12 13:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 4162 The inventory tool lacks validation for barcodes (4.22 KB, patch)
2014-07-12 16:11 UTC, Mark Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 4162 The inventory tool lacks validation for barcodes (4.42 KB, patch)
2014-08-05 20:15 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 4162 The inventory tool lacks validation for barcodes (4.97 KB, patch)
2014-08-15 15:19 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 4162 The inventory tool lacks validation for barcodes (5.16 KB, patch)
2014-08-16 22:26 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 4162 The inventory tool lacks validation for barcodes (5.38 KB, patch)
2014-08-17 09:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 4162 - Followup: only count potential barcodes (1.17 KB, patch)
2014-08-17 22:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 4162 - Followup: only count potential barcodes (1.33 KB, patch)
2014-08-18 10:26 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:23:52 UTC


---- 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

Comment 1 Katrin Fischer 2014-07-12 10:22:30 UTC
I tried uploading a .png file, the result looks interesting. This could certainly be improved.
Comment 2 Mark Tompsett 2014-07-12 13:53:03 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-07-12 14:01:04 UTC
> 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?
Comment 4 Mark Tompsett 2014-07-12 16:11:59 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2014-08-05 20:15:56 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2014-08-15 11:51:29 UTC
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.
Comment 7 Mark Tompsett 2014-08-15 14:27:46 UTC
I am attempting to find a suitable regular expression to replace the [:print:] code sections with.
Comment 8 Mark Tompsett 2014-08-15 15:19:12 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2014-08-16 22:26:37 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2014-08-17 09:11:54 UTC
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.
Comment 11 Mark Tompsett 2014-08-17 22:26:18 UTC
(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!
Comment 12 Katrin Fischer 2014-08-17 22:28:49 UTC
If you want to provide a follow-up here or on a separate bug I can test again.
Comment 13 Mark Tompsett 2014-08-17 22:49:05 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2014-08-17 22:55:28 UTC
Changed status to signed off, because Katrin suggested treating it like a QA followup in our discussion on the #koha IRC channel.
Comment 15 Katrin Fischer 2014-08-18 10:26:21 UTC
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.
Comment 16 Tomás Cohen Arazi 2014-08-20 20:58:58 UTC
Patches pushed to master.

Good job Mark!