Bug 8425

Summary: Autobarcode generates duplicate barcodes with AcqCreateItems = on order
Product: Koha Reporter: David Cook <dcook>
Component: AcquisitionsAssignee: Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski>
Status: Needs Signoff --- QA Contact:
Severity: normal    
Priority: P5 - low CC: baptiste.wojtkowski, caroline.cyr-la-rose, gmcharlt, jonathan.druart, liz, m.de.rooy, mglavica, philippe.blouin, sam.sowanick
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8683
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11273
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18280
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30056
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 8425: Add an offset parameter for barcode generation in hbyymmincr barcode.pl
Bug 8425: Increment the barcode on each call of set_barcode()
Bug 8425: Add an offset parameter for barcode generation in hbyymmincr barcode.pl
Bug 8425: Increment the barcode on each call of set_barcode()
Bug 8425: Increment the barcode on each call of set_barcode()

Description David Cook 2012-07-12 05:46:07 UTC
Duplicate barcodes are generated when placing an Order in Acquisitions when AcqCreateItem = "placing an order" and Autobarcode is turned on.

The problem is that you can accidentally attach 3 items to an order, but only 1 will be saved to the database. When you go to receive your order, you can only receive 1 item as the other two were never made, since the barcode wasn't unique. 

In 3.8.0, a software error comes up which prevent any item creation, I believe, but master (3.9.x) doesn't throw any warnings or errors.
Comment 1 Katrin Fischer 2012-07-12 05:50:35 UTC
Hm, this should not happen, but maybe it will help to set the UniqueItemFields to barcode? In my tests it worked really nicely, but perhaps something changed.
Comment 2 David Cook 2012-07-12 06:10:06 UTC
(In reply to comment #1)
> Hm, this should not happen, but maybe it will help to set the
> UniqueItemFields to barcode? In my tests it worked really nicely, but
> perhaps something changed.

No luck. Setting "barcode" in UniqueItemFields makes no change.

When did you test it?
Comment 3 Katrin Fischer 2012-07-12 06:21:01 UTC
Hm, I think I signed off on the patch introducing it, so the bug history should tell.
Comment 4 David Cook 2012-07-12 06:39:22 UTC
(In reply to comment #3)
> Hm, I think I signed off on the patch introducing it, so the bug history
> should tell.

Which patch is that, Katrin?

---

Also, when I look at my standard install 3.8.0 or my dev install 3.8.0 branch, the clear button works in the item block. However, it doesn't work in 3.8.2 (maybe 3.8.1) or higher...

Also, in my standard 3.8.0 install, duplicate barcodes creates this error:

DBD::mysql::st execute failed: Duplicate entry '201180550469528' for key 'itembarcodeidx' at .../lib/C4/Items.pm line 2127.

While in my dev install 3.8.0 branch, a nice JS window pops up to say:

"Duplicate values detected. Please correct the errors and resubmit."

As mentioned before, in 3.8.2+, no error message is generated and only the first item is created.
Comment 5 David Cook 2012-07-12 06:45:01 UTC
(In reply to comment #4)
> (In reply to comment #3)
> 
> Also, when I look at my standard install 3.8.0 or my dev install 3.8.0
> branch, the clear button works in the item block. However, it doesn't work
> in 3.8.2 (maybe 3.8.1) or higher...
> 

Hmm. I was a bit hasty there. The clear button clears all the fields except date and barcode... although I'm not sure that's a good thing...
Comment 6 David Cook 2012-07-16 01:51:41 UTC
Still not sure why this one is happening...

The relevant code in neworderempty.tt looks identical. neworderempty.pl looks like it should be passing everything correctly to the template too. 

additems.js seems close enough, although check_uniqueness.pl is very different...but a quick test didn't show a difference. 

Even when I replace all these files with files from a version where the dupe check works, the JS still fails to pick up the duplication of the barcodes. 

I'll have to try git bisect again, but anyone have any ideas on this one? I figure the problem must be with additems.js and check_uniqueness.pl...
Comment 7 Katrin Fischer 2012-08-24 06:32:11 UTC
Hi David, I filed a separate bug for the problem with the clear link, it's bug 8683 now.
Comment 8 Katrin Fischer 2012-08-24 06:38:01 UTC
I can replicate the problem:

When using Autobarcode items created on order will all get the same(!) barcode. Additionally the check on saving the order fails, so you get no notice and only one item is created.

I think this is because Autobarcode checks for the next barcode in the database. We are creating more than one item at once, so there is a problem.

I am not sure if that part of the problem can be solved easily, but the form should warn on saving when duplicate barcodes are detected.

Settings:
UniqueItemFields: barcode
AcqCreateItem: placing an order
Autobarcode: 1, 2, 3...

Turning off Autobarcode (do not generate) didn't solve the problem. There is a javascript error that you can see when saving the order in Firebug. Filing another bug for the broken duplicate check.
Comment 9 Liz Rea 2012-12-12 03:06:29 UTC
oops sorry - wrong bug!
Comment 10 Galen Charlton 2013-08-14 21:32:19 UTC
I'm not able to reproduce the problem -- I suspect it was fixed by the patch for bug 8495.

David: could you check to see if you're still seeing this?
Comment 11 David Cook 2013-08-16 06:28:25 UTC
(In reply to Galen Charlton from comment #10)
> I'm not able to reproduce the problem -- I suspect it was fixed by the patch
> for bug 8495.
> 
> David: could you check to see if you're still seeing this?

Really? That's odd. Admittedly, I don't fully comprehend the black magic that is the AJAX item add form in Acquisitions but I'm still seeing this :/.

The first item you add will have a new barcode ($nextnum in comparison to the database). However, each new item you add to an order will have the exact same barcode as that one since they're all using the same database data, right? I don't see any checks against the barcode for items already in the order...

Steps to Reproduce:

1) Syspref "AcqCreateItem" to "placing an order"
2) Autobarcode to any of the options (although I do get a Javascript error for " generated in the form <branchcode>yymm0001.". So probably try the others for the time being...)
3) UniqueItemFields to "barcode"

4) Go to acquisitions > new basket 
5) Click on the "barcode" field for a new item, and add that item
6) Repeat step #5 and observe that they all have the same barcode
Comment 12 Jonathan Druart 2015-09-04 08:40:27 UTC
The problem is that the items are not inserted in the DB when you click on the "Add item" button. So the item form generated just after does not know an item is on the road.
Hopefully, there is now a check when you submit the complete order "Duplicate values detected. Please correct the errors and resubmit."
The form is not submitted, there are identical barcodes.

