Bug 13738 - Add RESTful Borrower service
Summary: Add RESTful Borrower service
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 12272 13019
Blocks: 13737
  Show dependency treegraph
 
Reported: 2015-02-20 13:28 UTC by Kyle M Hall
Modified: 2020-11-30 21:44 UTC (History)
6 users (show)

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


Attachments
[WIP] Bug 13738 - Add RESTful Borrower service (6.94 KB, patch)
2015-02-20 13:37 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 Kyle M Hall 2015-02-20 13:28:49 UTC
We should have a basic RESTful CRUD service for Koha patrons.
Comment 1 Kyle M Hall 2015-02-20 13:37:43 UTC
Created attachment 36081 [details] [review]
[WIP] Bug 13738 - Add RESTful Borrower service
Comment 2 Kyle M Hall 2015-02-20 13:43:52 UTC
My first inclination is to remove the ability to fetch a borrower by multiple id types. Instead, I think we should force it to be my borrowernumber only, and if you don't have the borrowernumber, you can find it by searching ( not yet implemented ) on userid or cardnumber.

Any thoughts?
Comment 3 Martin Renvoize 2015-02-23 09:12:55 UTC
Comment on attachment 36081 [details] [review]
[WIP] Bug 13738 - Add RESTful Borrower service

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

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 :).
Comment 4 Martin Renvoize 2015-02-23 09:19:15 UTC
Remind me, which bug adds Koha::Borrower and Koha::Borrowers.. it needs to be a dependancy ;)
Comment 5 Katrin Fischer 2015-02-23 10:02:43 UTC
About the delete - this will probably create a problem with lists if we don't use the internal functions. We do a bit more there and not all is caught by FK constraints in the database (actually there are some bugs for stuff that we should catch, but don't).
Comment 6 Jonathan Druart 2015-02-23 12:14:16 UTC
(In reply to Martin Renvoize from comment #4)
> Remind me, which bug adds Koha::Borrower and Koha::Borrowers.. it needs to
> be a dependancy ;)

It's bug 13019
Comment 7 Martin Renvoize 2015-02-23 13:28:07 UTC
(In reply to Katrin Fischer from comment #5)
> About the delete - this will probably create a problem with lists if we
> don't use the internal functions. We do a bit more there and not all is
> caught by FK constraints in the database (actually there are some bugs for
> stuff that we should catch, but don't).

That's sort of the point of my comment cait..  I'd like to see the Objects moving towards being a single piont fo truth.. so i'd rather see the logic around deleting, creating, updateing and even reading a borrower moved from C4::Members into Koha::Borrowers... whether this starts as the Koha::Borrower object referencing the C4::Members routine, or writing it properly I don't really mind.. but in new code I'd certainly like to strive for 'doing it right' the first time and using only the Koha::* stuff in the Koha::Services unless absolutely nesessary.

Just my thoughts
Comment 8 Katrin Fischer 2015-02-23 14:00:12 UTC
Sounds all good to me :)

Just wanted to point out that actually we *must* do it differently, because else it will be buggy. Also we should have one central place to check for fines, checkouts, existing guarantor etc. - have to get that right.
Comment 9 Katrin Fischer 2020-01-06 14:57:24 UTC
I believe this has been resolved with the patrons routes?