Bug 17390 - Add REST API endpoint for Authorised Values
Summary: Add REST API endpoint for Authorised Values
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 8 votes (vote)
Assignee: Bugs List
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 34920 32981 32997
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-03 15:31 UTC by Kyle M Hall
Modified: 2024-02-16 15:40 UTC (History)
8 users (show)

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


Attachments
Bug 17390 - Add REST API endpoint for Authorised Values (5.09 KB, patch)
2016-10-03 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17390 - Update swagger.min.json (73.27 KB, patch)
2016-10-03 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17390 - Add REST API endpoint for Authorised Values (5.33 KB, patch)
2019-08-07 08:53 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17390: Update for current Koha codebase (3.46 KB, patch)
2019-08-07 08:54 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17390: Add /authorised_values endpoint (26.50 KB, patch)
2020-06-12 14:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17390: Add /authorised_values endpoint (30.62 KB, patch)
2020-06-12 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17390: Add /authorised_values endpoint (23.13 KB, patch)
2024-02-14 10:14 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 Kyle M Hall 2016-10-03 15:31:41 UTC
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.
Comment 1 Kyle M Hall 2016-10-03 15:33:33 UTC
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.
Comment 2 Kyle M Hall 2016-10-03 15:33:41 UTC
Created attachment 55980 [details] [review]
Bug 17390 - Update swagger.min.json
Comment 3 Katrin Fischer 2016-10-03 15:36:05 UTC
Jonathan is about to move this to the Koha namespace - we should try to get those patches in first, I think?
Comment 4 Josef Moravec 2019-08-07 08:53:56 UTC
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.
Comment 5 Josef Moravec 2019-08-07 08:54:03 UTC
Created attachment 92037 [details] [review]
Bug 17390: Update for current Koha codebase
Comment 6 Jonathan Druart 2020-06-07 10:05:12 UTC
I am going to work on this.
Comment 7 Jonathan Druart 2020-06-12 14:20:05 UTC
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
Comment 8 Tomás Cohen Arazi 2020-06-12 15:56:15 UTC
The commit is missing the controller code.
Comment 9 Jonathan Druart 2020-06-12 16:24:49 UTC
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
Comment 10 Tomás Cohen Arazi 2020-06-12 17:26:36 UTC
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.
Comment 11 Jonathan Druart 2020-06-13 10:43:47 UTC
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.
Comment 12 Jonathan Druart 2020-06-13 10:56:30 UTC
> 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?
Comment 13 Tomás Cohen Arazi 2020-06-13 11:21:03 UTC
(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.
Comment 14 Tomás Cohen Arazi 2020-06-13 11:39:08 UTC
(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.
Comment 15 Katrin Fischer 2020-06-13 17:05:48 UTC
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
Comment 16 Pedro Amorim 2024-02-12 13:52:17 UTC
Is this still relevant after bug 32981 and 32997?
Comment 17 Arthur Suzuki 2024-02-12 14:18:31 UTC
Given there is another implementation I guess this one can be closed duplicate

*** This bug has been marked as a duplicate of bug 32981 ***
Comment 18 Martin Renvoize 2024-02-13 16:49:33 UTC
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.
Comment 19 Martin Renvoize 2024-02-14 10:14:10 UTC
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
Comment 20 Martin Renvoize 2024-02-14 10:14:53 UTC
Just a quick rebase here so it applies again, I've not tested or reviewed however to setting to 'Needs signoff'