Bug 7131 - way to overlay items in in marc import
Summary: way to overlay items in in marc import
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P3 new feature (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-01 19:53 UTC by Nicole C. Engard
Modified: 2018-06-01 06:20 UTC (History)
12 users (show)

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


Attachments
Patch adds the ability to overlay items when Staging (12.22 KB, patch)
2012-12-31 21:01 UTC, Elliott Davis
Details | Diff | Splinter Review
Patch adds the ability to overlay items when Staging (13.19 KB, patch)
2013-01-08 20:15 UTC, Elliott Davis
Details | Diff | Splinter Review
Patch adds the ability to overlay items when Staging (13.19 KB, patch)
2013-03-07 19:38 UTC, Elliott Davis
Details | Diff | Splinter Review
Patch adds the ability to overlay items when Staging (13.34 KB, patch)
2013-03-07 20:38 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7131 - way to overlay items in in marc import (12.45 KB, patch)
2013-03-26 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - way to overlay items in in marc import (12.50 KB, patch)
2013-03-26 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - Followup 1 - Clean up sub BatchCommitItems (5.32 KB, patch)
2013-04-05 11:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - Followup 2 - Allow overlaying by barcode (2.96 KB, patch)
2013-04-05 11:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - Followup 2 - Allow overlaying by barcode (4.64 KB, patch)
2013-04-05 13:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - Followup 2 - Allow overlaying by barcode (6.00 KB, patch)
2013-04-05 14:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - Followup 2 - Allow overlaying by barcode (5.99 KB, patch)
2013-04-05 14:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7131 - Followup 2 - Allow overlaying by barcode (6.04 KB, patch)
2013-06-06 14:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Test Marc File (526 bytes, application/octet-stream)
2013-06-06 15:54 UTC, Kyle M Hall
Details
Test Marc File - Overlay by Itemnumber (386 bytes, application/octet-stream)
2013-06-06 15:56 UTC, Kyle M Hall
Details
Test Marc File - Overlay by Barcode (393 bytes, application/octet-stream)
2013-06-06 15:59 UTC, Kyle M Hall
Details
[Signed off] Bug 7131 - way to overlay items in in marc import (12.56 KB, patch)
2013-06-13 21:55 UTC, Melia Meggs
Details | Diff | Splinter Review
[Signed off] Bug 7131 - Followup 2 - Allow overlaying by barcode (6.07 KB, patch)
2013-06-13 22:26 UTC, Melia Meggs
Details | Diff | Splinter Review
[Signed off] Bug 7131 - Followup 1 - Clean up sub BatchCommitItems (5.45 KB, patch)
2013-06-13 22:29 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 7131 - Followup 2 - Allow overlaying by barcode (6.12 KB, patch)
2013-08-19 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7131 - QA Followup (799 bytes, patch)
2013-10-03 17:23 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 Nicole C. Engard 2011-11-01 19:53:25 UTC
Scenario:

I have put in an order with an on order item attached to it.  The item has been received and my cataloger goes in to OCLC and creates a MARC record with a 952 with the barcode and other information.  When she goes to import the records in to Koha she can match on the ISBN for the bib record, but there is no way to say that this new item should overlay the on order item record. 

Ideas:

I'm not sure how to do this and it's obviously just a wish list item.  I'm open to ideas.
Comment 1 Katrin Fischer 2011-11-01 21:59:35 UTC
Hi Nicole,

It's on my wish list too :)

I think the problem is, that at the moment we have no way to overwrite item records. Which is mostly good. Once a barcode is in the database, the import scripts will not touch it. I think it would be neat to have way to specify fields and subfields that can be overwritten, while others are blocked (like information about current checkout or information important for statistics).
I think this would not only be cool for item information, but also for overlaying bibliographic records.
Comment 2 Ian Walls 2011-11-02 06:36:06 UTC
I think this would require us to have not only Biblio matching rules, but Item matching rules as well.  Barcode would be a default rule (like ISBN/ISSN are), and itemnumber would be the equivalent to matching on biblionumber.

With some of the forthcoming developments, we'd also need to handle Order record and Subscription record matching, as well.  Nothing to worry about today, but something we should plan for in the structural change.

We also need Authority matching rules, so we can enable authority import via the staff client.
Comment 3 Elliott Davis 2012-12-31 21:01:35 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2013-01-02 09:10:08 UTC
Hi Elliott,
reading your test plan I have some questions about this new feature:
How are the items matched? By barcode or itemnumber?
Will all subfields be overlayed? What happens when the item is currently on loan?
Comment 5 Elliott Davis 2013-01-02 17:30:27 UTC
Hey Katrin,

Items are matched on itemnumber.  If no match is found the item is added.  

The overlay function is just using the built in function ModItemFromMarc so only fields listed in the marc file.  I am unsure of how the internal overlay functions in Koha work if an item is on loan and its data is overlaid.
Comment 6 Katrin Fischer 2013-01-02 20:02:22 UTC
Hi Elliott, 

sorry, but I need a bit more information (didn't you see that coming?) ;)

We have to make sure that the items are not overlayed in a way that destroys critical data like the due date. Could you check please and make sure it doesn't?
I think it's possible to change all subfields in the interface if you configure your frameworks accordingly. So we need some big red warnings or better make it impossible to do something stupid. 

Also we need to document that it only matches on itemnumber, as I would have expected it to match on barcode. Afaik the import scripts work mostly with barcode, warning you if a barcode already exists in the database and not adding the item in that case.

There is currently no "overlay" option for items in Koha I am aware of. I can only think that one of the existing features works like an overlay - maybe editing items?
Comment 7 Elliott Davis 2013-01-04 14:29:10 UTC
Per a conversation with Katrin I think this is what needs to happen with this patch:

The item match needs to happen on the barcode in addition to the itemnumber.  

There also needs to be a check added to make sure the date_due isn't going to be overwritten.  Since the record is a MARC::Record object we can just remove the field.
Comment 8 Elliott Davis 2013-01-07 22:52:02 UTC
Cait,

I have been mulling this over for a few days since we spoke and I think that the barcode matching could be:

A) Added as a follow up patch by a community member that would like to see item overlays on barcodes 

B) Added in a separate ticket as a dependency to this ticket.  