It seems quite tricky to fix this one.
Comment 13 Baptiste Wojtkowski (bwoj) 2024-07-31 10:12:55 UTC
Created attachment 169883 [details] [review]
Bug 8425: Add an offset parameter for barcode generation in hbyymmincr barcode.pl

In order to propose a patch for 8425 and 30056, we need to have the possibility to ask for multiple barcodes without storing a new barcode. See 8425 and 30056 for more information.

Note:
Since 30056 also needs this patch, these are splitted in order to rebase
easier
Comment 14 Baptiste Wojtkowski (bwoj) 2024-07-31 10:12:58 UTC
Created attachment 169884 [details] [review]
Bug 8425: Increment the barcode on each call of set_barcode()

Duplicate barcodes are generated when placing an Order in Acquisitions when AcqCreateItem = "placing an order" and Autobarcode is turned on.

The problem is that you can accidentally attach 3 items to an order, but only 1 will be saved to the database. When you go to receive your order, you can only receive 1 item as the other two were never made, since the barcode wasn't unique.

In 3.8.0, a software error comes up which prevent any item creation, I believe, but master (3.9.x) doesn't throw any warnings or errors.

Yes.. This tries and solve a bug from 3.8.0 /o/

Test plan:
1 - set syspref "autoBarcode" to generated in the form <branchcode> yymm001
2 - set the barcode field to "barcode.pl" in marc structure
3 - create a new basket
4 - add one item to this basket
5 - click on the barcode field -> it should have a barcode
6 - click on add item and click on the new barcode fiels, it should have
 the same value
 APPLY PATCH:
