Bug 26328 - incremental barcode generation fails when incorrectly converting strings to numbers
Summary: incremental barcode generation fails when incorrectly converting strings to n...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2020-09-01 00:24 UTC by Pablo AB
Modified: 2022-12-12 21:24 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05,21.05.14,20.11.18


Attachments
Bug 26328: Cast barcode from varchar to integer for incremental barcode (1.85 KB, patch)
2020-12-04 05:40 UTC, David Cook
Details | Diff | Splinter Review
Bug 26328: Cast barcode from varchar to integer for incremental barcode (1.91 KB, patch)
2022-03-11 15:51 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 26328: Cast barcode from varchar to integer for incremental barcode (1.98 KB, patch)
2022-03-21 14:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26328: Add test (1.27 KB, patch)
2022-03-21 14:57 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 Pablo AB 2020-09-01 00:24:31 UTC
When using barcode.pl or barcode_manual.pl:, if a barcode is "85350E65", because of the "E" the value builder interprets it as having exponential notation: "8.535e69".

This happens on C4::Barcodes::ValueBuilder::incremental, on get_barcode().
When it says

   SELECT MAX(ABS(barcode)) FROM items;

probably should be instead

   SELECT MAX(ABS(CAST(barcode AS UNSIGNED))) FROM items;
Comment 1 David Cook 2020-09-01 00:55:06 UTC
That's an interesting one.

Your suggestion returns a value of 9223372036854775807. I would say that is an undesirable outcome as well.

It's probably better to use something like this:

SELECT MAX(ABS(barcode)) FROM items WHERE barcode REGEXP '^[0-9]+$';
Comment 2 Pablo AB 2020-09-01 02:10:04 UTC
You are right. On my tests:

SELECT barcode FROM items ORDER BY ABS(CAST(barcode AS UNSIGNED)) DESC LIMIT 5;
  770101E1 → 770101 (CAST on SELECT)

SELECT barcode FROM items ORDER BY ABS(barcode) DESC LIMIT 5;
  85350E65 → 8.535e69 (ABS on SELECT)

Your proposal seems the way to go.
Comment 3 David Cook 2020-09-02 04:33:15 UTC
I have a long TODO list, but I can look at adding a patch for this down the road.
Comment 4 David Cook 2020-12-04 05:29:17 UTC
I actually ran into this problem again today, and at the moment the query I like best is actually the following:

SELECT max(cast(barcode as unsigned)) from items;

I'll write up a patch now...
Comment 5 David Cook 2020-12-04 05:40:53 UTC
Created attachment 114179 [details] [review]
Bug 26328: Cast barcode from varchar to integer for incremental barcode

Without this patch, the incremental barcode generation will
treat 978e0143019375 as having an exponent and interpret it as a very
large number.

With this patch, the incremental barcode generation will first cast
barcode varchar strings to integers before finding a max() value.
In this case 978e0143019375 becomes 978 instead of
1.7976931348623157e308

Test plan:

0. Using koha-testing-docker

Before applying patch:

1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=autobarcode
2. Set to "generated in the form 1, 2, 3"
3. Go to http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additema&searchid=scs_1607059974968
4. Add item with barcode 978e0143019375
5. Click "p - Barcode"
6. Note the barcode is "Inf"