I say this because it lies outside the scope of the original spec.  

I think the removal of due dates can be added because of the potential to damage item records.  

Let me know what you think.
Comment 9 Katrin Fischer 2013-01-08 08:31:52 UTC
I am OK with that, but I would like to see it documented, maybe as a hint beside the item match option? And on the help page? My reasoning was to not have something work in an unexpected way - like people wondering why they get no matches. Matching on itemnumbers means, this is going to work only for reimporting data.
Comment 10 Elliott Davis 2013-01-08 20:15:06 UTC Comment hidden (obsolete)
Comment 11 Elliott Davis 2013-01-08 20:15:53 UTC
Cait,

When this patch makes it through Nicole will add the additional matching info to the manual with makes it through to the help files.

Elliott
Comment 12 Kyle M Hall 2013-03-07 13:07:19 UTC
Comment on attachment 14475 [details] [review]
Patch adds the ability to overlay items when Staging

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

::: C4/ImportBatch.pm
@@ +696,5 @@
>      $sth->execute();
>      while (my $row = $sth->fetchrow_hashref()) {
>          my $item_marc = MARC::Record->new_from_xml(StripNonXmlChars($row->{'marcxml'}), 'UTF-8', $row->{'encoding'});
> +	#delete date_due subfield as to not accidentally delete item checkout due dates
> +	$item_marc->field('952')->delete_subfield(code => 'q');

This is a problem. Not all frameworks will have items.onloan mapped to 952$q. We need to look up the problem tag/subtag here using C4::Biblio::GetMarcFromKohaField or get the value via C4::Biblio::GetRecordValue.
Comment 13 Kyle M Hall 2013-03-07 13:09:23 UTC
(In reply to comment #12)
> Comment on attachment 14475 [details] [review] [review]
> Patch adds the ability to overlay items when Staging
> 
> Review of attachment 14475 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/ImportBatch.pm
> @@ +696,5 @@
> >      $sth->execute();
> >      while (my $row = $sth->fetchrow_hashref()) {
> >          my $item_marc = MARC::Record->new_from_xml(StripNonXmlChars($row->{'marcxml'}), 'UTF-8', $row->{'encoding'});
> > +	#delete date_due subfield as to not accidentally delete item checkout due dates
> > +	$item_marc->field('952')->delete_subfield(code => 'q');
> 
> This is a problem. Not all frameworks will have items.onloan mapped to
> 952$q. We need to look up the problem tag/subtag here using
> C4::Biblio::GetMarcFromKohaField or get the value via
> C4::Biblio::GetRecordValue.

I just realized GetRecordValue will be useless in this scenario, you need to use GetMarcFromKohaField. ($MARCfield,$MARCsubfield)=GetMarcFromKohaField($kohafield,$frameworkcode);
Comment 14 Elliott Davis 2013-03-07 19:38:23 UTC Comment hidden (obsolete)
Comment 15 Elliott Davis 2013-03-07 19:39:02 UTC
Changed onloan deletion check from hardcoded value to framework value based on biblionumber.
Comment 16 Kyle M Hall 2013-03-07 20:20:01 UTC
Elliot, I think you forget to amend your commit. This patch appears to be the same as the previous patch.
Comment 17 Elliott Davis 2013-03-07 20:38:48 UTC Comment hidden (obsolete)
Comment 18 Alex Arnaud 2013-03-19 14:47:03 UTC
What do you mean by "You should see updated item information after the overlay" ?
Do we expect to keep all information in koha or file data should overwrite item ? 

I save a record (with 1 item) to a marc file. 
I change the following item information:
  - Change current location,
  - check it out,
  - place an hold,

I imported the file and make it matches the record:
item is checked out and hold placed => Ok !
But location has not been overwritten with file data.

I have the same behavior without the patch.
Seems it doesn't work.
Comment 19 Alex Arnaud 2013-03-19 15:20:43 UTC
Please forget my last comment. But i found another problem.

I selected "Replace items if matching bib was found" when staging record for import. But when going to manage staged MARC records for the this batch, item processing is set to "Always add item".

I can change it to select "Replace items if matching bib was found" again, but clicking on "Aplly differente matching rules" set it to "Always add item" once again.
Comment 20 Kyle M Hall 2013-03-26 11:37:10 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-03-26 12:43:13 UTC
I have been unable to reproduce your problem here.

(In reply to comment #19)
> Please forget my last comment. But i found another problem.
> 
> I selected "Replace items if matching bib was found" when staging record for
> import. But when going to manage staged MARC records for the this batch,
> item processing is set to "Always add item".
> 
> I can change it to select "Replace items if matching bib was found" again,
> but clicking on "Aplly differente matching rules" set it to "Always add
> item" once again.
Comment 22 Kyle M Hall 2013-03-26 12:43:52 UTC Comment hidden (obsolete)
Comment 23 Brendan Gallagher 2013-03-27 21:00:21 UTC
(In reply to comment #6)
> Hi Elliott, 
> 
> sorry, but I need a bit more information (didn't you see that coming?) ;)
> 
> We have to make sure that the items are not overlayed in a way that destroys
> critical data like the due date. Could you check please and make sure it
> doesn't?
> I think it's possible to change all subfields in the interface if you
> configure your frameworks accordingly. So we need some big red warnings or
> better make it impossible to do something stupid. 
> 
> Also we need to document that it only matches on itemnumber, as I would have
> expected it to match on barcode. Afaik the import scripts work mostly with
> barcode, warning you if a barcode already exists in the database and not
> adding the item in that case.
> 
> There is currently no "overlay" option for items in Koha I am aware of. I
> can only think that one of the existing features works like an overlay -
> maybe editing items?

Hi Cait - 

I see no reason as to why it couldn't just match on itemnumber alone?  Isn't the itemnumber "more important" than the barcode as far as database relations are concerned.  I mean if it doesn't match on itemnumber then it's not in the database - so there for no match.

The reason that I say this - is that this is for order records.  And the workflow around here is that the onorder item doesn't have a barcode until it is fully processed - which happens after it's ordered or received?  Sorry I know now is not the right time for this discussion and we should deal with after the release of 3.12...  But we are talking with the customer who submitted this enhancement - and they will drop this development if we "have" to match on barcode too.  I hope that make sense.

Brendan
Comment 24 Katrin Fischer 2013-03-27 21:08:55 UTC
Hi Brendan,

I thought this was dealt with the comment Elliott put in - it should be clear that it matches only on itemnumber, so will only work for items you exported from Koha. Because else you can't match.

I think this feature could also be useful for migrations, but therefore matching on barcode would be nice. :) Not sure what this library's workflow for on order items is like.

Elliot put this comment in the commit message, but I can't see it on the patch right now and too tired to test:
[UPDATE]: warning added to use only with existing items
Comment 25 Jared Camins-Esakov 2013-03-27 21:16:04 UTC
(In reply to comment #23)
> I see no reason as to why it couldn't just match on itemnumber alone?  Isn't
> the itemnumber "more important" than the barcode as far as database
> relations are concerned.  I mean if it doesn't match on itemnumber then it's
> not in the database - so there for no match.

It could, but that's not the way people would necessarily expect this to work. If matching can be done only on item number, item overlay can be done *only if the records in question were exported from Koha*, which means no post-migration corrections. That's why the documentation would need to be very clear that matching works only with item numbers.
Comment 26 Brendan Gallagher 2013-03-27 21:34:27 UTC
(In reply to comment #25)
> (In reply to comment #23)
> > I see no reason as to why it couldn't just match on itemnumber alone?  Isn't
> > the itemnumber "more important" than the barcode as far as database
> > relations are concerned.  I mean if it doesn't match on itemnumber then it's
> > not in the database - so there for no match.
> 
> It could, but that's not the way people would necessarily expect this to
> work. If matching can be done only on item number, item overlay can be done
> *only if the records in question were exported from Koha*, which means no
> post-migration corrections. That's why the documentation would need to be
> very clear that matching works only with item numbers.

Totally Agreed (I for some reason had it in my notes that this HAD to be both barcode and itemnumber at the sametime - good to know - but I do see the importance of barcode matching - so we should add that in too).  Great I will bring this back up when it's the correct time for new enhancements.
Comment 27 Kyle M Hall 2013-04-05 11:55:51 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2013-04-05 11:55:58 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-04-05 13:49:23 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-04-05 14:01:01 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2013-04-05 14:29:55 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2013-06-06 14:27:54 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2013-06-06 15:54:21 UTC
Created attachment 18709 [details]
Test Marc File

A basic MARC record with 2 items attached
Comment 34 Kyle M Hall 2013-06-06 15:56:06 UTC
Created attachment 18710 [details]
Test Marc File - Overlay by Itemnumber

The same MARC record, but with one item removed ( itemnumber 1 ) and the second item edited such that the barcode is changed to 10000002, where previously it was 00000002.
Comment 35 Kyle M Hall 2013-06-06 15:59:01 UTC
Created attachment 18711 [details]
Test Marc File - Overlay by Barcode

The same original MARC file, but this time with the item having the barcode 00000002 removed, the 952$9 ( itemnumber ) field removed from the remaining item, and the callnumber altered from "TEST ITEM 1" to "TEST ITEM 1 - Updated".
Comment 36 hbankhead 2013-06-06 17:45:50 UTC
I Henry Bankhead hereby approve this development and attest to the proven fact that it performs as expected. This I solemnly *swear*.
Henry Bankhead
Comment 37 Melia Meggs 2013-06-13 21:55:15 UTC
Created attachment 18967 [details] [review]
[Signed off] Bug 7131 - way to overlay items in in marc import

When staging biblios with items attached you previously had only 2 options (add/don't add).
This patch adds a third option to replace an item record if a match is found else it adds the item.

Test Plan:
1) Stage a file of biblios with items attached.
2) Import the batch into the catalog.
3) Run the indexer so the matcher will match
4) Modify the item data for at least one bib in the file
5) Re-stage the file with the item matching option set to "Replace items if matching bib was found"
6) Let the indexer run again
7) You should see updated item information afte the overlay

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov>
Comment 38 Melia Meggs 2013-06-13 22:26:46 UTC Comment hidden (obsolete)
Comment 39 Melia Meggs 2013-06-13 22:29:35 UTC
Created attachment 18969 [details] [review]
[Signed off] Bug 7131 - Followup 1 - Clean up sub BatchCommitItems
Comment 40 Melia Meggs 2013-06-13 22:34:06 UTC
Phew!  Finally got Henry's name on all three of these patches.  He has tested overlaying items thoroughly, so I am adding his signoff.
Comment 41 Katrin Fischer 2013-07-17 13:53:54 UTC
Hi Kyle,
could you please also add the new option to the command line tool for staging marc records (/misc/stage_file.pl)?
Comment 42 Marcel de Rooy 2013-08-19 11:44:48 UTC
Created attachment 20455 [details] [review]
Bug 7131 - Followup 2 - Allow overlaying by barcode

This patch adds the ability to overlay by either itemnumber,
or barcode. Itemnumbers take precendence over barcodes, which
allows us to batch update item barcodes with an overlay.

Test Plan:
1) Create a new record with 2 items, make sure to give it a unique ISBN
2) Download the record as MARCXML
3) Edit the MARC XML
   a) Delete one of the two items
   b) Change the barcode in the barcode field to something unused