7 - click on the barcode field -> it should have a barcode
8 - click on add item and click on the new barcode fields, it should have
  an higher barcode.

Note : The barcode is incremented each time the barcode with an empty
value is clicked on and only on "hbyymmincr" mode. Therefore, if the
librarian removes the value from barcode and click again, they could get
another barcode. I do not know if it is an issue.
Comment 15 Baptiste Wojtkowski (bwoj) 2024-07-31 10:15:41 UTC
Note, the proposed approach does not solve any problem of concurrent accesses to barcode.pl but I do not think it could be resolved without refactoring barcode.pl.
Comment 16 Baptiste Wojtkowski (bwoj) 2024-07-31 10:25:06 UTC
Note: this should also resolve: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30056
Comment 17 Biblibre Sandboxes 2024-09-11 16:23:25 UTC
Created attachment 171350 [details] [review]
Bug 8425: Add an offset parameter for barcode generation in hbyymmincr barcode.pl

In order to propose a patch for 8425 and 30056, we need to have the possibility to ask for multiple barcodes without storing a new barcode. See 8425 and 30056 for more information.

Note:
Since 30056 also needs this patch, these are splitted in order to rebase
easier

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Comment 18 Biblibre Sandboxes 2024-09-11 16:23:29 UTC
Created attachment 171351 [details] [review]
Bug 8425: Increment the barcode on each call of set_barcode()

Duplicate barcodes are generated when placing an Order in Acquisitions when AcqCreateItem = "placing an order" and Autobarcode is turned on.

The problem is that you can accidentally attach 3 items to an order, but only 1 will be saved to the database. When you go to receive your order, you can only receive 1 item as the other two were never made, since the barcode wasn't unique.

In 3.8.0, a software error comes up which prevent any item creation, I believe, but master (3.9.x) doesn't throw any warnings or errors.

Yes.. This tries and solve a bug from 3.8.0 /o/

Test plan:
1 - set syspref "autoBarcode" to generated in the form <branchcode> yymm001
2 - set the barcode field to "barcode.pl" in marc structure
3 - create a new basket
4 - add one item to this basket
5 - click on the barcode field -> it should have a barcode
6 - click on add item and click on the new barcode fiels, it should have
 the same value
 APPLY PATCH:
7 - click on the barcode field -> it should have a barcode
8 - click on add item and click on the new barcode fields, it should have
  an higher barcode.

