Bug 13607 - Patron management API
Summary: Patron management API
Status: RESOLVED DUPLICATE of bug 16330
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Robin Sheat
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-22 01:00 UTC by Robin Sheat
Modified: 2018-10-14 09:27 UTC (History)
14 users (show)

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


Attachments
Bug 13607: patron management API (51.00 KB, patch)
2015-01-22 01:32 UTC, Robin Sheat
Details | Diff | Splinter Review
Simple script to test the new API calls (948 bytes, application/x-perl)
2015-02-03 13:57 UTC, Magnus Enger
Details
Bug 13607: fix to account for _ in fieldnames (1.16 KB, patch)
2015-02-08 23:21 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 13607: fix to set default messaging preferences (970 bytes, patch)
2015-02-25 04:09 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 13607: correct handling of dateexpiry and dateenrolled (3.86 KB, patch)
2015-03-03 04:32 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 13607: test cases fixes (1.00 KB, patch)
2015-03-30 03:52 UTC, Robin Sheat
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2015-01-22 01:00:56 UTC
Koha needs an API that allows creation, updating, and deleting of patrons.
Comment 1 Robin Sheat 2015-01-22 01:32:23 UTC
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.)
Comment 2 Robin Sheat 2015-01-22 01:52:52 UTC
Documentation of the API is currently here:

http://wiki.koha-community.org/wiki/Talk:Koha_/svc/_HTTP_API#Patron_Management_API
Comment 3 Jonathan Druart 2015-01-22 08:19:56 UTC
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
Comment 4 Martin Renvoize 2015-01-22 09:02:29 UTC
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..
Comment 5 Martin Renvoize 2015-01-22 09:05:56 UTC
Also... have you seen Jesse's work on Koha::Service?

https://github.com/pianohacker/koha/commit/40908d6e58c40e6ffaa488266a70ef5233fe9642
Comment 6 Robin Sheat 2015-01-22 11:38:42 UTC
(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.
Comment 7 Martin Renvoize 2015-01-27 11:00:13 UTC
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.
Comment 8 Robin Sheat 2015-01-27 22:38:25 UTC
(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.
Comment 9 Magnus Enger 2015-02-03 13:57:02 UTC
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.
Comment 10 Magnus Enger 2015-02-03 14:05:55 UTC
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.
Comment 11 Magnus Enger 2015-02-05 13:59:48 UTC
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.
Comment 12 Robin Sheat 2015-02-08 23:21:23 UTC
Created attachment 35737 [details] [review]
Bug 13607: fix to account for _ in fieldnames
Comment 13 Robin Sheat 2015-02-08 23:22:55 UTC
(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.
Comment 14 Magnus Enger 2015-02-13 08:11:07 UTC
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.
Comment 15 Robin Sheat 2015-02-25 04:09:48 UTC
Created attachment 36148 [details] [review]
Bug 13607: fix to set default messaging preferences
Comment 16 Robin Sheat 2015-02-25 04:10:56 UTC
(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.
Comment 17 Robin Sheat 2015-03-03 04:32:49 UTC
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.
Comment 18 Mirko Tietgen 2015-03-11 18:45:31 UTC
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).
Comment 19 Robin Sheat 2015-03-12 00:31:48 UTC
(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.
Comment 20 Robin Sheat 2015-03-30 03:52:56 UTC
Created attachment 37353 [details] [review]
Bug 13607: test cases fixes
Comment 21 Mirko Tietgen 2016-01-28 13:08:19 UTC
I assume Robin is not going to work on this any longer?
Comment 22 David Cook 2016-02-01 02:40:30 UTC
(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).
Comment 23 Benjamin Rokseth 2016-04-25 08:27:04 UTC
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
Comment 24 Katrin Fischer 2018-10-14 09:27:15 UTC
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 ***