Bug 42221: Fix EAN-13 autoBarcode not incrementing

The EAN-13 barcode query used max(abs(barcode)) which allowed non-EAN-13
barcodes (e.g. 14-digit KTD sample barcodes) to dominate the result.
The saved 13-digit EAN-13 barcodes were always smaller, so $nextnum
never advanced and the same barcode was generated on every page load.

Fix by extracting EAN-13 logic into C4::Barcodes::ValueBuilder::EAN13
(consistent with other barcode type packages) and scoping the query to
WHERE length(barcode) = 13 so only valid-length barcodes contribute to
the max. Both barcode.pl and Koha::REST::V1::Biblios now call the shared
function, eliminating the duplicated inline code.

Tests: adds EAN-13 test cases to t/db_dependent/Barcodes_ValueBuilder.t
Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

C4/Barcodes/ValueBuilder.pm

@@ -92,6 +92,33 @@ sub get_barcode {
     return $nextnum;
 }
 
+package C4::Barcodes::ValueBuilder::EAN13;
+use Modern::Perl;
+use C4::Context;
+use Algorithm::CheckDigits qw( CheckDigits );
+
+sub get_barcode {
+    my ($args) = @_;
+
+    # Filter to 13-digit barcodes only so non-EAN-13 items (e.g. 14-digit sample
+    # barcodes) do not dominate max() and prevent the sequence from advancing.
+    my $query = "SELECT max(cast(barcode as unsigned)) FROM items WHERE length(barcode) = 13";
+    my $sth   = C4::Context->dbh->prepare($query);
+    $sth->execute();
+    my ($nextnum) = $sth->fetchrow_array;
+
+    my $ean = CheckDigits('ean');
+    if ( $nextnum && $ean->is_valid($nextnum) ) {
+        my $next = $ean->basenumber($nextnum) + 1;
+        $nextnum = $ean->complete($next);
+        $nextnum = '0' x ( 13 - length($nextnum) ) . $nextnum;    # pad zeros
+    } else {
+        warn "ERROR: invalid EAN-13 " . ( $nextnum // '' ) . ", using increment";
+        $nextnum = ( $nextnum // 0 ) + 1;
+    }
+    return $nextnum;
+}
+
 1;
 
 =head1 Barcodes::ValueBuilder

Koha/REST/V1/Biblios.pm

@@ -332,27 +332,7 @@ sub add_item {
                 ($barcode) = C4::Barcodes::ValueBuilder::hbyymmincr::get_barcode( { year => $year, mon => $month } );
                 $barcode = $homebranch . $barcode;
             } elsif ( $autoBarcode eq 'EAN13' ) {
-
+                ($barcode) = C4::Barcodes::ValueBuilder::EAN13::get_barcode();
-                # not the best, two catalogers could add the same
-                # barcode easily this way :/
-                my $query = "select max(abs(barcode)) from items";
-                my $dbh   = C4::Context->dbh;
-                my $sth   = $dbh->prepare($query);
-                $sth->execute();
-                my $nextnum;
-                while ( my ($last) = $sth->fetchrow_array ) {
-                    $nextnum = $last;
-                }
-                my $ean = CheckDigits('ean');
-                if ( $ean->is_valid($nextnum) ) {
-                    my $next = $ean->basenumber($nextnum) + 1;
-                    $nextnum = $ean->complete($next);
-                    $nextnum = '0' x ( 13 - length($nextnum) ) . $nextnum;    # pad zeros
-                } else {
-                    warn "ERROR: invalid EAN-13 $nextnum, using increment";
-                    $nextnum++;
-                }
-                $barcode = $nextnum;
             } else {
                 warn "ERROR: unknown autoBarcode: $autoBarcode";
             }

cataloguing/value_builder/barcode.pl

@@ -27,8 +27,6 @@ use C4::Barcodes::ValueBuilder;
 use C4::Biblio      qw( GetMarcFromKohaField );
 use Koha::DateUtils qw( dt_from_string );
 
-use Algorithm::CheckDigits qw( CheckDigits );
-
 use CGI      qw ( -utf8 );
 use C4::Auth qw( check_cookie_auth );
 my $input = CGI->new;
@@ -64,24 +62,7 @@ my $builder = sub {
     { # Generates a barcode where hb = home branch Code, yymm = year/month catalogued, incr = incremental number, reset yearly -fbcit
         ( $nextnum, $scr ) = C4::Barcodes::ValueBuilder::hbyymmincr::get_barcode( \%args );
     } elsif ( $autoBarcodeType eq 'EAN13' ) {
-
+        ($nextnum) = C4::Barcodes::ValueBuilder::EAN13::get_barcode( \%args );
-        # not the best, two catalogers could add the same barcode easily this way :/
-        my $query = "select max(abs(barcode)) from items";
-        my $dbh   = $params->{dbh};
-        my $sth   = $dbh->prepare($query);
-        $sth->execute();
-        while ( my ($last) = $sth->fetchrow_array ) {
-            $nextnum = $last;
-        }
-        my $ean = CheckDigits('ean');
-        if ( $ean->is_valid($nextnum) ) {
-            my $next = $ean->basenumber($nextnum) + 1;
-            $nextnum = $ean->complete($next);
-            $nextnum = '0' x ( 13 - length($nextnum) ) . $nextnum;    # pad zeros
-        } else {
-            warn "ERROR: invalid EAN-13 $nextnum, using increment";
-            $nextnum++;
-        }
     } else {
         warn "ERROR: unknown autoBarcode: $autoBarcodeType";
     }

t/db_dependent/Barcodes_ValueBuilder.t

@@ -17,7 +17,8 @@
 use Modern::Perl;
 
 use Test::NoWarnings;
-use Test::More tests => 9;
+use Test::More tests => 14;
+use Test::Warn;
 use Test::MockModule;
 use t::lib::TestBuilder;
 
@@ -63,4 +64,26 @@ my $item_5 = $builder->build_sample_item( { barcode => '978e0143019375' } );
 is( $nextnum, '979', 'incremental barcode' );
 is( $scr,     undef, 'incremental javascript' );
 
+# EAN-13 tests
+$dbh->do(q|DELETE FROM items|);
+$builder->build_sample_item( { barcode => '3999900001927' } );    # valid 13-digit EAN-13
+( $nextnum, $scr ) = C4::Barcodes::ValueBuilder::EAN13::get_barcode( \%args );
+is( $nextnum, '3999900001934', 'EAN-13 increments correctly from valid EAN-13' );
+is( $scr,     undef,           'EAN-13 javascript' );
+
+$dbh->do(q|DELETE FROM items|);
+$builder->build_sample_item( { barcode => '39999000019193' } );    # 14-digit, should be ignored
+warning_like(
+    sub { ( $nextnum, $scr ) = C4::Barcodes::ValueBuilder::EAN13::get_barcode( \%args ) },
+    qr/ERROR: invalid EAN-13/,
+    'EAN-13 warns when no valid 13-digit barcode found'
+);
+is( $nextnum, 1, 'EAN-13 not confused by 14-digit barcode (falls back to increment)' );
+
+$dbh->do(q|DELETE FROM items|);
+$builder->build_sample_item( { barcode => '39999000019193' } );    # 14-digit
+$builder->build_sample_item( { barcode => '3999900001927' } );     # 13-digit EAN-13
+( $nextnum, $scr ) = C4::Barcodes::ValueBuilder::EAN13::get_barcode( \%args );
+is( $nextnum, '3999900001934', 'EAN-13 picks max among 13-digit barcodes, ignores 14-digit' );
+
 $schema->storage->txn_rollback;