The svc new_biblio and biblio scripts delete items, but it's not clear why. Item creation over svc is a desirable option. The svc API should also support deletion of items - maybe using HTTP DELETE method.
OK, some developments: 1. I convinced the library that it would be better to modify the items to set a state of withdrawn, rather than delete them (because it preserves circulation logs and so on); 2. Bug 7613 will allow creation of items through import_bib; so I am retitling this bug and a patch will be attached as soon as I've finished reviewing it.
Created attachment 8540 [details] [review] svc/bib: support appending ?items=1 to the URL to fetch or modify items
Patch applies cleanly, and now works with the new architecture where Items are separate to Biblio data ans should be handled as such.
http://koha:8080/cgi-bin/koha/svc/bib/1 <?xml version="1.0" encoding="UTF-8"?> <record xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd" xmlns="http://www.loc.gov/MARC21/slim"> <leader>00118nam a22000617a 4500</leader> <controlfield tag="008">120321b xxu||||| |||| 00| 0 eng d</controlfield> <datafield tag="245" ind1=" " ind2=" "> <subfield code="a">w</subfield> </datafield> <datafield tag="999" ind1=" " ind2=" "> <subfield code="c">1</subfield> <subfield code="d">1</subfield> </datafield> </record> http://koha:8080/cgi-bin/koha/svc/bib/1?items=1 <record xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd" xmlns="http://www.loc.gov/MARC21/slim"> <leader>00118nam a22000617a 4500</leader> <controlfield tag="008">120321b xxu||||| |||| 00| 0 eng d</controlfield> <datafield tag="245" ind1=" " ind2=" "> <subfield code="a">w</subfield> </datafield> <datafield tag="999" ind1=" " ind2=" "> <subfield code="c">1</subfield> <subfield code="d">1</subfield> </datafield> <datafield tag="952" ind1=" " ind2=" "> <subfield code="w">2012-03-21</subfield> <subfield code="p">1</subfield> <subfield code="r">2012-03-21</subfield> <subfield code="4">0</subfield> <subfield code="0">0</subfield> <subfield code="9">1</subfield> <subfield code="b">MPL</subfield> <subfield code="1">0</subfield> <subfield code="d">2012-03-21</subfield> <subfield code="7">0</subfield> <subfield code="2">ddc</subfield> <subfield code="s">2012-03-21</subfield> <subfield code="l">2</subfield> <subfield code="a">MPL</subfield> </datafield> </record> GET Works as advertised
Created attachment 8544 [details] [review] Bug 7729 : svc/bib: support appending ?items=1 to the URL to fetch or modify items Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
QA Comments: perlcritic OK (scv/bib) Test on an UNIMARC installation, the patch add a node 995 per item. Passed QA
QA comment: another question: The code says: + my $fullrecord = $record->clone(); then a few lines later: - foreach my $field ($record->field($itemtag)) { + foreach my $field ( $record->field($itemtag) ) { $record->delete_field($field); } then + foreach my $field ( $fullrecord->field($itemtag) ) { + my $one_item_record = $record->clone(); + $one_item_record->add_fields($field); + ModItemFromMarc( $one_item_record, $biblionumber, + $field->subfield($itemsubfield) ); + } so= we clone the record, remove items from old record if there are, then reintroduce item in the new record. Shouldn't we have - foreach my $field ($record->field($itemtag)) { + foreach my $field ( $fullrecord->field($itemtag) ) { $record->delete_field($field); } ?
MJ, please answer to my comment 7 question and switch back to "passed QA" if it's not a bug, or provide a follow-up (and back to passed QA, as it should be a trivial folluw-up)
Pasting the reply that I emailed in and it never appeared: Could you explain the benefit of using $fullrecord in the foreach that deletes from $record, please? My fear is that if future new code between the clone and the delete extend $fullrecord somehow, then deleting all $fullrecord's fields from $record could raise an error. The reason why we readd the item details into $one_item_record is that tests suggested ModItemFromMarc only accepts records with one item, so we send it item-containing records one at a time. It might be more efficient to delete the just-added item and recycle $one_item_record, rather than clone the item-less $record each time: I've not tested it and felt it was probably safest to use a new clone. Thanks for considering this enhancement.
(In reply to comment #9) > Pasting the reply that I emailed in and it never appeared: I'm not sure I understand your answer, but I'm not sure I can explain clearly my concern either. So pushing, please everybody double check to be sure there's no problem
Created attachment 9182 [details] [review] svc/new_bib: support ?items=1 to add POSTed items to the new biblio too
follow-up patch posted to extend new_bib in a similar way, adding support for ?items=1
(In reply to comment #12) > follow-up patch posted to extend new_bib in a similar way, adding support > for ?items=1 MJ, the attachment Bug 7729 : svc/bib: support appending ?items=1 to the URL to fetch or modify items (2.41 KB, patch) has been pushed, right ? So only the follow-up is requiring signoff now ? if i'm right please obsolet the 1st patch (Detail on the right of the patch, then "Edit Detail" link
Created attachment 10738 [details] [review] Bug 7729 : svc/new_bib: support ?items=1 to add POSTed items to the new biblio too Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
(In reply to comment #14) > Created attachment 10738 [details] [review] > Bug 7729 : svc/new_bib: support ?items=1 to add POSTed items to the new > biblio too > > Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> passing QA... looks good $ koha-qa.pl - a3b7e27 Bug 7729 : svc/new_bib: support ?items=1 to add POSTed items to the new biblio too svc/new_bib - perlcritic-progressive tests... OK - perl -c syntax tests... OK - xt/tt_valid.t tests... OK - xt/author/vaild-template.t tests... OK
Maybe it's worth backporting to 3.8