4) Transform the xml file into marc with xml2marc
5) Browse to 'Stage MARC records for import'
6) Upload the binary marc file
7) Choose the following options:
    Record matching rule: ISBN
    Action if matching record found: Ignore incoming record
    Action if no match is found: Ignore incoming record
    Check for embedded item record data: Yes
    How to process items: Replace items if matching bib was found
8) Click 'Stage for import' button
9) Verify a matching record was found, then click 'Manage staged records' link
10) Verify the rules are still set correctly
11) Click 'Import this batch into the catalog'
12) The import should tell you:
    1 record was ignored
    1 item was replaced
13) View the record details and verify the item's barcode was replaced
    with your updated barcode value
14) Download the record as MARCXML
15) Edit the MARC XML
    a) Delete one of the two items
    b) Delete the itemnumber field for the remaining item
    c) Alter the item's callnumber to a new value
16) Repeat steps 4 through 12
17) View the record details and verify the item's callnumber was replace
    with your updated callnumber value

Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov>
Comment 43 Marcel de Rooy 2013-08-19 11:45:32 UTC
Trivial rebase for followup no 2 (updatedatabase)
Comment 44 Marcel de Rooy 2013-08-19 12:05:59 UTC
When I import the test file test.marc, no items are created (though the form says that two items have been added). Item processing was Always add items. (There was no match.)

