Bug 23666 - Add routes for extended patron attributes
Summary: Add routes for extended patron attributes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Joonas Kylmälä
URL: https://wiki.koha-community.org/wiki/...
Keywords:
: 26674 (view as bug list)
Depends on: 20443 27857 28031
Blocks: 28002
  Show dependency treegraph
 
Reported: 2019-09-22 22:22 UTC by Stefan Kugler
Modified: 2021-12-13 21:12 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adds routes for managing a patron's extended attributes. The following routes are added: GET /patrons/:patron_id/extended_attributes POST /patrons/:patron_id/extended_attributes PUT /patrons/:patron_id/extended_attributes DELETE /patrons/:patron_id/extended_attributes/:extended_attribute_id PATCH /patrons/:patron_id/extended_attributes/:extended_attribute_id
Version(s) released in:
21.05.00


Attachments
Bug 23666: Unit tests (14.80 KB, patch)
2021-03-19 11:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23666: Add spec (10.26 KB, patch)
2021-03-19 11:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23666: Add extended attributes routes (8.29 KB, patch)
2021-03-19 11:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23666: (QA follow-up) Fix wrong required permissions (2.06 KB, patch)
2021-03-19 15:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23666: Unit tests (14.87 KB, patch)
2021-03-23 17:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23666: Add spec (10.32 KB, patch)
2021-03-23 17:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23666: Add extended attributes routes (8.35 KB, patch)
2021-03-23 17:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23666: (QA follow-up) Fix wrong required permissions (2.12 KB, patch)
2021-03-23 17:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23666: Add PATCH /patron/:patron_id/extended_attributes/:extended_attribute_id (10.75 KB, patch)
2021-03-25 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23666: Add PATCH /patron/:patron_id/extended_attributes/:extended_attribute_id (10.81 KB, patch)
2021-03-26 12:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23666: (follow-up) Adjust to new exceptions (2.16 KB, patch)
2021-03-31 12:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23666: Unit tests (14.94 KB, patch)
2021-04-09 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23666: Add spec (10.39 KB, patch)
2021-04-09 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23666: Add extended attributes routes (8.42 KB, patch)
2021-04-09 15:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23666: (QA follow-up) Fix wrong required permissions (2.19 KB, patch)
2021-04-09 15:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23666: Add PATCH /patron/:patron_id/extended_attributes/:extended_attribute_id (10.88 KB, patch)
2021-04-09 15:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23666: (follow-up) Adjust to new exceptions (2.23 KB, patch)
2021-04-09 15:18 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 Stefan Kugler 2019-09-22 22:22:31 UTC
Add API routes to manage additional patron attributes.

A simple implementation could only allow a modification of all attributes en bloc, eg:

GET       /patrons/{patron_id}/attributes           Get all additional attributes
POST/PUT? /patrons/{patron_id}/attributes           Create/overwrite additional attributes
DELETE    /patrons/{patron_id}/attributes           Delete all additional attributes

A more granular implementation could be eg:

GET      /patrons/{patron_id}/attributes/{code}    Get additional attribute "code"
PUT      /patrons/{patron_id}/attributes/{code}    Overwrite additional attribute "code"
DELETE   /patrons/{patron_id}/attributes/{code}    Delete additional attribute "code"

To be discussed: How to handle repeatable attributes with multiple values.
Comment 1 Tomás Cohen Arazi 2019-09-23 13:54:39 UTC
Hi, Stefan. I think you should add this proposal for discussion and voting here:

https://wiki.koha-community.org/wiki/REST_api_RFCs

One thing to note is that attributes might be repeatable, so using POST AND PUT is valid (POST for adding, PUT for replacing).
Comment 2 Martin Renvoize 2020-10-21 09:53:02 UTC
Hmmm, 

I'm not sure I agree with the proposal here... patron attributes can be defined with various restrictions and requirements directly related to patrons.  As such, I think the attributes themselves could/should be seamlessly embedded into the patron endpoints and an attributes endpoint should be tasked at managing the attribute categories and requirement themselves.
Comment 3 Martin Renvoize 2021-03-03 15:34:30 UTC
*** Bug 26674 has been marked as a duplicate of this bug. ***
Comment 4 Tomás Cohen Arazi 2021-03-03 15:50:29 UTC
I'm submitting an RFC proposal, to sumarize:

