https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- *** Bug 26674 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27857 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27857 [Bug 27857] Koha::Patron->extended_attributes skips checks -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118508 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118508&action=edit Bug 23666: Unit tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118510 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118510&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- URL| |https://wiki.koha-community | |.org/wiki/Patrons_extended_ | |attributes_endpoint_RFC Text to go in the| |This development adds release notes| |routes for managing a | |patron's extended | |attributes. | | | |The following | |routes are added: | |GET | |/patrons/:patron_id/extende | |d_attributes | |POST | |/patrons/:patron_id/extende | |d_attributes | |PUT | |/patrons/:patron_id/extende | |d_attributes | |DELETE | |/patrons/:patron_id/extende | |d_attributes/:extended_attr | |ibute_id -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118508|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118699&action=edit Bug 23666: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118510|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118701&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #20 from 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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|martin.renvoize@ptfs-europe |joonas.kylmala@helsinki.fi |.com | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118818 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118818&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|This development adds |This development adds release notes|routes for managing a |routes for managing a |patron's extended |patron's extended |attributes. |attributes. | | |The following |The following |routes are added: |routes are added: |GET |GET |/patrons/:patron_id/extende |/patrons/:patron_id/extende |d_attributes |d_attributes |POST |POST |/patrons/:patron_id/extende |/patrons/:patron_id/extende |d_attributes |d_attributes |PUT |PUT |/patrons/:patron_id/extende |/patrons/:patron_id/extende |d_attributes |d_attributes |DELETE |DELETE |/patrons/:patron_id/extende |/patrons/:patron_id/extende |d_attributes/:extended_attr |d_attributes/:extended_attr |ibute_id |ibute_id | |PATCH | |/patrons/:patron_id/extende | |d_attributes/:extended_attr | |ibute_id -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice one.. works great. Signing off the last patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #26 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119389 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119389&action=edit Bug 23666: Add spec Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #31 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119391&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #33 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119393&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|28161 | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28161 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28161 [Bug 28161] Allow Koha::Patron->extended_attributes be passed a library_id -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ephetteplace@cca.edu -- You are receiving this mail because: You are watching all bug changes.