After applying patch:
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additema&searchid=scs_1607059974968
2. Click "p - Barcode"
3. Note the barcode is "39999000019194"
Comment 6 Pablo AB 2020-12-04 06:07:26 UTC
(In reply to David Cook from comment #4)
> I actually ran into this problem again today, and at the moment the query I
> like best is actually the following:
> 
> SELECT max(cast(barcode as unsigned)) from items;

Why you like the most this one (except for the ABS identical to my first suggestion), instead of your alternative con comment 1? I remember that at the time your suggestion seems better.
Comment 7 David Cook 2020-12-07 01:02:15 UTC
(In reply to Pablo AB from comment #6)
> Why you like the most this one (except for the ABS identical to my first
> suggestion), instead of your alternative con comment 1? I remember that at
> the time your suggestion seems better.

Option 1: SELECT MAX(ABS(barcode)) FROM items WHERE barcode REGEXP '^[0-9]+$';

This query won't generate any warnings, so that's good. But it's also fairly long and a little complex.

Option 2: SELECT max(cast(barcode as unsigned)) from items;

This query will show "Truncated incorrect..." warnings, but it's simpler/easier to read/maintain. 

--

In the end, it doesn't really matter too much I think.
Comment 8 Aleisha Amohia 2021-01-10 22:04:27 UTC
Is this ready for testing? If not can you please update the status appropriately?
Comment 9 David Cook 2021-01-10 22:09:36 UTC
(In reply to Aleisha Amohia from comment #8)
> Is this ready for testing? If not can you please update the status
> appropriately?

It was never properly signed off, so I think it's still in Needs Signoff, unfortunately.
Comment 10 Catherine 2021-01-19 22:08:53 UTC
I followed the test plan, however it did not work accordingly.
Comment 11 David Cook 2021-01-22 01:25:45 UTC
(In reply to Catherine from comment #10)
> I followed the test plan, however it did not work accordingly.

Can you describe how it did not work? It worked great last time I looked at it.
Comment 12 Catherine 2021-01-22 01:58:16 UTC
Hi David,
after applying the patch, I did step 2 however "39999000019194" did not appear. Nothing changed when I did step 2. Possible that I made an error during the test plan, so someone else should test again.
Comment 13 David Cook 2021-01-22 02:26:10 UTC
(In reply to Catherine from comment #12)
> Hi David,
> after applying the patch, I did step 2 however "39999000019194" did not
> appear. Nothing changed when I did step 2. Possible that I made an error
> during the test plan, so someone else should test again.

Hmm weird. I'll give it another go sometime too.

Were you using a fresh koha-testing-docker? Or were you testing it with a different Koha dev setup?
Comment 14 ByWater Sandboxes 2022-03-11 15:51:41 UTC
Created attachment 131592 [details] [review]
Bug 26328: Cast barcode from varchar to integer for incremental barcode

Without this patch, the incremental barcode generation will
treat 978e0143019375 as having an exponent and interpret it as a very
large number.

With this patch, the incremental barcode generation will first cast
barcode varchar strings to integers before finding a max() value.
In this case 978e0143019375 becomes 978 instead of
1.7976931348623157e308

Test plan:

0. Using koha-testing-docker

Before applying patch:

1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=autobarcode
2. Set to "generated in the form 1, 2, 3"
3. Go to http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additema&searchid=scs_1607059974968
4. Add item with barcode 978e0143019375
5. Click "p - Barcode"
6. Note the barcode is "Inf"

After applying patch:
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additema&searchid=scs_1607059974968
2. Click "p - Barcode"
3. Note the barcode is "39999000019194"

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Comment 15 Marjorie Barry-Vila 2022-03-11 15:53:29 UTC
Hi,
I tested on sandbox https://sandbox.bywatersolutions.com/signoff/bz26328 and for me test plan worked well.

Regards
Marjorie
Comment 16 Jonathan Druart 2022-03-21 14:53:43 UTC
Hey, you forgot the tests!
Comment 17 Jonathan Druart 2022-03-21 14:57:23 UTC
Created attachment 131976 [details] [review]
Bug 26328: Cast barcode from varchar to integer for incremental barcode

Without this patch, the incremental barcode generation will
treat 978e0143019375 as having an exponent and interpret it as a very
large number.

With this patch, the incremental barcode generation will first cast
barcode varchar strings to integers before finding a max() value.
In this case 978e0143019375 becomes 978 instead of
1.7976931348623157e308

Test plan:

0. Using koha-testing-docker

Before applying patch:

1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=autobarcode
2. Set to "generated in the form 1, 2, 3"
3. Go to http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additema&searchid=scs_1607059974968
4. Add item with barcode 978e0143019375
5. Click "p - Barcode"
6. Note the barcode is "Inf"

After applying patch:
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additema&searchid=scs_1607059974968
2. Click "p - Barcode"
3. Note the barcode is "39999000019194"

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2022-03-21 14:57:29 UTC
Created attachment 131977 [details] [review]
Bug 26328: Add test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Fridolin Somers 2022-03-23 20:50:48 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 20 Kyle M Hall 2022-03-25 13:58:39 UTC
Pushed to 21.11.x for 21.11.05
Comment 21 Andrew Fuerste-Henry 2022-04-15 18:15:15 UTC
Pushed to 21.05.x for 21.05.14
Comment 22 Victor Grousset/tuxayo 2022-04-17 20:20:54 UTC
Backported: Pushed to 20.11.x branch for 20.11.18