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).

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.

*** Bug 26674 has been marked as a duplicate of this bug. ***

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.

Created attachment 118508 [details] [review]
Bug 23666: Unit tests

Created attachment 118509 [details] [review]
Bug 23666: Add spec

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 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 ?

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.

(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.

(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.

(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.

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?

(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.

Created attachment 118550 [details] [review]
Bug 23666: (QA follow-up) Fix wrong required permissions

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

Created attachment 118699 [details] [review]
Bug 23666: Unit tests

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

Created attachment 118700 [details] [review]
Bug 23666: Add spec

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

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>

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>

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!

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.

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>

(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!

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>

Nice one.. works great.  Signing off the last patch

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?

Created attachment 119042 [details] [review]
Bug 23666: (follow-up) Adjust to new exceptions

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

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>

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>

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>

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>

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>

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>

Pushed to master for 21.05, thanks to everybody involved!

Enhancement not pushed to 20.11.x