Bug 37550

Summary: bulkmarcimport.pl dies when adding items throws an exception
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Command-line UtilitiesAssignee: Nick Clemens (kidclamp) <nick>
Status: Needs documenting --- QA Contact: Kyle M Hall (khall) <kyle>
Severity: normal    
Priority: P5 - low CC: bernard.scaife, fridolin.somers, lucas, martin.renvoize, phil, robin, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37478
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.06
Circulation function:
Attachments: Bug 37550: Move item check after error handling
Bug 37550: Unit test for CheckItemPreSave
Bug 37550: CheckItemPreSave should run barcodes through barcodedecode
Sample file
Bug 37550: Move item check after error handling
Bug 37550: Unit test for CheckItemPreSave
Bug 37550: CheckItemPreSave should run barcodes through barcodedecode
Bug 37550: Move item check after error handling
Bug 37550: Unit test for CheckItemPreSave
Bug 37550: CheckItemPreSave should run barcodes through barcodedecode

Description Nick Clemens (kidclamp) 2024-08-01 17:37:23 UTC
The script runs item addition in an eval, and skips the record if there is an error. Before we deal with the error, however, we try to see if items were added by accessing a hash of itemnumebrs returned. When we throw an exception, the variable is not set. We should move that line after the error handling
Comment 1 Nick Clemens (kidclamp) 2024-08-01 18:08:25 UTC
Created attachment 169959 [details] [review]
Bug 37550: Move item check after error handling

To test:
1 - Grab the sample file on this bug report
2 - perl misc/migration_tools/bulkmarcimport.pl -b --file=spaceditems.pl -v
3 - An exception for 'Duplicate ID' is thrown and script dies
4 - Apply patch
5 - run script again
6 - The script finishes, no items are added, but record is
Comment 2 Nick Clemens (kidclamp) 2024-08-01 18:08:27 UTC
Created attachment 169960 [details] [review]
Bug 37550: Unit test for CheckItemPreSave
Comment 3 Nick Clemens (kidclamp) 2024-08-01 18:08:29 UTC
Created attachment 169961 [details] [review]
Bug 37550: CheckItemPreSave should run barcodes through barcodedecode

In Koha::Item we run a barcode through barcodedecode before any save.
We should do the same when checking barcodes to avoid a duplicate error
when the barcode is cleaned before it is written to the DB

To test:
1 - Follow previous test plan
2 - Note that after this patch is applied there is no exception
    All items are skipped as duplicate barcodes
Comment 4 Phil Ringnalda 2024-08-23 00:23:40 UTC
I don't see the sample file.
Comment 5 Nick Clemens (kidclamp) 2024-08-23 12:41:42 UTC
Created attachment 170630 [details]
Sample file
Comment 6 Phil Ringnalda 2024-08-23 15:38:36 UTC
Created attachment 170648 [details] [review]
Bug 37550: Move item check after error handling

To test:
1 - Grab the sample file on this bug report
2 - perl misc/migration_tools/bulkmarcimport.pl -b --file=spaceditems.pl -v
3 - An exception for 'Duplicate ID' is thrown and script dies
4 - Apply patch
5 - run script again
6 - The script finishes, no items are added, but record is

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 7 Phil Ringnalda 2024-08-23 15:38:39 UTC
Created attachment 170649 [details] [review]
Bug 37550: Unit test for CheckItemPreSave

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 8 Phil Ringnalda 2024-08-23 15:38:41 UTC
Created attachment 170650 [details] [review]
Bug 37550: CheckItemPreSave should run barcodes through barcodedecode

In Koha::Item we run a barcode through barcodedecode before any save.
We should do the same when checking barcodes to avoid a duplicate error
when the barcode is cleaned before it is written to the DB

To test:
1 - Follow previous test plan
2 - Note that after this patch is applied there is no exception
    All items are skipped as duplicate barcodes

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 9 Kyle M Hall (khall) 2024-11-01 12:18:43 UTC
Created attachment 173868 [details] [review]
Bug 37550: Move item check after error handling

To test:
1 - Grab the sample file on this bug report
2 - perl misc/migration_tools/bulkmarcimport.pl -b --file=spaceditems.pl -v
3 - An exception for 'Duplicate ID' is thrown and script dies
4 - Apply patch
5 - run script again
6 - The script finishes, no items are added, but record is

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall (khall) 2024-11-01 12:18:50 UTC
Created attachment 173869 [details] [review]
Bug 37550: Unit test for CheckItemPreSave

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall (khall) 2024-11-01 12:18:52 UTC
Created attachment 173870 [details] [review]
Bug 37550: CheckItemPreSave should run barcodes through barcodedecode

In Koha::Item we run a barcode through barcodedecode before any save.
We should do the same when checking barcodes to avoid a duplicate error
when the barcode is cleaned before it is written to the DB

To test:
1 - Follow previous test plan
2 - Note that after this patch is applied there is no exception
    All items are skipped as duplicate barcodes

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Katrin Fischer 2024-11-07 16:33:45 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 13 Lucas Gass (lukeg) 2024-12-05 21:51:00 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 14 Fridolin Somers 2024-12-18 10:52:32 UTC
Arf this script changed a lot since Bug 29440
Please provide patches for 23.111.x if you can
Comment 15 Wainui Witika-Park 2025-06-24 06:13:58 UTC
Not backporting to 22.11 unless requested