Bug 6448

Summary: EAN-13 barcode support
Product: Koha Reporter: Dobrica Pavlinusic <dpavlin>
Component: CatalogingAssignee: Dobrica Pavlinusic <dpavlin>
Status: CLOSED FIXED QA Contact: Paul Poulain <paul.poulain>
Severity: enhancement    
Priority: P5 - low CC: chris, cnighswonger, koha.sekjal, mglavica, paul.poulain, veron
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Bug 6448 [2/3] Barcodes::EAN13 autoBarcode
Bug 6448 [3/3] EAN-13 barcode labels
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Bug 6448 [2/3] Barcodes::EAN13 autoBarcode
Bug 6448 [3/3] EAN-13 barcode labels
Bug 6448 [3/3] EAN-13 barcode labels
Bug 6448 [3/3] EAN-13 barcode labels
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Bug 6448 [2/3] Barcodes::EAN13 autoBarcode
Bug 6448 - carp on invalid barcode

Description Dobrica Pavlinusic 2011-06-02 09:51:23 UTC
We are migrating one library from custom software to Koha, and they are using EAN-13 zero-padded barcodes (primary key in legacy system) instead of ISBN or ISSN. Since books allready have barcodes on them, we can't change it.

To make things more insteresting, at least one barcode reader reports it as UPC-A without first leading zero, since EAN-13 has backwards compatibility with it.
Comment 1 Dobrica Pavlinusic 2012-03-28 13:51:12 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2012-03-28 13:51:17 UTC Comment hidden (obsolete)
Comment 3 Dobrica Pavlinusic 2012-03-28 13:51:21 UTC Comment hidden (obsolete)
Comment 4 Marc VĂ©ron 2012-04-14 07:31:37 UTC
Applying: Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
error: patch failed: C4/Circulation.pm:36
error: C4/Circulation.pm: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:5086
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Comment 5 Dobrica Pavlinusic 2012-04-14 10:30:08 UTC Comment hidden (obsolete)
Comment 6 Dobrica Pavlinusic 2012-04-14 10:31:16 UTC
Comment on attachment 8668 [details] [review]
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter

This patch was obsoleted by new one which is rebased on current master
Comment 7 Dobrica Pavlinusic 2012-04-14 10:44:54 UTC
Marc, thanks for reporting bitrot since master moved quite a bit since I wrote it :-)

I can't re-order patches on bugzilla, and since only first one required change and now it's last one if you are using git bz to apply them you have to do that in correct order (which is included in commit message).

Patches will be in correct order once signed off, so I'm reluctant to re-upload last two patches again just to fix order, so I'm including example of how to apply it using git bz to make it easier:

dpavlin@koha-dev:/srv/koha$ git bz apply 6448
Bug 6448 - EAN-13 barcode support

Bug 6448 [2/3] Barcodes::EAN13 autoBarcode
Apply? [yn] n
Bug 6448 [3/3] EAN-13 barcode labels
Apply? [yn] n
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Apply? [yn] y

Applying: Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
dpavlin@koha-dev:/srv/koha$ git bz apply 6448
Bug 6448 - EAN-13 barcode support

Bug 6448 [2/3] Barcodes::EAN13 autoBarcode
Apply? [yn] y

Applying: Bug 6448 [2/3] Barcodes::EAN13 autoBarcode
Bug 6448 [3/3] EAN-13 barcode labels
Apply? [yn] y

Applying: Bug 6448 [3/3] EAN-13 barcode labels
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter
Apply? [yn] n
Comment 8 Dobrica Pavlinusic 2012-06-09 12:49:08 UTC Comment hidden (obsolete)
Comment 9 Dobrica Pavlinusic 2012-06-09 12:50:41 UTC
First patch again bit-rotted because development moved on. Instruction how to apply diffs in correct order still apply
Comment 10 Dobrica Pavlinusic 2012-06-10 15:16:39 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2012-06-10 15:25:01 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2012-06-10 15:27:59 UTC Comment hidden (obsolete)
Comment 13 Chris Cormack 2012-06-10 15:36:47 UTC
Created attachment 10130 [details] [review]
Bug 6448 [3/3] EAN-13 barcode labels

