Koha needs an API that allows creation, updating, and deleting of patrons.
Created attachment 35455 [details] [review] Bug 13607: patron management API This provides a patron management API. It allows a remote service to: * add patrons * update patron details * delete patrons It presents a simple POST-based API and returns its results as XML. This API supports fields available in the borrower table, and extended attribute types also. Full documentation will be on a wiki page linked to from the bug. To test: * construct a sequence of API requests, make sure it works. * ensure that no side effects of these changes came about, particularly dealing with unrelated things (default_messageprefs.pl, sco-patron-image.pl, systempreferences.)
Documentation of the API is currently here: http://wiki.koha-community.org/wiki/Talk:Koha_/svc/_HTTP_API#Patron_Management_API
Hi Robin, Did you have a look at koha-restful? It seems it does the same job as this patch. https://git.biblibre.com/biblibre/koha-restful
Hi Robin, A much needed feature.. but I'm not 100% I agree with the implementation. I think we should really be attempting to follow restful best practice. Personally, I would: Use POST for the create operation only Use PUT for the update operation (possibly with your current implementation in here 'update or create' use DELETE for the delete operation only use GET for retrieving patron records use json (or xml) bodies to actually move the data around.. The above would bring us much more in line with the rest of the modern world. Parsing XML is hard in the browser, and such an API should be designed for maximum flexibility, i.e to be simply used in as many environments as possible.. including the browser. Anywho.. If I get the time, I might submit an alternative patch.. if not I'll go ahead and test this..
Also... have you seen Jesse's work on Koha::Service? https://github.com/pianohacker/koha/commit/40908d6e58c40e6ffaa488266a70ef5233fe9642
(In reply to Jonathan Druart from comment #3) > Hi Robin, > Did you have a look at koha-restful? > It seems it does the same job as this patch. > > https://git.biblibre.com/biblibre/koha-restful I didn't because I didn't see it in koha, so I didn't really know it existed or what it could do. (In reply to Martin Renvoize from comment #4) > Use POST for the create operation only > Use PUT for the update operation (possibly with your current implementation > in here 'update or create' You don't always know if you're creating or updating from a membership management system thing. Hence upsert. Which is a word, look in wikipedia :) > use DELETE for the delete operation only > use GET for retrieving patron records > use json (or xml) bodies to actually move the data around.. Some of these verbs are (I think, though I haven't checked) harder to use from a browser. > The above would bring us much more in line with the rest of the modern > world. Parsing XML is hard in the browser, and such an API should be > designed for maximum flexibility, i.e to be simply used in as many > environments as possible.. including the browser. a) if you use things like DELETE and PUT, IIRC they're hard to use from a browser (I might be wrong, that used the be the case though), b) with the changes I added to C4::Service you can add switching to JSON easily ... but then you still need XML to authenticate. It's silly to mix technologies like that (even though they are already mixed, at least you can do user management using all one thing), c) the intention of this isn't really to work from a browser, but to add integration to things like salesforce that manage users. (In reply to Martin Renvoize from comment #5) > Also... have you seen Jesse's work on Koha::Service? > > https://github.com/pianohacker/koha/commit/ > 40908d6e58c40e6ffaa488266a70ef5233fe9642 I hadn't, again, it's not in Koha so I didn't know it existed. At a (very quick) glance, it looks to be a definite improvement over the original C4::Service, but as it is, too much stuff is being crammed into C4::Service. My first impression of that is that it needs to be split up.
I wasn't disputing upsert as a word.. more that in my experience it's the PUT method that used for it: http://stackoverflow.com/questions/7297578/correct-http-method-for-restful-create-or-update gives a farily concise reasoning. As for the lack of browser support.. I can now having googled it see where your coming from on this.. It's not the browsers so much as the html versions. Pretty much all browser will support the requests via javascript, but it would seem the HTML 4.01 and XHTML 1.0 both only support GET and POST as Form submission methods. Hmm, this one does then need some thought applying. Your other points are all valid.. What I'm thinking here, as you've already put the work in, is to sign off on this as it stands and let it progress into koha. However, I hope to deprecate the /svc namespace at some point and push for a nice concise set of restful routes (including a patrons route). Some thoughts regarding this discussion can be found on the recently created wiki page: http://wiki.koha-community.org/wiki/New_REST_API_RFC and we hope to get a bit of a group together at the next hackfest.
(In reply to Martin Renvoize from comment #7) > I wasn't disputing upsert as a word.. more that in my experience it's the > PUT method that used for it: Yeah, I'd misread what you said there. > As for the lack of browser support.. I can now having googled it see where > your coming from on this.. It's not the browsers so much as the html > versions. Pretty much all browser will support the requests via javascript, > but it would seem the HTML 4.01 and XHTML 1.0 both only support GET and POST > as Form submission methods. Hmm, this one does then need some thought > applying. Hmm, these probably aren't suitable for form submission anyway. It's not like they're providing HTML back. > What I'm thinking here, as you've already put the work in, is to sign off on > this as it stands and let it progress into koha. However, I hope to > deprecate the /svc namespace at some point and push for a nice concise set > of restful routes (including a patrons route). Yes, this would be good. I just had to conform to what was already in koha. Or really, pick one of the methods in koha and conform to that (which was an obvious choice: pick the one that gives me consistency with the authentication endpoint.) Some rationalisation of what's available would be good. Note that with my changes to C4::Service, it'd be quite easy to allow a 'format=json' type argument to change what you're getting. We could even add support for 'format=usmarc' if you want to use that as your formatting method. I'm sure someone somewhere likes that idea. They would be a crazy person.
Created attachment 35660 [details] Simple script to test the new API calls A simple script that makes 4 basic calls to the API, to authenticate, then create, update and delete a patron.
I did some basic exercising of the API calls with the attached script, and it seems to work as advertised. Yay! * Minor problem: - The POD (synopsis and description) in C4/Output/XMLStream.pm seems to mix up XML and JSON a bit. * Major problem: - EnhancedMessagingPreferences = allow - Create a new patron from scratch - Change the category from the default one - Firebug reports a 500 error for /cgi-bin/koha/members/default_messageprefs.pl The actual error reported: Software error: Can't use string ("C4::Service") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/C4/Service.pm line 262.
If I try to include one of the B_* fields in my borrower data, I get this : <response> <status>failed</status> <type>data</type> <message>Invalid fieldname provided: B_address </message> </response> Probably because of this line: die "Invalid fieldname provided (update): $f\n" if $f =~ /[^A-Za-z0-9]/; Also, the "message" in the response would look better without the trailing newline, I think.
Created attachment 35737 [details] [review] Bug 13607: fix to account for _ in fieldnames
(In reply to Magnus Enger from comment #11) > Also, the "message" in the response would look better without the trailing > newline, I think. I agree, but if you remove the \n then die adds "in file blah.pl line 123", which I think is even worse than a trailing newline.
This problem from comment 10 is still a problem: * Major problem: - EnhancedMessagingPreferences = allow - Create a new patron from scratch - Change the category from the default one - Firebug reports a 500 error for /cgi-bin/koha/members/default_messageprefs.pl The actual error reported: Software error: Can't use string ("C4::Service") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/C4/Service.pm line 262.
Created attachment 36148 [details] [review] Bug 13607: fix to set default messaging preferences
(In reply to Magnus Enger from comment #14) > - EnhancedMessagingPreferences = allow > - Create a new patron from scratch > - Change the category from the default one > - Firebug reports a 500 error for > /cgi-bin/koha/members/default_messageprefs.pl I did this with the new patch added, and it doesn't fail. With the previous version, I was failing to set the default preferences, and this apparently upsets something in Koha.
Created attachment 36356 [details] [review] Bug 13607: correct handling of dateexpiry and dateenrolled With this patch: * if a dateexpiry is not provided, it is generated based on the category. * if a dateenrolled is not provided, it is generated based on 'now'. This applies when a user is created only.
I have played with this a little and I like it. Works for me so far, will try to break it some more. I think userid (for account login) is automatically created from 'firstname'.'surname' if not explicitly given (in the staff client), I wonder if that should be the same here for consinstency (personally I don't mind).
(In reply to Mirko Tietgen from comment #18) > I have played with this a little and I like it. Works for me so far, will > try to break it some more. > > I think userid (for account login) is automatically created from > 'firstname'.'surname' if not explicitly given (in the staff client), I > wonder if that should be the same here for consinstency (personally I don't > mind). I kinda feel like for an API things should in general be made to be more explicit. But I'm not particularly hung up on the idea either way.
Created attachment 37353 [details] [review] Bug 13607: test cases fixes
I assume Robin is not going to work on this any longer?
(In reply to Mirko Tietgen from comment #21) > I assume Robin is not going to work on this any longer? As far as I know, Robin isn't working on Koha at all anymore, so that's probably a safe assumption. I'm sure Chris Cormack or Liz Rea would be able to confirm that though (or Robin if he's still lurking).
Hi, for the record, there is a new thread based on the new restful API that would hopefully replace this bug: Bug 16330 - REST API: add routes to add, update and delete patrons
I believe this has been fixed with the new REST API endpoint for patrons. *** This bug has been marked as a duplicate of bug 16330 ***