Bug 34008 - REST API: Add a list (GET) endpoint for itemtypes
Summary: REST API: Add a list (GET) endpoint for itemtypes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andreas Jonsson
QA Contact: Marcel de Rooy
URL:
Keywords: rel_23_11_candidate
Depends on:
Blocks:
 
Reported: 2023-06-14 11:14 UTC by Nick Clemens (kidclamp)
Modified: 2023-11-14 15:09 UTC (History)
6 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:
23.11.00,23.05.06


Attachments
Bug 34008: Add REST endpoint for list of itemtypes (14.00 KB, patch)
2023-09-16 10:53 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 34008: Add REST endpoint for list of itemtypes (13.95 KB, patch)
2023-09-17 08:31 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 34008: Add REST endpoint for list of itemtypes (13.93 KB, patch)
2023-09-17 08:36 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 34008: Add REST endpoint for list of itemtypes (13.91 KB, patch)
2023-09-17 08:38 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 34008: Add REST endpoint for list of itemtypes (13.96 KB, patch)
2023-09-17 12:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 34008: Add REST endpoint for list of itemtypes (14.51 KB, patch)
2023-10-26 12:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34008: Remove explicit embed in controller. (848 bytes, patch)
2023-10-27 10:00 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 34008: Rename item_type in api. (12.50 KB, patch)
2023-10-27 10:01 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 34008: Add REST endpoint for list of itemtypes (19.43 KB, patch)
2023-10-30 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34008: (QA follow-up) 'item_type_id' should be used (3.66 KB, patch)
2023-10-30 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34008: Harmonize attribute names (8.91 KB, patch)
2023-10-30 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-06-14 11:14:53 UTC

    
Comment 1 Andreas Jonsson 2023-09-16 10:12:07 UTC

I'm peeking at the authorised_values and see that it requires permission catalogue.  So, I will assume that this is appropriate also for item types.

I will also prefer 'itemtypes' over 'item_types' as itemtypes is far more commonly used in the code base.
Comment 2 Andreas Jonsson 2023-09-16 10:53:16 UTC
Created attachment 155734 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any
Comment 3 David Nind 2023-09-16 21:51:21 UTC
Hi Andreas.

I've had a go at testing using koha-testing-docker (KTD) (note: I'm not a developer, and APIs are not something I know a lot about!).

1. Using either curl or Postman for the requests, I get "Page Not Found (development mode)" (curl = HTML output; postman = the same).

