Bug 39282

Summary: When adding an order from file, data entered in the "Item information" tab is not saved and invalid items are created
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: AcquisitionsAssignee: Nick Clemens (kidclamp) <nick>
Status: Pushed to stable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: critical    
Priority: P1 - high CC: alyssa.drake, chughesman, cslone, emily.lamancusa, jheltibridle, jvinke, martin.renvoize, mteal, nick, paul.derscheid, phil, rcoert, sam.sowanick
Version: MainKeywords: regression
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38757
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
This fixes an issue that occurs in Acquisitions when adding an order from a new or staged file. If the system preference AcqCreateItem is set to create items when the order is placed, and item information is not imported from the uploaded MARC file, then the librarian would enter the item information in the "Item information" tab when confirming the order. The information from this tab was not getting processed correctly, which led to "empty" items being created.
Version(s) released in:
25.05.00, 24.11.03
Circulation function:
Bug Depends on: 34355    
Bug Blocks:    
Attachments: Bug 39282: Unit tests for item creation
Bug 39282: Ensure items are created using Item Information values if not using MarcItemFieldsToOrder
Bug 39282: Clarify use of budget codes and ids
Bug 39282: Use generated MARC record to create item and return itemnumber
Test records for adding to basket
Bug 39282: Unit tests for item creation
Bug 39282: Ensure items are created using Item Information values if not using MarcItemFieldsToOrder
Bug 39282: Clarify use of budget codes and ids
Bug 39282: Use generated MARC record to create item and return itemnumber
Bug 39282: Unit tests for item creation
Bug 39282: Ensure items are created using Item Information values if not using MarcItemFieldsToOrder
Bug 39282: Clarify use of budget codes and ids
Bug 39282: Use generated MARC record to create item and return itemnumber
Bug 39282: (follow-up) Use generated budget and don't rely on existing data

Description Lucas Gass (lukeg) 2025-03-07 21:26:19 UTC
TO recreate:

1. In acquisitions create or find a vendor 
2. Make a new basket, either from 'New file' or 'Staged file'
3. Receive the order 
4. 500 error 

This is what I see in the logs: 

==> /var/log/koha/kohadev/plack-api-error.log <==
[2025/03/07 21:25:45] [ERROR] GET /api/v1/acquisitions/orders: unhandled exception (Mojo::Exception)<<Can't call method "bookable" on an undefined value at /kohadevbox/koha/Koha/Item.pm line 1922.>>
Comment 1 Emily Lamancusa (emlam) 2025-03-11 19:37:46 UTC
See also https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38757#c1

This happens only when uploading a MARC record that does not contain item data, if the items are created when the order is placed.

If you go to the bib record and look at the item data (by clicking Edit > Manage Items, since the holdings table also won't load), you'll see that the items have no data at all! Which is why they cause Error 500s on the receiving page as well as the item holdings table.

It looks like the data from the "Item information" tab is not being processed. It's being passed from the template to addorderiso2709.pl in the @tags, @subfields, @field_values, and @serials parameters, which are then passed to Koha::MarcOrder->import_record_and_create_order_lines by way of the $client_item_fields hash...but it seems like they're getting lost somewhere and aren't actually making it into the items that get created.

I don't have time to dig into this any further at the moment, unfortunately, but I hope that's a helpful start for someone who does!
Comment 2 Lucas Gass (lukeg) 2025-03-11 19:49:51 UTC
Very helpful, thanks Emily.
Comment 3 Chris Slone 2025-03-11 20:50:25 UTC
We seem to be seeing this when trying to create new baskets and adding to the baskets with a staged (or new ) file. This isn't happening on all .mrc files we try to use. but two out of three we've been try to get to work. Perhaps a different thing?

1.) 1. In acquisitions create or find a vendor 
2. Make a new basket
3. Add to basket, either from 'New file' or 'Staged file'
4. 500 error
Comment 4 Nick Clemens (kidclamp) 2025-03-12 17:11:10 UTC
Created attachment 179222 [details] [review]
Bug 39282: Unit tests for item creation
Comment 5 Nick Clemens (kidclamp) 2025-03-12 17:11:13 UTC
Created attachment 179223 [details] [review]
Bug 39282: Ensure items are created using Item Information values if not using MarcItemFieldsToOrder

When using MarcFieldsToOrder and not MarcItemFieldsToOrder it is expected that all of the item
information will come from the second tab 'Item information' on addorderiso2709.pl

Currently we are not and blank items are beign created

