Bug 15877

Summary: C4::Barcodes does not correctly calculate db_max for 'annual' barcodes
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: brendan, jonathan.druart, julian.maurice, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14752, 16561    
Attachments: Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes
Bug 15877 - Unit tests
Bug 15877 - Unit tests
[SIGNED-OFF] Bug 15877 - Unit tests
[SIGNED-OFF] Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes
Bug 15877 - Unit tests
Bug 15877 - Unit tests
Bug 15877 - Unit tests
Bug 15877 - Unit tests
Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes
Bug 15877 - Unit tests

Description Nick Clemens 2016-02-22 13:12:38 UTC
The SQL query to find the database max value for annual barcodes is:
  42         my $query = "SELECT max(substring_index(barcode,'-',-1)) AS chunk,barcode FROM items WHERE barcode LIKE ? GROUP BY barcode";
  43                 # FIXME: unreasonably expensive query on large datasets

The 'max' in the first column creates confusion and disrupts the normal pairing of the db.

To recreate:
1 - Add two 'annual' type barcodes to the system (2016-0003, 2016-0006)

2 - Run the query:
SELECT max(substring_index(barcode,'-',-1)) AS chunk,barcode FROM items WHERE barcode LIKE '2016-%' GROUP BY barcode

3 - Note that returned values may not pair up, in my case I got things like:
chunk | barcode
0006  | 2016-0003
Comment 1 Nick Clemens 2016-02-24 17:12:18 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-02-24 17:57:41 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2016-02-24 18:03:33 UTC
Test plan:

1 - prove t/db_dependent/Barcodes.t
2 - All should pass
3 - Apply second patch (unit tests update)
4 - Test 3 should fail (annual) db_max equals greatest value when more than 1 present
5 - Apply first patch
6 - All tests should now pass
Comment 4 Nick Clemens 2016-02-24 22:49:42 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2016-03-16 05:47:04 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2016-03-16 05:47:24 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-03-16 11:21:20 UTC
Comment on attachment 49197 [details] [review]
[SIGNED-OFF] Bug 15877 - Unit tests

Review of attachment 49197 [details] [review]:
-----------------------------------------------------------------

A warning is raised by C4::Barcodes, use warning_is to catch it.

::: t/db_dependent/Barcodes.t
@@ +53,5 @@
> +        barcode => $barcodevalue
> +    }
> +});
> +
> +ok($barcodevalue eq $barcodeobj->db_max(), "(annual) First barcode saved to db is equal to db_max" );

use is instead of ok.

@@ +59,5 @@
> +$barcodevalue = $barcodeobj->next_value();
> +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> +$barcodevalue = $barcodeobj->next_value($barcodevalue);

Why are you calling 4x this method?
Comment 8 Nick Clemens 2016-03-25 20:50:33 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2016-03-25 20:52:28 UTC
(In reply to Jonathan Druart from comment #7)
> Review of attachment 49197 [details] [review] [review]:
> use is instead of ok.
ok (or should I say is ;-) )


> @@ +59,5 @@
> > +$barcodevalue = $barcodeobj->next_value();
> > +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> > +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> > +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> > +$barcodevalue = $barcodeobj->next_value($barcodevalue);
> Why are you calling 4x this method?

To set the value higher without using any constants. It's arbitrary to use 4, I just knew that triggered the problem for me each time and it felt more illustrative than 1
Comment 10 Nick Clemens 2016-04-05 18:52:37 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2016-04-05 18:55:25 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2016-04-05 19:48:21 UTC Comment hidden (obsolete)
Comment 13 Nick Clemens 2016-04-05 19:49:44 UTC
Edited the:
$dbh->do(q|DELETE FROM items|);

And replaced with:
$builder->clear( { source => 'Item' } );

Same for issues, a test crash wiped my database of all items so not an ideal construction
Comment 14 Jonathan Druart 2016-04-06 13:13:51 UTC
Created attachment 49961 [details] [review]
Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes

This patch alters the MySQL query to remove a max statement which
incorrectly groups full annual barcodes with numeric values.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2016-04-06 13:13:56 UTC
Created attachment 49962 [details] [review]
Bug 15877 - Unit tests

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Brendan Gallagher 2016-04-07 05:36:43 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 17 Julian Maurice 2016-05-25 08:35:26 UTC
Patches pushed to 3.22.x, will be in 3.22.7