This is needed to implement self-registration in frontends softwares. Because POST to /api/v1/patrons need the category as a mandatory argument, we need a way to retrieve a list of the categories.
Created attachment 159714 [details] [review] Bug 26297: Preparation: Add dateofbirthrequired as boolean in Result/Schema
Created attachment 159715 [details] [review] Bug 26297: Add patron categories to_api_mapping
Created attachment 159716 [details] [review] Bug 26297: API specs Test plan, k-t-d: 1) Access /api/v1/patron_categories 2) Verify the patron categories are correctly listed
Missing tests.
Created attachment 159717 [details] [review] Bug 26297: API specs Test plan, k-t-d: 1) Access /api/v1/patron_categories 2) Verify the patron categories are correctly listed
Created attachment 159722 [details] [review] Bug 26297: Add tests
Created attachment 159921 [details] [review] Bug 26297: Preparation: Add dateofbirthrequired as boolean in Result/Schema Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 159922 [details] [review] Bug 26297: Add patron categories to_api_mapping Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 159923 [details] [review] Bug 26297: API specs Test plan, k-t-d: 1) Access /api/v1/patron_categories 2) Verify the patron categories are correctly listed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 159924 [details] [review] Bug 26297: Add tests Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Should + dateofbirthrequired => 'date_of_birth_required', not be undef as well? I don't believe this is used.
(In reply to Katrin Fischer from comment #11) > Should > > + dateofbirthrequired => 'date_of_birth_required', > > not be undef as well? I don't believe this is used. git grep dateofbirthrequired Shows quite a lot of hits, it certainly appears to be used. The SQL comment is "the minimum age required for the patron category" which I think is what this is used for, but the naming is not clear in that regard imo.
(In reply to Pedro Amorim from comment #12) > (In reply to Katrin Fischer from comment #11) > > Should > > > > + dateofbirthrequired => 'date_of_birth_required', > > > > not be undef as well? I don't believe this is used. > > git grep dateofbirthrequired > > Shows quite a lot of hits, it certainly appears to be used. > The SQL comment is "the minimum age required for the patron category" which > I think is what this is used for, but the naming is not clear in that regard > imo. Yes, naming is terrible. We should rename it to something sensible in the API. Does something like "Lower_age_limit" work in English? We should make it clear that those fields belong together.
Created attachment 159955 [details] [review] Bug 26297: (QA Follow-up); Rename dateofbirthrequired from date_of_birth_required to lower_age_limit in api_mapping
Katrin, thank you, I agree. A couple notes: 1) I have made the following patch obsolete: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159714 I was thrown off by the naming and tinyint(1) in kohastructure and thought this column was meant to be a boolean indicating whether date of birth was supposed to be required or not. This is not the case, as we've established. 2) My understanding is that we're reluctant to fix things in the database due to existing reports, but it doesn't feel great to have to_api_mapping be fixing stuff that should really be fixed at the root of the problem. This specific naming is originally from 17 years ago, it's outdated, doesn't make sense and it should be fixed directly in the database structure imo. I'm going off on an tangent but I believe this is relevant. On a different bug I submitted this patch: https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=35570&attachment=159944 That atomicupdate will search for SQL reports containing the obsolete naming and notify, on upgrade, if some reports need to be manually looked at (and which). I wonder if we could adopt something like this going forward, and become a bit more open to updating and fixing the database schema. Note that all of the above is discussion only and should not block this bug 26297 imo.
Hi Pedro, as we have such a super specific column name here... we could just update the reports using the column maybe? I am not opposed to fixing the database in this case, just wonder if want to maybe do it separately from this bug?
(In reply to Katrin Fischer from comment #16) > Hi Pedro, as we have such a super specific column name here... we could just > update the reports using the column maybe? I am not opposed to fixing the > database in this case, just wonder if want to maybe do it separately from > this bug? Hi Katrin, thank you. I've added bug 35679.
Comment on attachment 159955 [details] [review] Bug 26297: (QA Follow-up); Review of attachment 159955 [details] [review]: ----------------------------------------------------------------- ::: api/v1/swagger/definitions/patron_category.yaml @@ +35,2 @@ > type: > - boolean Is it really a boolean.. the comments suggest it should be an int?
Created attachment 160412 [details] [review] Bug 26297: Update API spec lower_age_limit is a number, not a boolean
(In reply to Martin Renvoize from comment #18) > Comment on attachment 159955 [details] [review] [review] > Bug 26297: (QA Follow-up); > > Review of attachment 159955 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: api/v1/swagger/definitions/patron_category.yaml > @@ +35,2 @@ > > type: > > - boolean > > Is it really a boolean.. the comments suggest it should be an int? Well spotted thanks! I completely missed this. It made sense and worked when I first wrote the patch turning it into boolean on schema files, but that was wrong ( and now obsolete) so this API spec needed updating.
1. Why Koha/REST/V1/PatronCategories.pm and not under Koha/REST/V1/Patrons? 2. I think you should return a filtered list using Koha::Patron::Categories->search_with_library_limits instead.
Created attachment 161450 [details] [review] Bug 26297: (QA follow-up) Move to REST::V1::Patrons::Categories
Created attachment 161451 [details] [review] Bug 26297: (QA follow-up) Use search_with_library_limits
(In reply to Jonathan Druart from comment #21) > 1. Why Koha/REST/V1/PatronCategories.pm and not under Koha/REST/V1/Patrons? > > 2. I think you should return a filtered list using > Koha::Patron::Categories->search_with_library_limits instead. 1. An oversight, makes much more sense under Koha/REST/V1/Patrons. Patch added. 2. Patch added. It seems there are no tests in Koha for search_with_library_limits or did I miss it? Opened bug 35910 for that.
(In reply to Pedro Amorim from comment #24) > 2. Patch added. It seems there are no tests in Koha for > search_with_library_limits or did I miss it? Opened bug 35910 for that. Scratch that, found them.
Should we really expose all those attributes to all staff with catalogue only permission? So far this is only accessible for staff with access to the admin module.
Created attachment 161484 [details] [review] Bug 26297: Add patron categories to_api_mapping Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 161485 [details] [review] Bug 26297: API specs Test plan, k-t-d: 1) Access /api/v1/patron_categories 2) Verify the patron categories are correctly listed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Bug 26297: (QA follow-up) Move to REST::V1::Patrons::Categories Bug 26297: (QA follow-up) Use search_with_library_limits JD amended-patch: squashed + tidy
Created attachment 161486 [details] [review] Bug 26297: Add tests Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 161487 [details] [review] Bug 26297: Rename date_of_birth_required to lower_age_limit in api_mapping
Created attachment 161488 [details] [review] Bug 26297: Update API spec lower_age_limit is a number, not a boolean
Created attachment 161496 [details] [review] Bug 26297: (QA follow-up): Update required permission /api/v1/patrons endpoint now requires parameters:manage_patron_categories instead of just 'catalogue'
(In reply to Jonathan Druart from comment #26) > Should we really expose all those attributes to all staff with catalogue > only permission? > > So far this is only accessible for staff with access to the admin module. Hi Jonathan, thanks! I've updated the required API permission to parameters:manage_patron_categories to match what's required by the original admin/categories.pl page. Are we able to put this back to SO? Thank you!
Hi, please let me know if there's something else required here!
(In reply to Jonathan Druart from comment #26) > Should we really expose all those attributes to all staff with catalogue > only permission? > > So far this is only accessible for staff with access to the admin module. Maybe we need to re-discuss the idea of having some sort of 'views' for resources, suitable for rendering dropdowns, etc. Full resource access is one thing, being about to choose one on a dropdown is another. Same as searching for patrons to display a dropdown, it doesn't require access to the full patron information.
I've reviewed this and have a few remarks. * I felt tempted to submit all this squashed (minor/tiny thing) * The tests shouldn't delete all data, this is bad. They should probably use `q=` to constraint the resultset instead. And if we are trying to test the `search_with_library_limits` nature of the implementation, then we need to properly build the tests by adding a couple libraries, several categories limited to one and the other, and then crafting a `q=` query that searches for them all, and we only get the right ones for the branch we are logged at. * I don't like it that it returns the categories limited by branch by default. That would work for rendering dropdowns, but not for a CRUD page for categories. And that use case is vital for implementing `GET /patron_categories`. * If we need a way to restrict the returned categories by branch, then we should add a switch for that. * The standard parameters we add to all `list()` endpoints are missing on the spec (i.e. no pagination, no q=, etc). * No `use Koha::Patron::Categories;` * We should use Koha::V1::Patron::Categories, double plural sounds wrong (i.e. Patrons).
* The change on the required permissions broke the tests
(In reply to Tomás Cohen Arazi from comment #36) > * We should use Koha::V1::Patron::Categories, double plural sounds wrong > (i.e. Patrons). We did this all the time, nevermind :-D
Created attachment 165390 [details] [review] Bug 26297: Add patron categories to_api_mapping Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 165391 [details] [review] Bug 26297: API specs Test plan, k-t-d: 1) Access /api/v1/patron_categories 2) Verify the patron categories are correctly listed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Bug 26297: (QA follow-up) Move to REST::V1::Patrons::Categories Bug 26297: (QA follow-up) Use search_with_library_limits JD amended-patch: squashed + tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 165392 [details] [review] Bug 26297: Add tests Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 165393 [details] [review] Bug 26297: (QA follow-up) Spec fixes This patch fixes some spec misses. To test: 1. Run: $ ktd --shell k$ prove xt/api.t => FAIL: Tests fail! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pedro, I think this is missing: * The tests shouldn't delete all data, this is bad. They should probably use `q=` to constraint the resultset instead. And if we are trying to test the `search_with_library_limits` nature of the implementation, then we need to properly build the tests by adding a couple libraries, several categories limited to one and the other, and then crafting a `q=` query that searches for them all, and we only get the right ones for the branch we are logged at. * I don't like it that it returns the categories limited by branch by default. That would work for rendering dropdowns, but not for a CRUD page for categories. And that use case is vital for implementing `GET /patron_categories`. * If we need a way to restrict the returned categories by branch, then we should add a switch for that. Regarding always using `search_with_library_limits`, I would be happy to stand corrected if you explain why it makes sense as-is. I added the missing parameters in a follow-up, and some minor squashed fixes, including the tests failing for permissions. The only missing bit would be redoing the tests. Happy to help if you want me to take on it.
Created attachment 165453 [details] [review] Bug 26297: (QA follow-up): Update tests prove t/db_dependent/api/v1/patron_categories.t
Created attachment 165655 [details] [review] Bug 26297: (QA follow-up): Update tests prove t/db_dependent/api/v1/patron_categories.t
*** Bug 34007 has been marked as a duplicate of this bug. ***
Pushed for 24.05! Well done everyone, thank you!
There's a merge error here leading to an invalid spec file.
Created attachment 165827 [details] [review] Bug 26297: (RM follow-up) Fix specification file We had a duplicate 'description' key introduced in the patron category specification file here which causes errors on bundling the specs. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Last patch attached needed on main Katrin
Follow-up will be pushed to main.
Pushed to 23.11.x for 23.11.06
Enhancement will not be backported to 23.05.x
API route change, nothing to add/edit in the Koha manual. I assume API documentation is done automatically?
(In reply to Tomás Cohen Arazi (tcohen) from comment #43) > Regarding always using `search_with_library_limits`, I would be happy to > stand corrected if you explain why it makes sense as-is. > > I added the missing parameters in a follow-up, and some minor squashed > fixes, including the tests failing for permissions. The only missing bit > would be redoing the tests. Happy to help if you want me to take on it. I need a route without this restriction (ie. return all categories), how do we do?
(In reply to Jonathan Druart from comment #56) > (In reply to Tomás Cohen Arazi (tcohen) from comment #43) > > Regarding always using `search_with_library_limits`, I would be happy to > > stand corrected if you explain why it makes sense as-is. > > > > I added the missing parameters in a follow-up, and some minor squashed > > fixes, including the tests failing for permissions. The only missing bit > > would be redoing the tests. Happy to help if you want me to take on it. > > I need a route without this restriction (ie. return all categories), how do > we do? Please file a bug with the specific use case you need to address and we discuss its implementation there. I have some ideas from earlier this year, for endpoints designed for rendering dropdowns/selectors.