To test:
 1 - Apply unit tests
 2 - prove -v t/db_dependent/Koha/MarcOrder.t
 3 - It fails
 4 - Set MarcItemFields to Order:
     price: 949$p
     quantity: 947$c
     budget_code: 947$f
 5 - Ensure MarcItemFieldsToOrder is blank
 6 - Find a basket in acquisitions
 7 - Add to basket from a file
 8 - Fill item information, set 'Matching' at top of form to 'Do not look for matchign records' and save
 9 - Orders are added
10 - Click on a title
11 - Items do not load and an error is generated, this is because items don't have any values
12 - Check the DB:
     SELECT * FROM items WHERE homebranch IS NULL\G
13 - Note items from order are listed
14 - Apply the rest of the patches
15 - Repeat 6-10
16 - This time the items load successfully

NOTE: This patch alone doesn't address all errors, but subsequent will, keeping separate to address
each issue individually, but test plan can be executed as a whole against all
Comment 6 Nick Clemens (kidclamp) 2025-03-12 17:11:15 UTC
Created attachment 179224 [details] [review]
Bug 39282: Clarify use of budget codes and ids

When getting items from the form on addorderis2907.pl we are getting actual budget id's,
we only deal with codes when fetching the values from the MARC.
The code here assigns an id into a variable called code - this patch clarifies that and
ensures that we fallback to using the id if one has been passed from the interface form

To test:
0 - Apply first two patches
1 - prove -v t/db_dependent/Koha/MarcOrders.t
2 - It fails
3 - Apply this patch
4 - It fails?
Comment 7 Nick Clemens (kidclamp) 2025-03-12 17:11:17 UTC
Created attachment 179225 [details] [review]
Bug 39282: Use generated MARC record to create item and return itemnumber

When creating items from the form we need to use the MARC record we build from the HTML->XML->MARC

This patch simply uses the variable we create and ensures we pass the itemnumbers back

To test:
1 - prove -v t/db_dependent/Koha/MarcOrders.t
2 - Still failing
3 - Apply this patch
4 - Success!

Phew! We did it.
Comment 8 Nick Clemens (kidclamp) 2025-03-12 17:52:46 UTC
Created attachment 179230 [details]
Test records for adding to basket

Test records for adding to basket
Comment 9 Andrew Fuerste-Henry 2025-03-13 18:10:08 UTC
Created attachment 179280 [details] [review]
Bug 39282: Unit tests for item creation

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Comment 10 Andrew Fuerste-Henry 2025-03-13 18:10:11 UTC
Created attachment 179281 [details] [review]
Bug 39282: Ensure items are created using Item Information values if not using MarcItemFieldsToOrder

When using MarcFieldsToOrder and not MarcItemFieldsToOrder it is expected that all of the item
information will come from the second tab 'Item information' on addorderiso2709.pl

Currently we are not and blank items are beign created

To test:
 1 - Apply unit tests
 2 - prove -v t/db_dependent/Koha/MarcOrder.t
 3 - It fails
 4 - Set MarcItemFields to Order:
     price: 949$p
     quantity: 947$c
     budget_code: 947$f
 5 - Ensure MarcItemFieldsToOrder is blank
 6 - Find a basket in acquisitions
 7 - Add to basket from a file
 8 - Fill item information, set 'Matching' at top of form to 'Do not look for matchign records' and save
 9 - Orders are added
10 - Click on a title
11 - Items do not load and an error is generated, this is because items don't have any values
12 - Check the DB:
     SELECT * FROM items WHERE homebranch IS NULL\G
13 - Note items from order are listed
14 - Apply the rest of the patches
15 - Repeat 6-10
16 - This time the items load successfully

NOTE: This patch alone doesn't address all errors, but subsequent will, keeping separate to address
each issue individually, but test plan can be executed as a whole against all

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Comment 11 Andrew Fuerste-Henry 2025-03-13 18:10:13 UTC
Created attachment 179282 [details] [review]
Bug 39282: Clarify use of budget codes and ids

When getting items from the form on addorderis2907.pl we are getting actual budget id's,
we only deal with codes when fetching the values from the MARC.
The code here assigns an id into a variable called code - this patch clarifies that and
ensures that we fallback to using the id if one has been passed from the interface form

To test:
0 - Apply first two patches
1 - prove -v t/db_dependent/Koha/MarcOrders.t
2 - It fails
3 - Apply this patch
4 - It fails?

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Comment 12 Andrew Fuerste-Henry 2025-03-13 18:10:16 UTC
Created attachment 179283 [details] [review]
Bug 39282: Use generated MARC record to create item and return itemnumber

