[Koha-bugs] [Bug 13738] Add RESTful Borrower service

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13738

--- Comment #3 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
Comment on attachment 36081
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36081
[WIP] Bug 13738 - Add RESTful Borrower service

Review of attachment 36081:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=13738&attachment=36081)
-----------------------------------------------------------------

Generally I'm really happy with this, it's a great starting point for us to
expand upon. Nice work Kyle!

::: Koha/Service/Borrower.pm
@@ +39,5 @@
> +            authnotrequired => 0,
> +            needed_flags    => { borrowers => 1 },
> +            routes          => [
> +                [ qr'GET /(\d+)',    'get_borrower' ],
> +                [ qr'POST /(\d+)',   'add_borrower' ],

Untested, but I beleive the '/(\d+) on this line is superflous and may infact
mean the POST method is unreachable if my memory serves my correctly.

@@ +88,5 @@
> +=head2 POST
> +
> +    Syntax: /rest/borrower
> +
> +    Creates a new patron. Data must be supplied as a POST of JSON data.

JSON data, I'm loving JSON and fully support it's use.. however, for the
borrower service I tihnk we should attempt to support XML aswell via content
negotiation. I can see this route being used internally in Koha allot, and also
being used for institutions to manage their borrowers externally.. Many said
institutions are still stuck in the dark ages of XML ;)

@@ +104,5 @@
> +    my $data = from_json( $json, { utf8 => 1 } );
> +
> +    my $borrower = Koha::Borrower->new()->set($data)->store();
> +
> +    $self->output( $borrower->as_hashref() );

Great start, but we should probably end up with some error handling here so we
can return a meaningful json error body if the insert isn't allowed for some
reason or another.

@@ +153,5 @@
> +
> +    my $borrower = Koha::Borrowers->find( { get_id_field() => $id } );
> +
> +    if ($borrower) {
> +        $borrower->delete(); #TODO Use C4::Members to delete

Related to the #TODO: Personally, I think I'd rather see the delete
functionality from C4::Members rolled into the $borrower object, an eventually
for scripts to all strt using the object as the defactly point of truth for
this sort of thing. I would be nice to start the move into the Koha with a
clean sweep of these sorts of functions :).

-- 
You are receiving this mail because:
You are watching all bug changes.