Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes
Summary: C4::Barcodes does not correctly calculate db_max for 'annual' barcodes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 14752 16561
  Show dependency treegraph
 
Reported: 2016-02-22 13:12 UTC by Nick Clemens
Modified: 2016-12-05 21:25 UTC (History)
4 users (show)

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


Attachments
Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes (1.16 KB, patch)
2016-02-24 17:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.01 KB, patch)
2016-02-24 17:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.14 KB, patch)
2016-02-24 22:49 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15877 - Unit tests (2.18 KB, patch)
2016-03-16 05:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes (1.20 KB, patch)
2016-03-16 05:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.27 KB, patch)
2016-03-25 20:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.57 KB, patch)
2016-04-05 18:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.61 KB, patch)
2016-04-05 18:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.66 KB, patch)
2016-04-05 19:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15877 - C4::Barcodes does not correctly calculate db_max for 'annual' barcodes (1.28 KB, patch)
2016-04-06 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15877 - Unit tests (2.73 KB, patch)
2016-04-06 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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