GET    /patrons/:patron_id/extended_attributes
POST   /patrons/:patron_id/extended_attributes
PUT    /patrons/:patron_id/extended_attributes <- to overwrite them all
DELETE /patrons/:patron_id/extended_attributes/:extended_attribute_id

Extended attributes could be queried by 'extended_attribute_type' to address the original 'more granular' approach.

I would add that the POST /patrons route needs to be extended to allow passing an 'extended_attributes' attribute, and that if there are some 'mandatory' extended attributes missing, patron creation should fail.

GET /patrons/:patron_id could be extended to be able to 'embed' the extended attributes as well.

The last two proposals should belong to their own bugs. I will file them once I have everything settled.

There's also the need to implement:

GET    /extended_attribute_types
GET    /extended_attribute_types/:extended_attribute_type_id
POST   /extended_attribute_types
PUT    /extended_attribute_types/:extended_attribute_type_id
DELETE /extended_attribute_types/:extended_attribute_type_id

And as types can be constrained by branch/library and patron category, we could think of a route for fetching types so we can use it to render forms. I still didn't find a suitable layout, will file a bug once I clear my mind, unless someone beats me to it.
Comment 5 Tomás Cohen Arazi 2021-03-19 11:11:10 UTC
Created attachment 118508 [details] [review]
Bug 23666: Unit tests
Comment 6 Tomás Cohen Arazi 2021-03-19 11:11:13 UTC
Created attachment 118509 [details] [review]
Bug 23666: Add spec
Comment 7 Tomás Cohen Arazi 2021-03-19 11:11:16 UTC
Created attachment 118510 [details] [review]
Bug 23666: Add extended attributes routes

This patch adds routes to handle patron's extended attributes:

GET    /patrons/:patron_id/extended_attributes
POST   /patrons/:patron_id/extended_attributes
PUT    /patrons/:patron_id/extended_attributes <- to overwrite them all
DELETE /patrons/:patron_id/extended_attributes/:extended_attribute_id

Controllers rely on the Koha::Patron methods for retrieving/adding
extended attributes. Exceptions are correctly catch and handled in the
responses. This is all covered by tests as well.

Note: I decidedto override the default sorting on the PUT response,
    because the overloaded search sorting it felt a bit wrong on the API.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
=> SUCCESS: Tests pass!
3. Play with your favourite tool and this routes
=> SUCCESS: They work well!
4. Sign off :-D
Comment 8 Joonas Kylmälä 2021-03-19 13:19:22 UTC
Comment on attachment 118509 [details] [review]
Bug 23666: Add spec

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

::: api/v1/swagger/paths/patrons_extended_attributes.json
@@ +78,5 @@
> +        }
> +      },
> +      "x-koha-authorization": {
> +        "permissions": {
> +          "catalogue": "1"

Shouldn't this be borrowers ?
Comment 9 Joonas Kylmälä 2021-03-19 13:30:13 UTC
I noticed there is no update() function for patron attribute. This would be useful in case there is mandatory attributes defined for patrons. Now having to use the overwrite method adds additional processing for the client a) they need to download all attributes and then b) upload all of them modified. I think it is not a blocker however, just a note that we should implement it in future.
Comment 10 Joonas Kylmälä 2021-03-19 13:41:03 UTC
(In reply to Joonas Kylmälä from comment #9)
> I noticed there is no update() function for patron attribute. This would be
> useful in case there is mandatory attributes defined for patrons. Now having
> to use the overwrite method adds additional processing for the client a)
> they need to download all attributes and then b) upload all of them
> modified. I think it is not a blocker however, just a note that we should
> implement it in future.

