Bug 36480

Summary: Add GET /libraries/:library_id/desks
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to main --- QA Contact:
Severity: enhancement    
Priority: P5 - low CC: david, lucas, martin.renvoize, nick, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36237
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
Bug Depends on:    
Bug Blocks: 36481    
Attachments: Bug 36480: Add Koha::Library->desks
Bug 36480: Add GET /libraries/:library_id/desks
Bug 36480: Add Koha::Library->desks
Bug 36480: Add GET /libraries/:library_id/desks
Bug 36480: Add Koha::Library->desks
Bug 36480: Add GET /libraries/:library_id/desks

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!