Bug 26297 - Add a route to list patron categories
Summary: Add a route to list patron categories
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 34007 (view as bug list)
Depends on:
Blocks: 17505 35539 36453
  Show dependency treegraph
 
Reported: 2020-08-25 14:31 UTC by Arthur Suzuki
Modified: 2024-04-30 10:01 UTC (History)
7 users (show)

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


Attachments
Bug 26297: Preparation: Add dateofbirthrequired as boolean in Result/Schema (846 bytes, patch)
2023-12-11 17:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: Add patron categories to_api_mapping (2.52 KB, patch)
2023-12-11 17:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: API specs (8.09 KB, patch)
2023-12-11 17:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: API specs (8.00 KB, patch)
2023-12-11 17:35 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: Add tests (2.69 KB, patch)
2023-12-12 10:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: Preparation: Add dateofbirthrequired as boolean in Result/Schema (901 bytes, patch)
2023-12-15 22:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26297: Add patron categories to_api_mapping (2.57 KB, patch)
2023-12-15 22:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26297: API specs (8.06 KB, patch)
2023-12-15 22:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26297: Add tests (2.74 KB, patch)
2023-12-15 22:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26297: (QA Follow-up); (1.67 KB, patch)
2023-12-18 11:57 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: Update API spec (857 bytes, patch)
2024-01-02 17:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: (QA follow-up) Move to REST::V1::Patrons::Categories (1.54 KB, patch)
2024-01-25 17:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: (QA follow-up) Use search_with_library_limits (915 bytes, patch)
2024-01-25 17:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: Add patron categories to_api_mapping (2.57 KB, patch)
2024-01-26 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26297: API specs (8.25 KB, patch)
2024-01-26 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26297: Add tests (2.74 KB, patch)
2024-01-26 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26297: Rename date_of_birth_required to lower_age_limit in api_mapping (1.64 KB, patch)
2024-01-26 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26297: Update API spec (859 bytes, patch)
2024-01-26 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26297: (QA follow-up): Update required permission (877 bytes, patch)
2024-01-26 09:59 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: Add patron categories to_api_mapping (2.62 KB, patch)
2024-04-23 13:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26297: API specs (8.23 KB, patch)
2024-04-23 13:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26297: Add tests (2.80 KB, patch)
2024-04-23 13:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26297: (QA follow-up) Spec fixes (1.88 KB, patch)
2024-04-23 13:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26297: (QA follow-up): Update tests (3.44 KB, patch)
2024-04-24 11:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 26297: (QA follow-up): Update tests (3.44 KB, patch)
2024-04-26 12:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26297: (RM follow-up) Fix specification file (1.01 KB, patch)
2024-04-30 06:32 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2020-08-25 14:31:36 UTC
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.
Comment 1 Pedro Amorim 2023-12-11 17:32:45 UTC
Created attachment 159714 [details] [review]
Bug 26297: Preparation: Add dateofbirthrequired as boolean in Result/Schema
Comment 2 Pedro Amorim 2023-12-11 17:32:48 UTC
Created attachment 159715 [details] [review]
Bug 26297: Add patron categories to_api_mapping
Comment 3 Pedro Amorim 2023-12-11 17:32:50 UTC Comment hidden (obsolete)
Comment 4 Pedro Amorim 2023-12-11 17:33:12 UTC
Missing tests.
Comment 5 Pedro Amorim 2023-12-11 17:35:09 UTC
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
Comment 6 Pedro Amorim 2023-12-12 10:58:03 UTC
Created attachment 159722 [details] [review]
Bug 26297: Add tests
Comment 7 Lucas Gass 2023-12-15 22:06:33 UTC
Created attachment 159921 [details] [review]
Bug 26297: Preparation: Add dateofbirthrequired as boolean in Result/Schema

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 8 Lucas Gass 2023-12-15 22:06:35 UTC
Created attachment 159922 [details] [review]
Bug 26297: Add patron categories to_api_mapping

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 9 Lucas Gass 2023-12-15 22:06:38 UTC
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>
Comment 10 Lucas Gass 2023-12-15 22:06:40 UTC
Created attachment 159924 [details] [review]
Bug 26297: Add tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 11 Katrin Fischer 2023-12-16 13:34:41 UTC
Should 

+        dateofbirthrequired               => 'date_of_birth_required',

