|
Description
Nick Clemens (kidclamp)
2023-06-14 11:14:53 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. 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 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 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 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 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 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. 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> (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. 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? 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. Created attachment 157976 [details] [review] Bug 34008: Remove explicit embed in controller. Created attachment 157977 [details] [review] Bug 34008: Rename item_type in api. * 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. Thank you, Marcel, for tidying up the code! (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. 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 Working on follow-ups for this one. (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! 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> 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> 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> Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 23.05.x for 23.05.06 *** Bug 17856 has been marked as a duplicate of this bug. *** |