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.

(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?

Hm, I think I signed off on the patch introducing it, so the bug history should tell.

(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.

(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...

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...

Hi David, I filed a separate bug for the problem with the clear link, it's bug 8683 now.

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.

oops sorry - wrong bug!

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?

(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

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.

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.

Note: this should also resolve: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30056

(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 ??

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?

(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.

> +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 :)

(In reply to Baptiste Wojtkowski (bwoj) from comment #23)
> > +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 :)

Thats quite hard. This area needs a major refactoring imo. The solution here does not address more fundamental issues, but looks like a workaround that increases technical debt?

Unless we totally rewrite the code of the module, which will de facto obsolete this patch, I do not know how we could solve this without that kind of tweak, but I'm not sure it would increase the technical debt since it is not more than a fixup to an existing debt and it simply improves the possibility of existing obsolete code.

Created attachment 176586 [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>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Followed the test plan, works as advertised.

(In reply to Baptiste Wojtkowski (bwoj) from comment #25)
> Unless we totally rewrite the code of the module, which will de facto
> obsolete this patch, I do not know how we could solve this without that kind
> of tweak, but I'm not sure it would increase the technical debt since it is
> not more than a fixup to an existing debt and it simply improves the
> possibility of existing obsolete code.

Understand your point. Adding Martin for another QA point of view.

Created attachment 176798 [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>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Followed the test plan, works as advertised.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

This is a bit painful, but the pragmatist in my says we should let this in as is..  I don't know when anyone is going to have any time to look at a cleaner solution.

(In reply to Marcel de Rooy from comment #19)
> (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 ??

Sad to see one of my earliest bug reports still alive and well hehe.

AcqCreateItem is set to "placing an order" in koha-testing-docker and in ./installer/data/mysql/mandatory/sysprefs.sql

So I'm surprised we've been quiet on this one for so long.

Locally, we default AcqCreateItem to "receiving an order", so maybe that's why we haven't revisited this one in all these years...

(In reply to David Cook from comment #31)
> AcqCreateItem is set to "placing an order" in koha-testing-docker and in
> ./installer/data/mysql/mandatory/sysprefs.sql
> 
> So I'm surprised we've been quiet on this one for so long.
> 
> Locally, we default AcqCreateItem to "receiving an order", so maybe that's
> why we haven't revisited this one in all these years...

It looks like autoBarcode defaults to 'OFF' out of the box, so a default configuration wouldn't generate this issue.

--

On /cgi-bin/koha/acqui/neworderempty.pl with AcqCreateItem set to "placing an order' and autoBarcode enabled, I see this message on the item input form:

"The autoBarcode system preference is set to incremental and items with blank barcodes will have barcodes generated upon save to database"

--

Wouldn't the simplest solution here be to prevent autoBarcode from working on the input form?