Current implementation doesn't scale barcodes because low-price
CCD barcode readers are very sensitive about size

Test scenario:

1. in Tools > Labels create or edit Layout and select EAN13 as barcode
   type

2. export one of existing batches using EAN13 layout and verify that
   generated pdf file contains barcodes

3. print pdf file and test it with barcode reader

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 14 Dobrica Pavlinusic 2012-06-10 15:53:23 UTC Comment hidden (obsolete)
Comment 15 Dobrica Pavlinusic 2012-06-11 13:18:32 UTC
Please ignore last patch which was git bz gone wrong and just concentrate on Chirs' patches
Comment 16 Ian Walls 2012-06-15 18:08:43 UTC
First patch does not update sysprefs.sql to reflect the new EAN13 option.  DB Rev is also at 3.07.... instead of 3.09.. (though the RM has to fix that manually, anyway).

In Patch 2, C4/Barcodes/EAN13 is missing copyright statement.  Also, subroutines are prototyped, which fails our perlcritic.  Another syspref altered without changes to sysprefs.sql.

Marking Failed QA for above reasons.
Comment 17 Dobrica Pavlinusic 2012-07-20 12:40:38 UTC Comment hidden (obsolete)
Comment 18 Dobrica Pavlinusic 2012-07-20 12:41:21 UTC
Created attachment 11042 [details] [review]
Bug 6448 [1/3] EAN-13/UPC-A itemBarcodeInputFilter

This filter which check validity of EAN-13 barcode and padd it with zeros
up to full 13 digit number. This will also expand 12 digit UPC-A barcodes
to EAN-13 automatically which is useful for older barcode readers which tend
to ignore first zero in EAN-13 if they have just UPC-A support.

It should be noted that EAN-13 or UPC-A product codes printed on books are not
good choice for barcodes in Koha since each item has to have unique barcode.

Test scenario:

1. prove t/Circulation_barcodedecode.t
   this checks expansion of 12 digit UPC-A to 13 digit EAN-13 and zero padding

2. in systempreferences search for itemBarcodeInputFilter and select EAN-13

3. edit one item and assign it valid EAN-13 barcode, eg. 0000000695152, check it out

4. test checkin with just 695152 to test leading zero expansion

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 19 Dobrica Pavlinusic 2012-07-20 12:42:02 UTC
Created attachment 11043 [details] [review]
Bug 6448 [2/3] Barcodes::EAN13 autoBarcode

Implement auto-incrementing EAN-13 barcodes

To make this work, C4::Barcodes::next was modified to call process_tail with
new incremented value so that process_tail can generate correct checksum.
Since process_tail is currenlty not used by any barcodes, this change is safe.

C4::Barcodes is used by addbiblio.pl when adding multiple records, while value_builder
is used in all other cases.

Test scenario:

1. prove t/Barcodes_EAN13.t

2. KOHA_CONF=/etc/koha/sites/fer/koha-conf.xml prove t/db_dependent/Barcodes.t
   this will check C4::Barcode implementataion

3. in systempreference change autoBarcode to incremental EAN-13 barcode

4. edit two items of any biblio assigning barcodes and verify that numbers
   are increasing. Have in mind that last digit is check digit, and it
   doesn't increment, but is calculated from barcode itself. Example with
   checksum in brackets: 000000086275[2], 000000086276[9], 000000086277[6]

5. Add Item and verify that it gets assigned next barcode

6. Add & Duplicate item and verify barcode increase

7. Add Multiple Copies and verify that barcode increase for each copy

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 20 Dobrica Pavlinusic 2012-07-20 12:42:48 UTC
I addressed QA issues in first two patches
Comment 21 Paul Poulain 2012-08-02 16:45:56 UTC
QA comment:
 * passes test-qa.pl
 * mostly include new feature using a lot of elsif, depending on the itemBarcodeInputFilter syspref. I tried to use "Remove spaces", still work well (dunno how to tests other cases)

passed QA
Comment 22 Paul Poulain 2012-08-03 09:23:23 UTC
Jenkins warning = this patch makes db_dependant test failed:
perl t/db_dependent/Barcodes.t 
1..66
ok 1 - use C4::Barcodes;