BTW My log contains several lines like: 
stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at /usr/local/lib64/perl5/Template/Provider.pm line 964., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/tools/stage-marc-import.pl

Can you reproduce or clarify?
Comment 45 Katrin Fischer 2013-08-19 12:11:14 UTC
Hi Marcel, maybe you don't have the branchcode in your installation? There is a known bug for it giving you a false positive message when that is the case and the items are not added. We had this happening quite recently when testing something.
Comment 46 Kyle M Hall 2013-08-19 12:12:34 UTC
> BTW My log contains several lines like: 
> stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at
> /usr/local/lib64/perl5/Template/Provider.pm line 964., referer:
> http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/tools/stage-marc-import.pl

I've never seen any errors like that. Do you get them on master? It seems very odd that these patches would trigger those errors, as none of them modify file IO in any way!
Comment 47 Marcel de Rooy 2013-08-19 12:14:16 UTC
(In reply to Kyle M Hall from comment #46)
> > BTW My log contains several lines like: 
> > stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at
> > /usr/local/lib64/perl5/Template/Provider.pm line 964., referer:
> > http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/tools/stage-marc-import.pl
> 
> I've never seen any errors like that. Do you get them on master? It seems
> very odd that these patches would trigger those errors, as none of them
> modify file IO in any way!

Will try the same import without these patches and check the log :)
Comment 48 Marcel de Rooy 2013-08-19 12:18:23 UTC
> Will try the same import without these patches and check the log :)
The import step produces these warnings already in current master for me..
Comment 49 Marcel de Rooy 2013-08-19 12:24:47 UTC
(In reply to Katrin Fischer from comment #45)
> Hi Marcel, maybe you don't have the branchcode in your installation? There
> is a known bug for it giving you a false positive message when that is the
> case and the items are not added. We had this happening quite recently when
> testing something.

Katrin,
Thanks! I found that bug 6852.
So I understand that the item contains a branchcode, and if that is not found, it just reports Fine but adds no items. Great :)

