It would be good and useful to at least be able to get the list of authorised values from Koha. The ability to create, update and delete could come later.
Created attachment 55979 [details] [review] Bug 17390 - Add REST API endpoint for Authorised Values It would be good and useful to at least be able to get the list of authorised values from Koha. The ability to create, update and delete could come later.
Created attachment 55980 [details] [review] Bug 17390 - Update swagger.min.json
Jonathan is about to move this to the Koha namespace - we should try to get those patches in first, I think?
Created attachment 92036 [details] [review] Bug 17390 - Add REST API endpoint for Authorised Values It would be good and useful to at least be able to get the list of authorised values from Koha. The ability to create, update and delete could come later.
Created attachment 92037 [details] [review] Bug 17390: Update for current Koha codebase
I am going to work on this.
Created attachment 105820 [details] [review] Bug 17390: Add /authorised_values endpoint This patch add the different routes for authorised values: * GET /authorised_values * GET /authorised_values/{authorised_value_id} * POST /authorised_values * PUT /authorised_values/{authorised_value_id} * DELETE /authorised_values/{authorised_value_id} Test plan: - Make sure the tests in t/db_dependent/api/v1/authorised_values.t pass - Test the different routes. For instance: GET /authorised_values # list all the avs GET /authorised_values?category=YES_NO # list all the YES_NO avs POST /authorised_values { "category": "YES_NO", "description": "not sure", "opac_description": "something else", "value": "maybe" } DELETE /authorised_values/X # with X the AV id of "maybe" Sponsored-by: Orex Digital
The commit is missing the controller code.
Created attachment 105849 [details] [review] Bug 17390: Add /authorised_values endpoint This patch add the different routes for authorised values: * GET /authorised_values * GET /authorised_values/{authorised_value_id} * POST /authorised_values * PUT /authorised_values/{authorised_value_id} * DELETE /authorised_values/{authorised_value_id} Test plan: - Make sure the tests in t/db_dependent/api/v1/authorised_values.t pass - Test the different routes. For instance: GET /authorised_values # list all the avs GET /authorised_values?category=YES_NO # list all the YES_NO avs POST /authorised_values { "category": "YES_NO", "description": "not sure", "opac_description": "something else", "value": "maybe" } DELETE /authorised_values/X # with X the AV id of "maybe" Sponsored-by: Orex Digital
I see a design issue on this route. As authorised values belong to auth val categories, we should really have that in the path: POST /authorised_value_categories GET /authorised_value_categories GET /authorised_value_categories/:authorised_value_category_id POST /authorised_value_categories/:authorised_value_category_id/values GET /authorised_value_categories/:authorised_value_category_id/values if this routes would be used for CRUD operations we should probably contemplate the idea of embedding the library limits on the response using x-koha-embed (i.e. if you are going to edit an authorised value in a form, you will want the library limits, and also some information about the category for rendering purposes. Usability and controller implementation: If you are going to use the routes only for rendering a dropdown somewhere, that form will have the field probably mapped to a specific auth val category. In that case you would be doing: GET /authorised_values?authorised_value_category_id=XX but the controller would not be as simple as the patch suggests, as it should consider library limits to start with. And as this is a general-purpose route, we cannot put constraints on what can be passed on the query parameters or even q=, and adding library limits to the combo, makes it a nightmare to implement, with really no need. And there's also permissions... Lets fix it by design: if we want to have (still need to figure the use case) a global search route for auth values, then make it require the the highest possible permissions and implement no restrictions on it. Or skip that route and implement: a. POST /authorised_value_categories b. GET /authorised_value_categories c. GET /authorised_value_categories/:authorised_value_category_id d. PUT /authorised_value_categories/:authorised_value_category_id e. DELETE /authorised_value_categories/:authorised_value_category_id f. POST /authorised_value_categories/:authorised_value_category_id/values g. GET /authorised_value_categories/:authorised_value_category_id/values h. GET /authorised_value_categories/:authorised_value_category_id/values/:authorised_value_id i. DELETE /authorised_value_categories/:authorised_value_category_id/values/:authorised_value_id Then on bug 25728 you would use: (g) for fetching the list (f) for adding a new value and the controller for (g) will just call Koha::AuthorisedValues->search_with_library_limits using the :authorised_value_category_id and passing the library_id that is read from then stashed user. Unless it is a superlibrarian, in which case the resultset to be passed to $c->objects->search would just be a regular ->search. Those routes, can then have more granular permissions requirements than a global one which would also be a nightmare to implement. I hope it makes sense. On of the things to consider, is just implementing the routes you need (i.e. not all of them). That way you don't get dragged into a rabbit hole.
I wanted to implement bug 25728 correctly by using a new REST API endpoint. I followed the approved RFC and what was done for cities but apparently that was not enough and the whole RFC must be rethink. I don't know how many of wrong RFC are on the wiki, but it would be good to remove or adjust them if they do not longer match what we want. I may be back on this one later, but at the moment I don't have more time to dedicate to this.
> f. POST /authorised_value_categories/:authorised_value_category_id/values This route reads wrong to me. How is that different than adding a patron with a given category? Or with funds, the route is GET /acquisitions/funds, should not it be /acquisitions/budgets/:budget_id:/funds in that case? Also note that there is no "authorised_value_category_id", but category_name is the PK of the authorised_value_categories table. Should we add one or you meant category_name?
(In reply to Jonathan Druart from comment #11) > I wanted to implement bug 25728 correctly by using a new REST API endpoint. > I followed the approved RFC and what was done for cities but apparently that > was not enough and the whole RFC must be rethink. I don't know how many of > wrong RFC are on the wiki, but it would be good to remove or adjust them if > they do not longer match what we want. Those RFC were marked as WIP by the author (Josef?) And I only added my two cents. My opinions are just opinions, Jonathan. What I highlighted is the fact that for some uses we'd better find another approach than the global route. > I may be back on this one later, but at the moment I don't have more time to > dedicate to this. I'm sorry to read that.
(In reply to Jonathan Druart from comment #12) > > f. POST /authorised_value_categories/:authorised_value_category_id/values > > This route reads wrong to me. > How is that different than adding a patron with a given category? Fair enough. > Or with funds, the route is GET /acquisitions/funds, should not it be > /acquisitions/budgets/:budget_id:/funds in that case? I belive it really depends on how you're gonna use it and what granularity you want for requiring permissions or limitations on the route. What I was pointing is the same that happened to the hold history bug: instead of a single search you need to fetch all results and deal with limitations on each of them, so I warned you about the 'global' route for fetching authorised values: it looks simple, but in the end only superlibrarians should be able to use it. As for POST you can safely use your code (and the global route) as I belive you can make any required checks in a really compact code in the controller. My aim was not to block you but suggest you constraint your target to what you really need instead of a whole complete API for authorised values so you don't get frustrated. So you can strip the more complex and debatable use cases and already have the route you need. Go ahead! > Also note that there is no "authorised_value_category_id", but category_name > is the PK of the authorised_value_categories table. Should we add one or you > meant category_name? We reached and agreement that ID's are appended _id for consistency. So I invented the name to make a point following the guidelines. That's why the RFC needs to be voted. We can sort this out easily with a conversation with Martin and a dev meeting.
Hi, I also hope we can move on this quickly and find some good solution, added to the next Dev meeting agenda: https://wiki.koha-community.org/wiki/Development_IRC_meeting_17_June_2020#Agenda
Is this still relevant after bug 32981 and 32997?
Given there is another implementation I guess this one can be closed duplicate *** This bug has been marked as a duplicate of bug 32981 ***
Actually.. there's a fair bit of additional functionality in this submission.. the other two bugs only implement listing functions and not add or edit.. so I think this could be rebased/reworked a little and retitled for those cases.
Created attachment 162138 [details] [review] Bug 17390: Add /authorised_values endpoint This patch add the different routes for authorised values: * GET /authorised_values * GET /authorised_values/{authorised_value_id} * POST /authorised_values * PUT /authorised_values/{authorised_value_id} * DELETE /authorised_values/{authorised_value_id} Test plan: - Make sure the tests in t/db_dependent/api/v1/authorised_values.t pass - Test the different routes. For instance: GET /authorised_values # list all the avs GET /authorised_values?category=YES_NO # list all the YES_NO avs POST /authorised_values { "category": "YES_NO", "description": "not sure", "opac_description": "something else", "value": "maybe" } DELETE /authorised_values/X # with X the AV id of "maybe" Sponsored-by: Orex Digital
Just a quick rebase here so it applies again, I've not tested or reviewed however to setting to 'Needs signoff'
I tried rebuilding the API definitions by using `yarn api:bundle`, but the command fails with several errors like the following: [48] api/v1/swagger/paths/authorised_values.yaml:298:11 at #/~1authorised_value_categories~1{authorised_value_category_name}~1authorised_values/get/responses/503/schema Can't resolve $ref 296 | description: Under maintenance 297 | schema: 298 | $ref: "../swagger.yaml#/definitions.json/definitions/error" 299 | x-koha-authorization: 300 | permissions: Error was generated by the bundler rule. As far as I can tell, this is because the definitions.json file was replaced by swagger.yaml and the references in api/v1/swagger/paths/authorised_values.yaml still point to that deleted file. I tried to manually fix it, but I am not familiar enough with Swagger YAML syntax and could not find the actual resolution. All I can do for now is point at the most likely culprit.