Bug 7729

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.

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

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

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