Bug 17608 - Fix behaviour of bulkmarcimport.pl -insert
Summary: Fix behaviour of bulkmarcimport.pl -insert
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-10 14:54 UTC by Magnus Enger
Modified: 2018-02-26 10:37 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2016-11-10 14:54:21 UTC
According to the POD in bulkmarcimport.pl:

    -insert
        if set, only insert when possible

This works as expected for bibliographic records, if -insert is specified and a records already exists (based on the criteria specified by the -match option), the record is skipped and no record is inserted. 

But I do find the behaviour related to items a bit surprising. Even if the record is skipped and not inserted, any items attached to the record in the file that is being imported is attached to the matching record. I would expect items to be ignored if the record exists and is skipped. 

So the question is: is this behaviour a bug or a feature? 

Even if it is a bug, I guess we must assume that some libraries out there are relying on the current behaviour of bulkmarcimport.pl, so that the best way to get the behaviour I expect is to add another command line switch, to ignore items for matching records. This way existing users will not get nasty surprises and I will get the behaviour I expect. 

Opinions?
Comment 1 David Gustafsson 2016-12-22 11:06:18 UTC
I'm currently looking into this as well. I came to the conclusion that the AddItemBatchFromMarc branch should really be executed on insert. The sub-documentation states that:

"The goal of this API is to have a similar effect to using AddBiblio
and AddItems in succession, but without inefficient repeated
parsing of the MARC XML bib record."

So it feels strange that is is run on update. Besides, I can't imagine a scenario where you would want to overwrite existing holdings items? I'm no librarian so some input on the practical aspects of this would be nice.
Comment 2 David Gustafsson 2016-12-22 11:08:22 UTC
lost a word: *should really ONLY be executed on insert
Comment 3 Katrin Fischer 2016-12-22 11:28:27 UTC
There is a feature to make it possible to update items matching the barcode or itemnumber, but I am not sure if this is relevant for bulkmarcimport. It's part of the staged marc import procedure.
Comment 4 David Gustafsson 2016-12-22 12:11:07 UTC
As far as I understand it no matching on barcode is performed in the marcbulkupdate-script. There is an "dedupebarcode"-option. If an existing barcode is found, but that does something quite different. If an item with the same barcode is found, the imported item's barcode is wiped, and imported as a new item.
Comment 5 David Gustafsson 2016-12-22 17:19:44 UTC
Have now investigated this further, and as far as I'm concerned it it a bit of a mess.

- When a record is inserted, the AddBiblio is called, which do not call "_strip_item_fields" (more on this later). AddItemBatchFromMarc is later called, and the items contained in the imported marc record are created.

- If the import is run a second time, and if the record is correctly matched with the previously created one, ModBiblio is called instead. Inside of ModBiblio _strip_item_fields is called (this is important). When "AddItemBatchFromMarc" is later called, the items have been stripped, so they are never again processed (as far as I understand it). If you remove the (holdings) items in Koha, and run the import again, still no items. I'm working with a modified script, so I will have to verify this behaviour with the original. But seems odd to me.

ModBiblioMarc is also called unnecessarily for updated items (it has already been called in ModBiblio). So this invocation should be done a in different logical branch for new records insertion.

So to sum things up, if I have understood the code correctly:

- Right now, items are ever only created on insert (and the deduplication logic is only used here, does not apply for updates).

- This should be separated into a separate logical branch, and not called also for updates (since it is bad for performance, and very confusing if trying to understand the code).

Besides, nowhere is the items inserted back into the marcxml. I don't know if this is by design, but could be a possible bug.
Comment 6 David Gustafsson 2016-12-23 12:16:22 UTC
I looked into the last issue a bit more. Tried adding items through the interface and looking at the resulting marcxml in the database (and having a look at the code base) and it seems to me like the items are never inserted into the marcxml saved in the biblioitems table, but can be inserted on the fly in some cases when fetching records, which seems reasonable. So in that case bulkimportmarc is consistent with standard behavior. The thing that confused me slightly is that there is an out-commented ModBiblioMarc in the AddItemBatchFromMarc:
# update the MARC biblio
# $biblionumber = ModBiblioMarc( $record, $biblionumber, $frameworkcode );

If one would comment out, that I believe the items would actually be inserted back into the database marcxml since the record has been modified (with items added) previously in the sub.
Comment 7 Magnus Enger 2016-12-23 12:20:40 UTC
(In reply to David Gustafsson from comment #6)
> I looked into the last issue a bit more. Tried adding items through the
> interface and looking at the resulting marcxml in the database (and having a
> look at the code base) and it seems to me like the items are never inserted
> into the marcxml saved in the biblioitems table, but can be inserted on the
> fly in some cases when fetching records, which seems reasonable. 

That is the expected behaviour, yes. It used to be that items (952 fields) were stored in the marcxml, but they were removed from there a few years ago. See bug 5579 for the details.
Comment 8 Magnus Enger 2018-02-22 11:09:58 UTC
Some further observations:

I have a bunch of MARC-records in a file that I import with:

bulkmarcimport.pl -insert -match=Control-number,001 -l /tmp/insert.log

(I use some more options as well, of course, but these are the interesting ones), because I want to check if the records exist (based on the value in 001) and only add them if they do not already exist in Koha. When I look at the log, I see something like this:

126210;insert;ok
126210;insert;ok
103703;insert;warning : already in database
103703;insert;ok
126211;insert;ok
126211;insert;ok

126210 and 126211 are OK, they did not exist and have now been added. 

Since the script has correctly figured out that 103703 is already in the database, I would expect it to be skipped. But when I look at the timestamps in the database it turns out the entries in the biblio and biblioitems tables were not affected, but biblio_metadata *was* actually updated. And the item was added to the record. This means I now have two items on this record, where there should just be one. 

This behaviour seems to be caused by having the logic for update/insert on lines 453-477 of bulkmarcimport, followed by calls to AddItemBatchFromMarc on line 479 (this adds the items, even if -insert was specified and the record was found to exist in koha) and ModBiblioMarc on line 487 (this updates biblio_metadata).
Comment 9 David Gustafsson 2018-02-22 17:50:19 UTC
We (Gothenburg University Library) run a heavily modified version of bulkmarcimport.pl where this issue should be fixed. We have not have the time to separate out all fixes into separate issues, and as one big patch the chance of getting it passed review is probably pretty low. Plus I think authorities might be broken (since we don't use that functionality). Here it is if you want to try it out: https://github.com/ub-digit/Koha/tree/bulkmarcimport
Comment 10 Magnus Enger 2018-02-26 10:37:42 UTC
(In reply to David Gustafsson from comment #9)
> Here it is if you want to try it out:
> https://github.com/ub-digit/Koha/tree/bulkmarcimport

Thanks! 

The community does tend to favour patches that make one logical change at a time, yes. So if you could split your work up into loical "chunks" and submit them, that would be great!