Actually, we cannot user overwrite() even for changing those attributes that are mandatory because the code deletes first all the attributes and then re-adds them! This also showed that we have no exception thrown when trying to delete mandatory attribute.
Comment 11 Tomás Cohen Arazi 2021-03-19 13:55:18 UTC
(In reply to Joonas Kylmälä from comment #8)
> Comment on attachment 118509 [details] [review] [review]
> Bug 23666: Add spec
> 
> Review of attachment 118509 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: api/v1/swagger/paths/patrons_extended_attributes.json
> @@ +78,5 @@
> > +        }
> > +      },
> > +      "x-koha-authorization": {
> > +        "permissions": {
> > +          "catalogue": "1"
> 
> Shouldn't this be borrowers ?

I don't remember now why I picked just staff access (catalogue). You are probably right. I think I found a place in which is was possible to see the attributes without much permissions.
Comment 12 Tomás Cohen Arazi 2021-03-19 13:58:50 UTC
(In reply to Joonas Kylmälä from comment #10)
> (In reply to Joonas Kylmälä from comment #9)
> > I noticed there is no update() function for patron attribute. This would be
> > useful in case there is mandatory attributes defined for patrons. Now having
> > to use the overwrite method adds additional processing for the client a)
> > they need to download all attributes and then b) upload all of them
> > modified. I think it is not a blocker however, just a note that we should
> > implement it in future.
> 
> Actually, we cannot user overwrite() even for changing those attributes that
> are mandatory because the code deletes first all the attributes and then
> re-adds them! This also showed that we have no exception thrown when trying
> to delete mandatory attribute.

Good point. But worth its own bug. This is mostly a controller method calling underlying implementations. Maybe this overwrite all route is not worth in the end, as it is replicating some current behavior (in the forms, passing all the attributes back and forth). The solution is to expose a PUT method.

I will add a PUT for individual attributes.
Comment 13 Joonas Kylmälä 2021-03-19 14:07:55 UTC
The patch set didn't apply so I had a bit hard time understanding what was going on. But now even more reading of the code and the patches from bug 27857 which this depends on I see that the deletion of attributes and adding of the new ones is all in one transaction in the Koha::Patron::extended_attributes() function so actually it works all just fine because throwing the exception would trigger the rollback. Sorry for the noise.

> I think I found a place in which is was possible to see the attributes without much permissions.

Please let us know if there is such a place. I couldn't find and it would be super scary if that was the case because these contain patron specific info which should be private (ie. borrowers permission required)!

Tomás, I'm moving this to assigned?
Comment 14 Tomás Cohen Arazi 2021-03-19 14:34:34 UTC
(In reply to Joonas Kylmälä from comment #13)
> The patch set didn't apply so I had a bit hard time understanding what was
> going on. But now even more reading of the code and the patches from bug
> 27857 which this depends on I see that the deletion of attributes and adding
> of the new ones is all in one transaction in the
> Koha::Patron::extended_attributes() function so actually it works all just
> fine because throwing the exception would trigger the rollback. Sorry for
> the noise.

27833
27858
27857
23666

^^ that's what I have on my branch, in that order.

> > I think I found a place in which is was possible to see the attributes without much permissions.
> 
> Please let us know if there is such a place. I couldn't find and it would be
> super scary if that was the case because these contain patron specific info
> which should be private (ie. borrowers permission required)!

Yeah, I cannot find that. I thought it was moremember.pl but it isn't.

> Tomás, I'm moving this to assigned?

Yes, I will adjust those permissions.
Comment 15 Tomás Cohen Arazi 2021-03-19 15:05:42 UTC
Created attachment 118550 [details] [review]
Bug 23666: (QA follow-up) Fix wrong required permissions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Martin Renvoize 2021-03-23 17:09:36 UTC
Created attachment 118699 [details] [review]
Bug 23666: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-03-23 17:09:39 UTC
Created attachment 118700 [details] [review]
Bug 23666: Add spec

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-03-23 17:09:43 UTC
Created attachment 118701 [details] [review]
Bug 23666: Add extended attributes routes

This patch adds routes to handle patron's extended attributes:

GET    /patrons/:patron_id/extended_attributes
POST   /patrons/:patron_id/extended_attributes
PUT    /patrons/:patron_id/extended_attributes <- to overwrite them all
DELETE /patrons/:patron_id/extended_attributes/:extended_attribute_id

Controllers rely on the Koha::Patron methods for retrieving/adding
extended attributes. Exceptions are correctly catch and handled in the
responses. This is all covered by tests as well.

Note: I decidedto override the default sorting on the PUT response,
    because the overloaded search sorting it felt a bit wrong on the API.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
=> SUCCESS: Tests pass!
3. Play with your favourite tool and this routes
=> SUCCESS: They work well!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-03-23 17:09:46 UTC
Created attachment 118702 [details] [review]
Bug 23666: (QA follow-up) Fix wrong required permissions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-03-23 17:10:26 UTC
This tree is a great step forward for the API's, nice work all involved!

Testing the API's and all works as expected for me.. Signing off!
Comment 21 Martin Renvoize 2021-03-23 17:24:21 UTC
I could certainly picture

PUT    /patrons/:patron_id/extended_attributes/:extended_attribute_id

as a future addition to this.. but I think for the existing code this is probably OK.
Comment 22 Tomás Cohen Arazi 2021-03-25 14:53:16 UTC
Created attachment 118818 [details] [review]
Bug 23666: Add PATCH /patron/:patron_id/extended_attributes/:extended_attribute_id

This patch adds the described route. It is designed to use the
underlying libraries' methods to update an existing attribute.

The tests cover the use cases.

Note: I added handling for two exceptions that can only occur on bad
data (i.e. not by using our codebase). This are:
- Koha::Exceptions::Patron::Attribute::InvalidType
- Koha::Exceptions::Patron::Attribute::NonRepeatable

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
=> SUCCESS: Tests pass!
3. PLay with the route
=> SUCCESS: Expected behavior!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2021-03-25 14:57:00 UTC
(In reply to Martin Renvoize from comment #21)
> I could certainly picture
> 
> PUT    /patrons/:patron_id/extended_attributes/:extended_attribute_id
> 
> as a future addition to this.. but I think for the existing code this is
> probably OK.

I decided to go with PATCH after our chat. And I posted it here. If possible, please re-test!
Comment 24 Martin Renvoize 2021-03-26 12:23:15 UTC
Created attachment 118854 [details] [review]
Bug 23666: Add PATCH /patron/:patron_id/extended_attributes/:extended_attribute_id

This patch adds the described route. It is designed to use the
underlying libraries' methods to update an existing attribute.

The tests cover the use cases.

Note: I added handling for two exceptions that can only occur on bad
data (i.e. not by using our codebase). This are:
- Koha::Exceptions::Patron::Attribute::InvalidType
- Koha::Exceptions::Patron::Attribute::NonRepeatable

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
=> SUCCESS: Tests pass!
3. PLay with the route
=> SUCCESS: Expected behavior!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2021-03-26 12:23:41 UTC
Nice one.. works great.  Signing off the last patch
Comment 26 Joonas Kylmälä 2021-03-30 06:54:33 UTC
There is a security bug I'm waiting feedback from other QA team members and it might block this one, I'm not sure. Martin, could you take a look at it and tell whether we should add it as a blocker for this one?
Comment 27 Tomás Cohen Arazi 2021-03-31 12:01:04 UTC
Created attachment 119042 [details] [review]
Bug 23666: (follow-up) Adjust to new exceptions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Kyle M Hall 2021-04-09 15:17:45 UTC
Created attachment 119388 [details] [review]
Bug 23666: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2021-04-09 15:17:57 UTC
Created attachment 119389 [details] [review]
Bug 23666: Add spec

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2021-04-09 15:18:00 UTC
Created attachment 119390 [details] [review]
Bug 23666: Add extended attributes routes

This patch adds routes to handle patron's extended attributes:

GET    /patrons/:patron_id/extended_attributes
POST   /patrons/:patron_id/extended_attributes
PUT    /patrons/:patron_id/extended_attributes <- to overwrite them all
DELETE /patrons/:patron_id/extended_attributes/:extended_attribute_id

Controllers rely on the Koha::Patron methods for retrieving/adding
extended attributes. Exceptions are correctly catch and handled in the
responses. This is all covered by tests as well.

Note: I decidedto override the default sorting on the PUT response,
    because the overloaded search sorting it felt a bit wrong on the API.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
=> SUCCESS: Tests pass!
3. Play with your favourite tool and this routes
=> SUCCESS: They work well!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2021-04-09 15:18:04 UTC
Created attachment 119391 [details] [review]
Bug 23666: (QA follow-up) Fix wrong required permissions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2021-04-09 15:18:08 UTC
Created attachment 119392 [details] [review]
Bug 23666: Add PATCH /patron/:patron_id/extended_attributes/:extended_attribute_id

This patch adds the described route. It is designed to use the
underlying libraries' methods to update an existing attribute.

The tests cover the use cases.

Note: I added handling for two exceptions that can only occur on bad
data (i.e. not by using our codebase). This are:
- Koha::Exceptions::Patron::Attribute::InvalidType
- Koha::Exceptions::Patron::Attribute::NonRepeatable

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_extended_attributes.t
=> SUCCESS: Tests pass!
3. PLay with the route
=> SUCCESS: Expected behavior!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2021-04-09 15:18:12 UTC
Created attachment 119393 [details] [review]
Bug 23666: (follow-up) Adjust to new exceptions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jonathan Druart 2021-04-22 12:39:32 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 35 Fridolin Somers 2021-04-29 14:23:40 UTC
Enhancement not pushed to 20.11.x