Bug 7729 - svc API should allow modification of items
Summary: svc API should allow modification of items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: MJ Ray (software.coop)
QA Contact: Mason James
URL: http://koha:8080/svc/biblio
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 18:51 UTC by MJ Ray (software.coop)
Modified: 2013-05-23 06:23 UTC (History)
4 users (show)

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


Attachments
svc/bib: support appending ?items=1 to the URL to fetch or modify items (2.39 KB, patch)
2012-03-22 16:42 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 7729 : svc/bib: support appending ?items=1 to the URL to fetch or modify items (2.41 KB, patch)
2012-03-22 20:48 UTC, Chris Cormack
Details | Diff | Splinter Review
svc/new_bib: support ?items=1 to add POSTed items to the new biblio too (1.94 KB, patch)
2012-04-13 16:16 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 7729 : svc/new_bib: support ?items=1 to add POSTed items to the new biblio too (1.96 KB, patch)
2012-07-10 08:09 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description MJ Ray (software.coop) 2012-03-16 18:51:01 UTC
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.
Comment 1 MJ Ray (software.coop) 2012-03-22 16:36:55 UTC
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.
Comment 2 MJ Ray (software.coop) 2012-03-22 16:42:42 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2012-03-22 20:39:03 UTC
Patch applies cleanly, and now works with the new architecture where Items are separate to Biblio data ans should be handled as such.
Comment 4 Chris Cormack 2012-03-22 20:45:15 UTC
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
Comment 5 Chris Cormack 2012-03-22 20:48:09 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2012-03-23 09:26:58 UTC
QA Comments: 
perlcritic OK (scv/bib)

Test on an UNIMARC installation, the patch add a node 995 per item.

Passed QA
Comment 7 Paul Poulain 2012-03-26 08:49:48 UTC
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);
         }
?
Comment 8 Paul Poulain 2012-03-28 13:50:56 UTC
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)
Comment 9 MJ Ray (software.coop) 2012-03-29 09:07:38 UTC
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.
Comment 10 Paul Poulain 2012-03-29 14:53:05 UTC
(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
Comment 11 MJ Ray (software.coop) 2012-04-13 16:16:26 UTC Comment hidden (obsolete)
Comment 12 MJ Ray (software.coop) 2012-04-13 16:17:22 UTC
follow-up patch posted to extend new_bib in a similar way, adding support for ?items=1
Comment 13 Paul Poulain 2012-04-20 10:18:19 UTC
(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
Comment 14 Chris Cormack 2012-07-10 08:09:22 UTC
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>
Comment 15 Mason James 2012-07-17 06:17:34 UTC
(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
Comment 16 Paul Poulain 2012-07-17 16:21:52 UTC
Maybe it's worth backporting to 3.8