When creating items from the form we need to use the MARC record we build from the HTML->XML->MARC

This patch simply uses the variable we create and ensures we pass the itemnumbers back

To test:
1 - prove -v t/db_dependent/Koha/MarcOrders.t
2 - Still failing
3 - Apply this patch
4 - Success!

Phew! We did it.

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Comment 13 Emily Lamancusa (emlam) 2025-03-13 19:22:06 UTC
Andrew, is it intentional that your signoff lines say John Vinke? Or should I fix those when I (hopefully!) sign off for QA?
Comment 14 John Vinke 2025-03-13 20:02:02 UTC
Yes, we tested this in the Koha-US Baby Devs SIG and I was having issues authenticating to bugzilla.  Since we were going over time he just did it.
Comment 15 Emily Lamancusa (emlam) 2025-03-13 20:04:29 UTC
(In reply to John Vinke from comment #14)
> Yes, we tested this in the Koha-US Baby Devs SIG and I was having issues
> authenticating to bugzilla.  Since we were going over time he just did it.

Ah, that makes sense. Thanks for testing! And welcome to the dev side! :)
Comment 16 Martin Renvoize (ashimema) 2025-03-14 07:57:14 UTC
Created attachment 179305 [details] [review]
Bug 39282: Unit tests for item creation

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize (ashimema) 2025-03-14 07:57:18 UTC
Created attachment 179306 [details] [review]
Bug 39282: Ensure items are created using Item Information values if not using MarcItemFieldsToOrder

When using MarcFieldsToOrder and not MarcItemFieldsToOrder it is expected that all of the item
information will come from the second tab 'Item information' on addorderiso2709.pl

Currently we are not and blank items are beign created

To test:
 1 - Apply unit tests
 2 - prove -v t/db_dependent/Koha/MarcOrder.t
 3 - It fails
 4 - Set MarcItemFields to Order:
     price: 949$p
     quantity: 947$c
     budget_code: 947$f
 5 - Ensure MarcItemFieldsToOrder is blank
 6 - Find a basket in acquisitions
 7 - Add to basket from a file
 8 - Fill item information, set 'Matching' at top of form to 'Do not look for matchign records' and save
 9 - Orders are added
10 - Click on a title
11 - Items do not load and an error is generated, this is because items don't have any values
12 - Check the DB:
     SELECT * FROM items WHERE homebranch IS NULL\G
13 - Note items from order are listed
14 - Apply the rest of the patches
15 - Repeat 6-10
16 - This time the items load successfully

NOTE: This patch alone doesn't address all errors, but subsequent will, keeping separate to address
each issue individually, but test plan can be executed as a whole against all

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize (ashimema) 2025-03-14 07:57:21 UTC
Created attachment 179307 [details] [review]
Bug 39282: Clarify use of budget codes and ids

When getting items from the form on addorderiso2709.pl we are getting actual budget id's,
we only deal with codes when fetching the values from the MARC.
The code here assigns an id into a variable called code - this patch clarifies that and
ensures that we fallback to using the id if one has been passed from the interface form

To test:
0 - Apply first two patches
1 - prove -v t/db_dependent/Koha/MarcOrders.t
2 - It fails
3 - Apply this patch
4 - It fails?

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize (ashimema) 2025-03-14 07:57:24 UTC
Created attachment 179308 [details] [review]
Bug 39282: Use generated MARC record to create item and return itemnumber

When creating items from the form we need to use the MARC record we build from the HTML->XML->MARC

This patch simply uses the variable we create and ensures we pass the itemnumbers back

To test:
1 - prove -v t/db_dependent/Koha/MarcOrders.t
2 - Still failing
3 - Apply this patch
4 - Success!

Phew! We did it.

Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize (ashimema) 2025-03-14 07:58:37 UTC
That took some head-scratching, but I believe we're all correct now.. the actual changes are fairly minimal, but they affect the code flow as expected.

Passing QA
Comment 21 Nick Clemens (kidclamp) 2025-03-14 18:17:21 UTC
Created attachment 179345 [details] [review]
Bug 39282: (follow-up) Use generated budget and don't rely on existing data
Comment 22 Katrin Fischer 2025-03-17 06:43:07 UTC
The bug description is a bit long - maybe it can be shortened a little in favor of a longer description in the release notes?
Comment 23 Katrin Fischer 2025-03-17 07:28:43 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 24 Paul Derscheid 2025-03-19 23:19:49 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.03