I added MPL for this report and now the items appear.
Comment 50 Marcel de Rooy 2013-08-19 13:21:15 UTC
Step-by-step :)
If I import with matching and no match is found, it follows Add incoming record, but if i chose Replace existing items for Item processing, now I am facing that the record is added but no items are added/replaced. 

This makes me wonder if we also need some two-way choice for item processing: what if we found a match and what if we did not? 
Or would it make more sense to just add items when there is no match and the items rule is Replace?
Note that the combo already contains two conditional options (add only if..).
Comment 51 Katrin Fischer 2013-09-29 21:08:29 UTC
Still applies cleanly and passes all the tests.
Comment 52 Katrin Fischer 2013-10-03 16:17:38 UTC
Starting to test this...
Comment 53 Katrin Fischer 2013-10-03 17:10:09 UTC
Posting some first testing results - unit tests is a bigger one, but it would also be good to fix the warning in the script:

Passes all tests and QA script.

0) Database changes done by updatedatabase and in kohastructure match.

1) Patch touches core routines in C4::ImportBatch.pm , but doesn't 
provide regression tests:
- BatchCommitRecords
- BatchCommitItems
- BatchRevertRecords

2) Regression test: stage_file.pl and commit_file.pl command line scripts

Note: Adding items works as before, new options for item overlay should 
  be added to stage_file.pl later on, but we need to make sure there are 
  no regressions.
   
