Bug 8684 - Duplicate check for creating items on order broken
Summary: Duplicate check for creating items on order broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Whiteboard:
Keywords:
: 9185 (view as bug list)
Depends on: 7178 11425
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-24 06:40 UTC by Katrin Fischer
Modified: 2016-12-05 21:23 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 8684: Fix duplicate check on creating items in the acquisition module (1.77 KB, patch)
2015-04-07 12:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8684: Add regression tests for SearchItemsByField (1.08 KB, patch)
2015-04-13 08:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8684: Fix duplicate check on creating items in the acquisition module (1.86 KB, patch)
2015-07-04 09:46 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 8684: Add regression tests for SearchItemsByField (1.15 KB, patch)
2015-07-04 09:47 UTC, Amit Gupta
Details | Diff | Splinter Review
[PASSED QA] Bug 8684: Fix duplicate check on creating items in the acquisition module (1.92 KB, patch)
2015-07-24 15:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8684: Add regression tests for SearchItemsByField (1.21 KB, patch)
2015-07-24 15:46 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2012-08-24 06:40:28 UTC
When creating items for an order, the duplicate check based on UniqueItemFields doesn't work any longer.

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

1) Create a new order
2) Add 2 items with the same barcode
3) Save the order

> only 1 item is created, no warning shown

Turning off Autobarcode (do not generate) didn't solve the problem. 

There is a javascript error that you can see when saving the order.
Comment 1 Sunita 2013-01-29 11:17:49 UTC
I tried adding barcode in Global System Preferences - Acquisitions - UniqueItemFields as barcode but this feature does not seem to work. Even same accession number is provided to new book it does not give any error message and it does not add that same number again but keeps that item as blank.
Comment 2 Katrin Fischer 2013-04-11 22:10:25 UTC
*** Bug 9185 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2013-04-11 22:10:49 UTC
Merging with duplicate bug 9185:

Settings:
AcqCreateItem = on receive
UniqueItemFields = barcode

1) Create a new order with one or more order lines
2) Close basket
3) Create a new invoice/shipment
4) Receive one item, use existing barcode

Problem:
- on saving, no warning appears
- item creation silently fails

Log:
...
[Sat Dec 01 23:11:05 2012] [error] [client 127.0.0.1] [Sat Dec  1 23:11:05 2012] finishreceive.pl: Indicator in 952 is empty at /home/katrin/kohaclone/C4/Biblio.pm line 2285., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=25&invoiceid=3
[Sat Dec 01 23:11:05 2012] [error] [client 127.0.0.1] [Sat Dec  1 23:11:05 2012] finishreceive.pl: DBD::mysql::st execute failed: Duplicate entry '3' for key 'itembarcodeidx' at /home/katrin/kohaclone/C4/Items.pm line 2105., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=25&invoiceid=3
[Sat Dec 01 23:11:05 2012] [error] [client 127.0.0.1] [Sat Dec  1 23:11:05 2012] finishreceive.pl: DBD::mysql::st execute failed: Column 'itemnumber' cannot be null at /home/katrin/kohaclone/C4/Acquisition.pm line 1094., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=25&invoiceid=3
...
Comment 4 Katrin Fischer 2013-06-16 22:01:11 UTC
The checks for duplicate values within the order work, but it's not correctly checking the database.
Comment 5 Jonathan Druart 2015-04-01 15:16:10 UTC
(In reply to Katrin Fischer from comment #4)
> The checks for duplicate values within the order work, but it's not
> correctly checking the database.

What did you mean with "not correctly"? Does this still valid?
Comment 6 Katrin Fischer 2015-04-01 15:20:25 UTC
Hm, thinking about it, this is probably what I meant:
- if you have duplicates in the forms on the order page (2 items, same barcode) it is found.
- if you have a barcode in an item, that already exists in the database, it isn't found

I haven't retested, but I think that's what happened.
Comment 7 Jonathan Druart 2015-04-07 11:54:05 UTC
Ok confirmed, still valid on master.
Comment 8 Jonathan Druart 2015-04-07 12:15:51 UTC
Actually introduced by bug 11425 (pushed after this bug report has been opened, hum...).
Comment 9 Jonathan Druart 2015-04-07 12:23:49 UTC Comment hidden (obsolete)
Comment 10 M. Tompsett 2015-04-11 03:55:36 UTC
Comment on attachment 37545 [details] [review]
Bug 8684: Fix duplicate check on creating items in the acquisition module

Review of attachment 37545 [details] [review]:
-----------------------------------------------------------------

Secondary thought:
$ git grep SearchItemsByField | grep \.t:
-- Nothing.
Perhaps including a test would
(a) increase testing coverage (always good, but a pain to do), and
(b) prevent brokenness like this, because people would trigger tests for C4::Items and it would be covered.
Suggested test would be different commit. Apply before patch, run test, it fails. Apply patch, run test, it passes.

As is, I believe the patch is right, but I have not tested it sufficiently (actually triggered it) to sign off.

::: C4/Items.pm
@@ -2591,5 @@
>  
> -    my $filters = [ {
> -            field => $field,
> -            query => $value,
> -    } ];

I see why someone may have been half awake reading the if code of the next function and thinking @{$filter->{filters}} when putting those square brackets there. But this isn't that structure, and clearly triggers the else which requires a hash ref, not an array. Now if only I could actually trigger this code, I'd sign this off. But I must be failing at following the test plan. :(
Comment 11 Jonathan Druart 2015-04-13 08:12:35 UTC Comment hidden (obsolete)
Comment 12 Amit Gupta 2015-07-04 09:46:21 UTC Comment hidden (obsolete)
Comment 13 Amit Gupta 2015-07-04 09:47:03 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-07-24 15:46:29 UTC
Created attachment 41171 [details] [review]
[PASSED QA] Bug 8684: Fix duplicate check on creating items in the acquisition module

The script acqui/check_uniqueness.pl has been introduced by bug 7178.
But bug 11425 added a call to a new subroutine SearchItemsByField in
order to refactore some code. This subroutine calls SearchItems with an
arrayref, which is not what SearchItems is expecting.
This broke the duplicate check done on creating items in the acquisition
module.

To reproduce the issue:
1/ Fill the pref UniqueItemFields with "barcode" and AcqCreateItem with
"placing an order"
2/ Create a new basket
3/ Create a new order
4/ Close the basket
5/ Receive items and set barcode with one already in your DB
6/ Save
7/ Nothing happen on the interface, but an error should have been added
in your Koha log file:
  Can't use an undefined value as an ARRAY reference at
  /home/koha/src/acqui/check_uniqueness.pl line 48.

Test plan:
1/ Repeat steps 1-6
2/ You should see a warning on the interface

Note that this should also happen with AcqCreateItem set to "creating an
order".

Works well, no errors.

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2015-07-24 15:46:55 UTC
Created attachment 41172 [details] [review]
[PASSED QA] Bug 8684: Add regression tests for SearchItemsByField

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Tomás Cohen Arazi 2015-07-24 17:10:44 UTC
Patches pushed to master.

Good job Jonathan!
Comment 17 Chris Cormack 2015-08-09 21:31:36 UTC
Pushed to 3.20.x will be in 3.20.3