Summary: | Add RESTful Borrower service | ||
---|---|---|---|
Product: | Koha | Reporter: | Kyle M Hall (khall) <kyle> |
Component: | Architecture, internals, and plumbing | Assignee: | Kyle M Hall (khall) <kyle> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | benjamin.rokseth, dcook, ere.maijala, jonathan.druart, martin.renvoize |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13607 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 12272, 13019 | ||
Bug Blocks: | 13737 | ||
Attachments: | [WIP] Bug 13738 - Add RESTful Borrower service |
Description
Kyle M Hall (khall)
2015-02-20 13:28:49 UTC
Created attachment 36081 [details] [review] [WIP] Bug 13738 - Add RESTful Borrower service 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 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 :). Remind me, which bug adds Koha::Borrower and Koha::Borrowers.. it needs to be a dependancy ;) 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). (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 (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 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. I believe this has been resolved with the patrons routes? |