Description
Nick Clemens (kidclamp)
2016-02-22 13:12:38 UTC
Created attachment 48363 [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. Created attachment 48365 [details] [review] Bug 15877 - Unit tests 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 Created attachment 48367 [details] [review] Bug 15877 - Unit tests Created attachment 49197 [details] [review] [SIGNED-OFF] Bug 15877 - Unit tests Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 49198 [details] [review] [SIGNED-OFF] 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> 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? Created attachment 49600 [details] [review] Bug 15877 - Unit tests Signed-off-by: Srdjan <srdjan@catalyst.net.nz> (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 Created attachment 49926 [details] [review] Bug 15877 - Unit tests Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 49927 [details] [review] Bug 15877 - Unit tests Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 49928 [details] [review] Bug 15877 - Unit tests Signed-off-by: Srdjan <srdjan@catalyst.net.nz> 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 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> 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> Pushed to Master - Should be in the May 2016 release. Thanks! Patches pushed to 3.22.x, will be in 3.22.7 |