Note : The barcode is incremented each time the barcode with an empty
value is clicked on and only on "hbyymmincr" mode. Therefore, if the
librarian removes the value from barcode and click again, they could get
another barcode. I do not know if it is an issue.

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Comment 19 Marcel de Rooy 2024-11-15 08:40:44 UTC
(In reply to Biblibre Sandboxes from comment #18)
> In 3.8.0, a software error comes up which prevent any item creation, I
> believe, but master (3.9.x) doesn't throw any warnings or errors.
> 
> Yes.. This tries and solve a bug from 3.8.0 /o/

Am I really reading this in 2024 ??
Comment 20 Marcel de Rooy 2024-11-15 09:08:28 UTC
get_barcode
    my $scr = qq~
        let elt = \$("#"+id);
        let homebranch = elt.parents('fieldset.rows:first')
                            .find('input[name="kohafield"][value="items.homebranch"]')
                            .siblings("select")
                            .val();

        if(typeof offset == 'undefined'){
            var offset = 0;
        }
        if ( \$(elt).val() == '' ) {
            \$(elt).val(homebranch + ($nextnum + offset));
        }
    ~;
TOGETHER WITH
+function set_barcode(id, force, offset=0) {
=> Default is zero. Why are we checking the type then later? Seems useless?

+if(typeof autobarcodetype == 'undefined') {
+    var autobarcodetype = "$autoBarcodeType";
+    var attempt = -1;
+}
=> This does not look good either? Why are we conditionally declaring vars here?

Why are we fixing this only for a specific autobarcode type?
Comment 21 Baptiste Wojtkowski (bwoj) 2024-11-15 14:04:09 UTC
(In reply to Marcel de Rooy from comment #20)
> get_barcode
>     my $scr = qq~
>         let elt = \$("#"+id);
>         let homebranch = elt.parents('fieldset.rows:first')
>                            
> .find('input[name="kohafield"][value="items.homebranch"]')
>                             .siblings("select")
>                             .val();
> 
>         if(typeof offset == 'undefined'){
>             var offset = 0;
>         }
>         if ( \$(elt).val() == '' ) {
>             \$(elt).val(homebranch + ($nextnum + offset));
>         }
>     ~;
> TOGETHER WITH
> +function set_barcode(id, force, offset=0) {
> => Default is zero. Why are we checking the type then later? Seems useless?


Tbh I'm very unsecure with this part of the code and I'm unable to prove that this body is not appended to any other function without this prototype, hence to secure the behavior I prefered adding this. If there is a way to be sure this function definition will not be appended to any other prototype, then this part of the code is useless.


> +if(typeof autobarcodetype == 'undefined') {
> +    var autobarcodetype = "$autoBarcodeType";
> +    var attempt = -1;
> +}
> => This does not look good either? Why are we conditionally declaring vars
> here?

The point here is that due to the way this works, the script is imported each time you click on "add item", hence this will reset the number of attempts and so one. This is also true if you put it in a document.ready.
We might use attempts as condition and only do this for attempts, it might be more understandable.


> Why are we fixing this only for a specific autobarcode type?

Good question, I'm on it.
Comment 22 Baptiste Wojtkowski (bwoj) 2024-11-15 15:48:25 UTC
Created attachment 174602 [details] [review]
Bug 8425: Increment the barcode on each call of set_barcode()

Duplicate barcodes are generated when placing an Order in Acquisitions when AcqCreateItem = "placing an order" and Autobarcode is turned on.

The problem is that you can accidentally attach 3 items to an order, but only 1 will be saved to the database. When you go to receive your order, you can only receive 1 item as the other two were never made, since the barcode wasn't unique.

In 3.8.0, a software error comes up which prevent any item creation, I believe, but master (3.9.x) doesn't throw any warnings or errors.

Yes.. This tries and solve a bug from 3.8.0 /o/

Test plan:
1 - set syspref "autoBarcode" to generated in the form <branchcode> yymm001
2 - set the barcode field to "barcode.pl" in marc structure
3 - create a new basket
4 - add one item to this basket
5 - click on the barcode field -> it should have a barcode
6 - click on add item and click on the new barcode fiels, it should have
 the same value
 APPLY PATCH:
7 - click on the barcode field -> it should have a barcode
8 - click on add item and click on the new barcode fields, it should have
  an higher barcode.

Note : The barcode is incremented each time the barcode with an empty
value is clicked on. Therefore, if the
librarian removes the value from barcode and click again, they could get
another barcode. I do not know if it is an issue.
Note: This patch is not splitted in two parts anymore

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Comment 23 Baptiste Wojtkowski (bwoj) 2024-11-15 15:50:46 UTC
> +if(typeof autobarcodetype == 'undefined') {
> +    var autobarcodetype = "$autoBarcodeType";
> +    var attempt = -1;
> +}
> => This does not look good either? Why are we conditionally declaring vars here?

@Marcel I did not address this point, tell me if you see a clean solution for this dirty code :)