Bug 23992 - Adding multiple items with barcode might end with infinite loop if same barcode will be next_value
Summary: Adding multiple items with barcode might end with infinite loop if same barco...
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Andrii Nugged
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-07 20:34 UTC by Andrii Nugged
Modified: 2019-11-16 08:08 UTC (History)
2 users (show)

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


Attachments
Bug 23992: Items multi add - generate 'no_next_barcode' error if no barcode (2.11 KB, patch)
2019-11-12 19:58 UTC, Andrii Nugged
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrii Nugged 2019-11-07 20:34:36 UTC
In /cataloguing/additem.pl we had the comment:
"FIXME : Please note that there is a risk of infinite loop here if we never find a suitable barcode".

I researched that when we are adding '$number_of_copies', there will be no infinite loop there in that 'for' loop EXCEPT if the 'next_value' method will return the same but defined barcode only (border case1). Other cases:

- without a barcode code always adds any number of items in a sequential way, so nothing to fix,

- if we have '$barcodevalue' defined in the beginning, code adds items with barcodes generating them with 'next_value', 
if then no barcode will be generated in 'next_value' then '$exist_itemnumber' will not be defined, so loop will keep adding items without barcodes. Sequentially. (case2)

- if barcode will be occupied so it will be skipped until next barcode will be generated, a free one, this was discussed with KohaSuomi as "might be not needed" but because this already created in this way in KC so let's keep this logic, so nothing to fix here too.

 
I am preparing patch to generate in case2: "no_barcode_value" error message if it was started with a barcode and no more barcodes generated,
or in case1: if the barcode after 'next_value' will be the same as old value.
Comment 1 Andrii Nugged 2019-11-12 19:58:16 UTC
Created attachment 95349 [details] [review]
Bug 23992: Items multi add - generate 'no_next_barcode' error if no barcode

When multiple items added with barcode from the first, we expect for next
barcode to be generated. If it will be the same barcode or empty one, then
error 'no_next_barcode' will be generated to inform user/operator that
operation wasn't completed as expected.
Comment 2 Andrii Nugged 2019-11-12 20:14:40 UTC
this might be "WIP" (not full patch without test plan), so I need community help, because it's hard to generate test plan: I tried with all variants of barcodes and in most cases we have barcode just incremented in "->next_value" method. 

With patch, the error "no_next_barcode" will be generated so this will break the loop and inform the user about the wrong generation in cases when we start adding WITH barcode but then for some reason the same or empty barcode started to appear.

If we start with empty barcode – then just requested numbers of items added without barcodes at all, as it was before.

Also, this will prevent the infinite loop (when $i++ won't be executed) in cases when the barcode generator might stuck and return the same old or some another barcode which will be always found for some item.
Comment 3 Jonathan Druart 2019-11-15 10:06:38 UTC
Hi Andrew,

Are you trying to fix a random FIXME or a bug you found? :)

If I understand correctly you are trying to fix a logic issue, and so an hypothetical situation where the barcode plugin will return $new = $old. Maybe we should fix that in the plugins then?

Even if the change makes sense to me I will need to think a bit more about it, especially the following "terribly hacky" fix I wrote few weeks ago.
Comment 4 Andrii Nugged 2019-11-15 13:46:52 UTC
(In reply to Jonathan Druart from comment #3)
> If I understand correctly you are trying to fix a logic issue, and so an
> hypothetical situation where the barcode plugin will return $new = $old.

or when it will return NO BARCODE (empty/undef) if such possible.

Because our plugins/barcodes can change by time and I don't know how might behave, I propose to detect this in the sequence, and inform user that "something went wrong.

But yes, if this never happens now and never later if someone will change barcode generator or use their own so that "no_barcode_generated" I've added not needed at all and this solution might look useless.

But idea was not so clear, I agree, but let's assume we:
1. started loop with barcode provided, so each next item should be with next_value barcode,
2. but eventually no next_value or it is the same for "unknown future reasons" (someone will develop Barcode generator which will do that),
3. so we should stop in the middle bringing error to operator/user that "we failed to go further with your sequence", not just silently continue adding empty or same barcodes.

But yes, this was unclear fix because it came as attempt to syncronize another bugfix in KohaSuomi fork which way behind of current KohaCommunity (v16 against v19), and because there's not exactly how this was solved in KohaSuomi, I wanted to have this fix in KC, port it back in KS, so then KS don't needs other fixes,

but I also agree this is "silver tape fix"
Comment 5 Andrii Nugged 2019-11-16 08:08:18 UTC
(In reply to Jonathan Druart from comment #3)

Also we had discussion about "sequence" of barcodes with KohaSuomi community/cataloguers but I still unclear in vision.

Do librarians needs to insert barcodes for items in the middle as THEY want? 
Or is it enough to number barcodes from free space from the max? 
Or fill randomly from given incrementally skipping existing ones and no matter how they spreaded, just should be unique and different?
Is these requirements worldwide or they need some settings + interface feedback about skipps/stopped generator/etc?

I proposed just to print error/do partial add/inform user when barcode sequence generated not as "user may expect", if users expects barcodes to start from their own wished barcode. 

But I don't have answer then "should we also inform that some barcode numbers were skipped"?
Example of the situation: if user will put first barcode which occupied, system will increment barcodes until free gap, and user will get all items numbered with totally diferent barcodes instead of that which he/she put on the beginning, and this even won't give any feedback to user. Should that be so?

Maybe – we should inform user that sequence was broken (jumped over occupied barcode, empty next one generated, barcode not re-generated), and stop?

I brought this question + partial solution here as WIP/unclear. Will re-translate this my message to IRC chat then.

And thanks for you that you're joined this small brainstorming :).