2. The API documentation also comes up with page not found (http://127.0.0.1:8081/api/v1/.html)

3. I tested with another REST API bug for sign off (bug 34333), and this didn't happen. Also, when I go back to master, the API docs are available again.

4. Possibly, this is something to do with the KTD set up? Or something isn't working as expected with your bug?

David Nind
Comment 4 Andreas Jonsson 2023-09-17 08:31:08 UTC
Created attachment 155737 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any
Comment 5 Andreas Jonsson 2023-09-17 08:36:32 UTC
Created attachment 155738 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any
Comment 6 Andreas Jonsson 2023-09-17 08:38:00 UTC
Created attachment 155739 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any
Comment 7 Andreas Jonsson 2023-09-17 08:38:43 UTC
Thank you for testing David!

It seems that the patch did not work on master due to Bug 33971 removing the q_header parameter from the API, which caused the loading of the API-schema to fail.

I have updated the patch.
Comment 8 David Nind 2023-09-17 12:08:56 UTC
Created attachment 155741 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2023-09-17 12:09:55 UTC
(In reply to Andreas Jonsson from comment #7)
> Thank you for testing David!
> 
....
> 
> I have updated the patch.

Thanks Andreas! It is now working and I've sigend it off.
Comment 10 Marcel de Rooy 2023-10-26 12:07:52 UTC
Created attachment 157915 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] perltidy -b t/db_dependent/api/v1/itemtypes.t # Resolve bad score of 44
[EDIT] chmod 755 t/db_dependent/api/v1/itemtypes.t
[EDIT] perltidy -b Koha/REST/V1/ItemTypes.pm
Lesson: Please run qa tools yourself and adjust accordingly?
Comment 11 Tomás Cohen Arazi 2023-10-27 04:46:54 UTC
I see a few problems with this implementation. I'm marking this one rel_23_11_candidate because they are easily solvable and I can help.

* What I will need input on is on the attributes names. This needs work. To start with, we call them item_type across the other endpoints. So the endpoint name and the id need a change. The rest of the attribute names need some love (i.e. we hide our weird internal names and put something better, along with a mapping).

* The controller shouldn't deal explicitly with embeds. Pick the /cities endpoint controller as an example and you'll be fine.
Comment 12 Andreas Jonsson 2023-10-27 10:00:57 UTC
Created attachment 157976 [details] [review]
Bug 34008: Remove explicit embed in controller.
Comment 13 Andreas Jonsson 2023-10-27 10:01:22 UTC
Created attachment 157977 [details] [review]
Bug 34008: Rename item_type in api.
Comment 14 Andreas Jonsson 2023-10-27 10:07:14 UTC
* I have removed the explicit embed.
* I've attached a patch for renaming to item_type
* As for renaming other attributes I am open to suggestions.  My personal preference is to stay consistent with the database column names, however.
Comment 15 Andreas Jonsson 2023-10-27 10:09:23 UTC
Thank you, Marcel, for tidying up the code!
Comment 16 Tomás Cohen Arazi 2023-10-27 11:38:49 UTC
(In reply to Andreas Jonsson from comment #14)
> * I have removed the explicit embed.

Good!

> * I've attached a patch for renaming to item_type

Good!

> * As for renaming other attributes I am open to suggestions.  My personal
> preference is to stay consistent with the database column names, however.

https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.2:_Resource_names

and we have used RFCs to propose attribute namings in the past:

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

I wouldn't suggest you add an RFC, but pick ideas from there.
Comment 17 Katrin Fischer 2023-10-28 11:31:10 UTC
I made some suggestions on the wiki page you added:
https://wiki.koha-community.org/wiki/Item_types_endpoint_RFC

From looking at the other endpoints I concluded:
* we usually don't use abbreviations
* we use underscores to separate words
* items endpoint uses not_for_loan_status - so we should use: not_for_loan
Comment 18 Tomás Cohen Arazi 2023-10-30 14:35:46 UTC
Working on follow-ups for this one.
Comment 19 Tomás Cohen Arazi 2023-10-30 15:04:58 UTC
(In reply to Katrin Fischer from comment #17)
> I made some suggestions on the wiki page you added:
> https://wiki.koha-community.org/wiki/Item_types_endpoint_RFC
> 
> From looking at the other endpoints I concluded:
> * we usually don't use abbreviations
> * we use underscores to separate words
> * items endpoint uses not_for_loan_status - so we should use: not_for_loan

Thanks!
Comment 20 Tomás Cohen Arazi 2023-10-30 16:18:16 UTC
Created attachment 158082 [details] [review]
Bug 34008: Add REST endpoint for list of itemtypes

Test plan:
* Enable the system preference RESTBasicAuth
* curl -s --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should give 401 Unauthorized
* curl -s -u koha:koha --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should produce JSON-list of itemtypes
* curl -s -u koha:koha --header "x-koha-embed: translated_descriptions" --request GET http://kohadev-intra.mydnsname.org:8081/api/v1/itemtypes
  should include the field translated_descriptions containing the translated descriptions, if any

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] perltidy -b t/db_dependent/api/v1/itemtypes.t # Resolve bad score of 44
[EDIT] chmod 755 t/db_dependent/api/v1/itemtypes.t
[EDIT] perltidy -b Koha/REST/V1/ItemTypes.pm
Lesson: Please run qa tools yourself and adjust accordingly?
Edit (tcohen): I restored the item_type_translated_description.yaml file
as the entire API was broken because of the lack of it.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2023-10-30 16:18:19 UTC
Created attachment 158083 [details] [review]
Bug 34008: (QA follow-up) 'item_type_id' should be used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2023-10-30 16:18:23 UTC
Created attachment 158084 [details] [review]
Bug 34008: Harmonize attribute names

This patch harmonizes the attribute names with what is used for `items`
and `checkouts` in terms of terminology.

It also adapts the tests so they are less random failure-prone (they had
a fixed value for the item type, which might make things explode if the
chosen value already exists on the DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2023-11-01 20:26:39 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 24 Fridolin Somers 2023-11-08 08:42:15 UTC
Pushed to 23.05.x for 23.05.06