not be undef as well? I don't believe this is used.
Comment 12 Pedro Amorim 2023-12-18 09:24:58 UTC
(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.
Comment 13 Katrin Fischer 2023-12-18 10:40:02 UTC
(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.
Comment 14 Pedro Amorim 2023-12-18 11:57:54 UTC
Created attachment 159955 [details] [review]
Bug 26297: (QA Follow-up);

Rename dateofbirthrequired from date_of_birth_required to lower_age_limit in api_mapping
Comment 15 Pedro Amorim 2023-12-18 12:06:15 UTC
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.
Comment 16 Katrin Fischer 2023-12-23 12:31:41 UTC
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?
Comment 17 Pedro Amorim 2024-01-02 09:38:30 UTC
(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 18 Martin Renvoize 2024-01-02 13:25:55 UTC
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?
Comment 19 Pedro Amorim 2024-01-02 17:20:41 UTC
Created attachment 160412 [details] [review]
Bug 26297: Update API spec

lower_age_limit is a number, not a boolean
Comment 20 Pedro Amorim 2024-01-02 17:22:11 UTC
(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.
Comment 21 Jonathan Druart 2024-01-25 11:29:21 UTC
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.
Comment 22 Pedro Amorim 2024-01-25 17:00:20 UTC
Created attachment 161450 [details] [review]
Bug 26297: (QA follow-up) Move to REST::V1::Patrons::Categories
Comment 23 Pedro Amorim 2024-01-25 17:00:22 UTC
Created attachment 161451 [details] [review]
Bug 26297: (QA follow-up) Use search_with_library_limits
Comment 24 Pedro Amorim 2024-01-25 17:01:42 UTC
(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.
Comment 25 Pedro Amorim 2024-01-25 17:02:36 UTC
(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.
Comment 26 Jonathan Druart 2024-01-26 08:09:46 UTC
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.
Comment 27 Jonathan Druart 2024-01-26 08:10:16 UTC
Created attachment 161484 [details] [review]
Bug 26297: Add patron categories to_api_mapping

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 28 Jonathan Druart 2024-01-26 08:10:19 UTC
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
Comment 29 Jonathan Druart 2024-01-26 08:10:22 UTC
Created attachment 161486 [details] [review]
Bug 26297: Add tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 30 Jonathan Druart 2024-01-26 08:10:24 UTC
Created attachment 161487 [details] [review]
Bug 26297: Rename date_of_birth_required to lower_age_limit in api_mapping
Comment 31 Jonathan Druart 2024-01-26 08:10:27 UTC
Created attachment 161488 [details] [review]
Bug 26297: Update API spec

lower_age_limit is a number, not a boolean
Comment 32 Pedro Amorim 2024-01-26 09:59:13 UTC
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'
Comment 33 Pedro Amorim 2024-01-26 10:02:05 UTC
(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!
Comment 34 Pedro Amorim 2024-03-27 14:28:44 UTC
Hi, please let me know if there's something else required here!
Comment 35 Tomás Cohen Arazi 2024-04-04 12:34:56 UTC
(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.
Comment 36 Tomás Cohen Arazi 2024-04-23 12:53:27 UTC
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).
Comment 37 Tomás Cohen Arazi 2024-04-23 12:57:00 UTC
* The change on the required permissions broke the tests
Comment 38 Tomás Cohen Arazi 2024-04-23 13:00:12 UTC
(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
Comment 39 Tomás Cohen Arazi 2024-04-23 13:05:53 UTC
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>
Comment 40 Tomás Cohen Arazi 2024-04-23 13:05:56 UTC
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>
Comment 41 Tomás Cohen Arazi 2024-04-23 13:05:59 UTC
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>
Comment 42 Tomás Cohen Arazi 2024-04-23 13:06:02 UTC
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>
Comment 43 Tomás Cohen Arazi 2024-04-23 13:10:18 UTC
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.
Comment 44 Pedro Amorim 2024-04-24 11:02:09 UTC
Created attachment 165453 [details] [review]
Bug 26297: (QA follow-up): Update tests

prove t/db_dependent/api/v1/patron_categories.t
Comment 45 Tomás Cohen Arazi 2024-04-26 12:53:17 UTC
Created attachment 165655 [details] [review]
Bug 26297: (QA follow-up): Update tests

prove t/db_dependent/api/v1/patron_categories.t
Comment 46 Tomás Cohen Arazi 2024-04-26 13:27:14 UTC
*** Bug 34007 has been marked as a duplicate of this bug. ***
Comment 47 Katrin Fischer 2024-04-29 16:55:12 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 48 Martin Renvoize 2024-04-30 06:30:34 UTC
There's a merge error here leading to an invalid spec file.
Comment 49 Martin Renvoize 2024-04-30 06:32:57 UTC
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>
Comment 50 Martin Renvoize 2024-04-30 06:34:09 UTC
Last patch attached needed on main Katrin
Comment 51 Katrin Fischer 2024-04-30 07:19:54 UTC
Follow-up will be pushed to main.
Comment 52 Katrin Fischer 2024-04-30 10:01:41 UTC
Pushed for 24.05!

Well done everyone, thank you!