Bug 26231

Summary: bulkmarcimport.pl does not import authority if it already has a 001 field
Product: Koha Reporter: David Cook <dcook>
Component: MARC Bibliographic record staging/importAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: aleisha, fridolin.somers, jonathan.druart, julian.maurice, lucas, m.de.rooy, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.05, 19.11.11, 19.05.16
Attachments: Bug 26231: Remove incorrect use of AddAuthority() when 001 present
Bug 26231: Remove incorrect use of AddAuthority() when 001 present
Bug 26231: Remove incorrect use of AddAuthority() when 001 present

Description David Cook 2020-08-18 02:17:07 UTC
I'm trying to import authorities which already have a 001 and bulkmarcimport.pl says that it's worked but it hasn't actually imported anything and no errors have been thrown.

I'm using this command:

koha-shell -c "perl /usr/share/koha/bin/migration_tools/bulkmarcimport.pl -file /home/dcook/auth.mrc -a -fk -d -v -l /tmp/auth.log" <instance>

It appears to be because the value from the 001 is being passed to AddAuthority() which - instead of adding the authority - is trying to update an existing authority.
Comment 1 David Cook 2020-08-18 02:30:44 UTC
Created attachment 108431 [details] [review]
Bug 26231: Remove incorrect use of AddAuthority() when 001 present

This patch removes the use of AddAuthority with a non-empty
authid argument, since that triggers an update rather than an insert.

In practice, the update also fails, but the error isn't raised,
as the database connection doesn't have RaiseError set.

Test plan:
1) Do not apply patch
2) Try to bulkmarcimport an authority file with a 001
3) Observe that the script reports success but no authority is added
4) Apply the path
5) Try to bulkmarcimport an authority file with a 001
6) Observe that the script reports success and the authority is added
Comment 2 Fridolin Somers 2020-09-03 07:10:11 UTC
Looks logical.
Maybe just log that value of 001 will be lost no ?
Comment 3 David Cook 2020-09-04 03:56:37 UTC
(In reply to Fridolin SOMERS from comment #2)
> Looks logical.
> Maybe just log that value of 001 will be lost no ?

Possibly although that's already the case now?
Comment 4 Katrin Fischer 2020-09-15 22:02:18 UTC
I think Bug 12722 - bulkmarcimport.pl: Should we move the old 001 to 035 would deal with the issue of the lost 001. 

I am adding Marcel here, if I IIRC this is related to work he did a while ago. (Bug 18014)
Comment 5 Kyle M Hall 2020-09-21 10:49:14 UTC
Created attachment 110448 [details] [review]
Bug 26231: Remove incorrect use of AddAuthority() when 001 present

This patch removes the use of AddAuthority with a non-empty
authid argument, since that triggers an update rather than an insert.

In practice, the update also fails, but the error isn't raised,
as the database connection doesn't have RaiseError set.

Test plan:
1) Do not apply patch
2) Try to bulkmarcimport an authority file with a 001
3) Observe that the script reports success but no authority is added
4) Apply the path
5) Try to bulkmarcimport an authority file with a 001
6) Observe that the script reports success and the authority is added

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Julian Maurice 2020-09-25 13:51:39 UTC
Created attachment 110761 [details] [review]
Bug 26231: Remove incorrect use of AddAuthority() when 001 present

This patch removes the use of AddAuthority with a non-empty
authid argument, since that triggers an update rather than an insert.

In practice, the update also fails, but the error isn't raised,
as the database connection doesn't have RaiseError set.

Test plan:
1) Do not apply patch
2) Try to bulkmarcimport an authority file with a 001
3) Observe that the script reports success but no authority is added
4) Apply the path
5) Try to bulkmarcimport an authority file with a 001
6) Observe that the script reports success and the authority is added

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 7 Jonathan Druart 2020-09-28 09:10:42 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 8 Lucas Gass 2020-10-16 16:13:36 UTC
backported to 20.05.x for 20.05.05
Comment 9 Aleisha Amohia 2020-10-20 01:58:57 UTC
backported to 19.11.x for 19.11.11
Comment 10 Victor Grousset/tuxayo 2020-10-20 21:08:42 UTC
Backported to 19.05.x branch for 19.05.16