Staging file:
  ./stage_file.pl --file ../../data/Katrin-biblios.mrc --add-items --comment "something" --match 3
Committing staged records to catalog:
  ./commit_file.pl --batch-number 2
  Produces lots of warnings, that are not there on master:
  Use of uninitialized value $bib_items_replaced in addition (+) at /home/katrin/kohaclone/C4/ImportBatch.pm line 639.
  (minor)
Reverting bath import:
  ./commit_file.pl --revert --batch-number 2
Comment 54 Kyle M Hall 2013-10-03 17:23:37 UTC
Created attachment 21797 [details] [review]
Bug 7131 - QA Followup
Comment 55 Katrin Fischer 2013-10-03 20:15:02 UTC
Hm there is a bug caused by this:

This is the result of importing a batch from command line without the patch. 
All numbers shown are correct.

MARC record staging report
------------------------------------
Input file:                 ../../Downloads/bib-4.utf8
Record type:                biblio
Number of input records:    1
Number of valid records:    1
Number of invalid records:  0
Number of records matched:  1
Number of items parsed:  4

And that's the result with the patch:

MARC record import report
----------------------------------------
Batch number:                    7
Number of new records added:     0
Number of records replaced:      1
Number of records ignored:       4
Number of items added:           0
Number of items ignored:         0

The number of records ignored should be the number of items ignored.

Commands used:
./stage_file.pl --file ../../Downloads/bib-4.utf8 --add-items --comment "something" --match 3
./commit_file.pl --batch-number 8

Matching rule 3 is 001 in my database, as my records all have unique identifiers.
Comment 56 Katrin Fischer 2013-10-03 20:41:27 UTC
Ok, posting my complete testing notes here for reference:

Passes all tests and QA script.

0) Database changes done by updatedatabase and in kohastructure match.

1) Patch touches core routines in C4::ImportBatch.pm , but doesn't 
provide regression tests:
- BatchCommitRecords
- BatchCommitItems
- BatchRevertRecords

2) Regression test: stage_file.pl and commit_file.pl command line scripts

Note: Adding items works as before, new options for item overlay should 
  be added to stage_file.pl later on, but we need to make sure there are 
  no regressions.
   
Staging file:
  ./stage_file.pl --file ../../data/Katrin-biblios.mrc --add-items --comment "something" --match 3
Committing staged records to catalog:
  ./commit_file.pl --batch-number 2
Reverting bath import:
  ./commit_file.pl --revert --batch-number 2
  
PROBLEM: The summaries are not correct regarding the numbe rof items  with the 
patch applied 

Without patch:
MARC record staging report
------------------------------------
Input file:                 ../../Downloads/bib-4.utf8
Record type:                biblio
Number of input records:    1
Number of valid records:    1
Number of invalid records:  0
Number of records matched:  1
Number of items parsed:  4

With patch applied:
MARC record import report
----------------------------------------
Batch number:                    7
Number of new records added:     0
Number of records replaced:      1
Number of records ignored:       4
Number of items added:           0
Number of items ignored:         0
  
3) Regression test: Authority import

Authority staging and importing works, no regressions found.
I noticed that item processing options are offered on the managed MARC
imports page, but this has already been reported as bug 9067.

4) Bibliographic records import

Tried different options for importing:
- Always add items.
  Still adds items, when the barcode doesn't exist in the database.
  If barcode already exists, no item is added.
- Ignore record if there is a match, but replace items.
  Record is ignored, item gets replaced.
* Delete item with the itemnumber in the import file.

PROBLEM:
- Replace incoming record on match, replace items.
  Item will not be created, but summary gives a wrong positive:
  Number of items replaced  1
  We should either create the item with a new itemnumber in that case, 
  or give a proper error message.

So 2 smaller problems, but overall it seems to work quite well.
Comment 57 Katrin Fischer 2013-10-03 21:00:19 UTC
After talking to Galen I am setting this to passed QA, but with some conditions:
 - Unit tests must be provided 
 - Summaries must be fixed
by the end of KohaCon to make this go in.
Comment 58 Galen Charlton 2013-10-30 04:38:20 UTC
Pushed to master, along of follow-ups.  One of the follow-ups teaches stage_file.pl how to set the item action and fixes commit_file.pl to display the summary counts correctly.

I've elected to waive requiring test cases, but they should be added soon.

Thanks, Kyle and Elliott!