3.436456908560981e17 not valid EAN-13 barcode at /home/paul/koha.dev/koha-community//C4/Barcodes/EAN13.pm line 43.
# Looks like you planned 66 tests but ran 1.
# Looks like your test exited with 255 just after 1;

The test dies because the
 25         ok($obj1 = C4::Barcodes->new($_),           "$pre Barcode Creation : new($_)");

calls C4::Barcodes::EAN13::parse, and the barcode is invalid


I thought it was because of my database, but I'm not sure it is. Is it because of the database jenkins uses or something else, I can't say.

But for sure, you're welcomed to investigate.
Comment 23 Paul Poulain 2012-08-03 09:32:43 UTC
(In reply to comment #22)
> I thought it was because of my database, but I'm not sure it is. Is it
> because of the database jenkins uses or something else, I can't say.

OK, it is my database, but also the jenkin's one !

I activated $debug, and see:
autoBarcode format = EAN13 at t/db_dependent/Barcodes.t line 26
trying to create new EAN13
Retrieving max database query.
(current) max barcode found: 3.436456908560981e17
3.436456908560981e17 not valid EAN-13 barcode at /home/paul/koha.dev/koha-community//C4/Barcodes/EAN13.pm line 43.

I see that the (current) max barcode found comes from
SELECT max(abs(barcode)) FROM items LIMIT 1;

which is wrong for me because I don't have EAN13 as barcodes, but seems it's also wrong for the jenkins database.
Dobrica, please provide needed information to chris_c !
Comment 24 Chris Cormack 2012-08-03 20:00:51 UTC
Yes, the tests should work with the sample db, do you need to add an EAN-13 to the sample data.

Either that or use Test::MockModule and DBD::Mock to mock a database. Otherwise Jenkins is going to keep complaining at an ever increasing number of people.
Comment 25 Dobrica Pavlinusic 2012-08-03 22:00:37 UTC
Adding EAN13 bar-code to database might help, but only if all other bar-codes are shorter than 13 digits.

Excuse my ignorance, but where is the sample data and how can I add to it?

I tried sandbox databases from http://git.koha-community.org/gitweb/?p=contrib/global.git;a=tree;f=sandbox and I can reproduce problem with them.

Other bar-code plugins seems to carp on invalid input instead of die, so that might be another possible solution.
Comment 26 Jared Camins-Esakov 2012-08-03 22:04:11 UTC
I think for the test the best option would be to use DBD::Mock, because there is no reason to think that all barcodes will be shorter than 13 digits on any arbitrary installation.
Comment 27 Dobrica Pavlinusic 2012-08-03 22:04:39 UTC
Created attachment 11349 [details] [review]
Bug 6448 - carp on invalid barcode

This is consistent with implementation of other barcode types
which all carp instead of die on invalid barcodes.
Comment 28 Dobrica Pavlinusic 2012-08-03 22:08:26 UTC
I would almost prefer DBD::Mock, but it's another dependency, we should really require it for production deployment so it seems like overkill to introduce it for this case.
Comment 29 Jared Camins-Esakov 2012-08-03 22:11:18 UTC
We are already using DBD::Mock in tests (or, if we aren't, we will be soon). I think people will be willing to install one additional dependency in order to ensure a more stable system.
Comment 30 Chris Cormack 2012-09-03 23:32:45 UTC
This still needs to be fixed (the test that is) or it is going to continue to fail.
Comment 31 Paul Poulain 2012-09-19 14:14:37 UTC
(In reply to comment #27)
> Created attachment 11349 [details] [review]
> Bug 6448 - carp on invalid barcode
> 
> This is consistent with implementation of other barcode types
> which all carp instead of die on invalid barcodes.

Hey, it seems no-one (including me) spotted that you've submitted a fix that should please jenkins !

Note = when you submit a follow-up to a pushed patch, please switch back to "need signoff" to let everyone see there's something new.

Anyway:
I applied the patch, and prove t/db_dependent/Barcodes.t
now returns a correct 
All tests successful.

(even if this is a workaround, because the test is not actually testing correctly. It just don't die)