Bug 36480 - Add GET /libraries/:library_id/desks
Summary: Add GET /libraries/:library_id/desks
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: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 36481
  Show dependency treegraph
 
Reported: 2024-04-01 20:21 UTC by Tomás Cohen Arazi
Modified: 2024-04-26 16:07 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds an API endpoint for requesting a list of desks for a library. For example: http://127.0.0.1:8080/api/v1/libraries/cpl/desks
Version(s) released in:
24.05.00


Attachments
Bug 36480: Add Koha::Library->desks (2.37 KB, patch)
2024-04-01 20:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36480: Add GET /libraries/:library_id/desks (8.02 KB, patch)
2024-04-01 20:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36480: Add Koha::Library->desks (2.42 KB, patch)
2024-04-02 06:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 36480: Add GET /libraries/:library_id/desks (8.05 KB, patch)
2024-04-02 06:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 36480: Add Koha::Library->desks (2.48 KB, patch)
2024-04-04 10:48 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36480: Add GET /libraries/:library_id/desks (8.12 KB, patch)
2024-04-04 10:48 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2024-04-01 20:21:24 UTC
I propose to add an endpoint for fetching a library's list of desks. Context is bug 36237.
Comment 1 Tomás Cohen Arazi 2024-04-01 20:29:17 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2024-04-01 20:29:21 UTC Comment hidden (obsolete)
Comment 3 David Nind 2024-04-02 06:07:11 UTC
Created attachment 164233 [details] [review]
Bug 36480: Add Koha::Library->desks

We add an accessor for the related desks. Tests are added.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Library.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2024-04-02 06:07:14 UTC
Created attachment 164234 [details] [review]
Bug 36480: Add GET /libraries/:library_id/desks

This patch adds the mentioned endpoint. For it, it does:

* Add Koha::Desk->to_api_mapping
* Add desk.yaml with the correct data structure for desks
* Add the route to the spec
* Add tests

Note: Lucas and I had doubts about the right return value for when the feature is disabled.
I opted for returning 404 with a message telling the feature is disabled. This can be discussed.

To test:
1. Apply this patches
2. Run:
   $ ktd
  k$ qa
=> SUCCESS: All green, all tests pass!
3. Play with this using Postman.
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2024-04-02 06:13:24 UTC
Testing notes (using KTD):

1. Tests pass before and after patches applied:
   - prove prove t/db_dependent/Koha/Library.t
   - prove t/db_dependent/api/v1/libraries.t

2. Enable these system preferences:
   - UseCirculationDesks
   - RESTBasicAuth 

3. Test the new API endpoint using Postman:
   - In the staff interface, add some desks for different libraries, some with multiple desks
   - Add a get request to request the desks for a library, for example add two desks to Centerville and one to Fairfield:
     . Set Basic Auth in Postamn use koha/koha
     . http://127.0.0.1:8080/api/v1/libraries/cpl/desks
     . http://127.0.0.1:8080/api/v1/libraries/ffl/desks
   - Results should return the desk information for the library, for example:
[
    {
        "desk_id": 11,
        "library_id": "CPL",
        "name": "Desk 1"
    },
    {
        "desk_id": 13,
        "library_id": "CPL",
        "name": "Desk 2"
    }
]
Comment 6 Pedro Amorim 2024-04-04 10:48:22 UTC
Created attachment 164413 [details] [review]
Bug 36480: Add Koha::Library->desks

We add an accessor for the related desks. Tests are added.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Library.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 7 Pedro Amorim 2024-04-04 10:48:24 UTC
Created attachment 164414 [details] [review]
Bug 36480: Add GET /libraries/:library_id/desks

This patch adds the mentioned endpoint. For it, it does:

* Add Koha::Desk->to_api_mapping
* Add desk.yaml with the correct data structure for desks
* Add the route to the spec
* Add tests

Note: Lucas and I had doubts about the right return value for when the feature is disabled.
I opted for returning 404 with a message telling the feature is disabled. This can be discussed.

To test:
1. Apply this patches
2. Run:
   $ ktd
  k$ qa
=> SUCCESS: All green, all tests pass!
3. Play with this using Postman.
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 8 Pedro Amorim 2024-04-04 10:48:49 UTC
I'm unsure about the 404 when the UseCirculationDesks feature is disabled.
Having said that, I don't know what to propose as an alternative, either.
Looking at REST/V1/Patrons.pm, it renders a 403 response if AllowPatronToSetCheckoutsVisibilityForGuarantor is disabled.
We should at least agree on a pattern and stick to it.
There doesn't seem to exist a 100% fit of an HTTP code for "Feature disabled".
Comment 9 Pedro Amorim 2024-04-04 10:52:25 UTC
Doh. Only now read the "Note: Lucas and I had doubts about the right return value for when the feature is disabled." in the original commit. Yeah, not a blocker, but deffo a pattern we should agree and stick to it going forward imo.
Comment 10 Katrin Fischer 2024-04-26 15:25:34 UTC
(In reply to Pedro Amorim from comment #9)
> Doh. Only now read the "Note: Lucas and I had doubts about the right return
> value for when the feature is disabled." in the original commit. Yeah, not a
> blocker, but deffo a pattern we should agree and stick to it going forward
> imo.

Do we need to follow-up on this or was it covered by our recent discussion on return values?
Comment 11 Katrin Fischer 2024-04-26 16:07:22 UTC
Pushed for 24.05!

Well done everyone, thank you!