Bug 36480

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>

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>

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"
    }
]

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>

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>

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".

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.

(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?

Pushed for 24.05!

Well done everyone, thank you!

Pushed to 23.11.x for 23.11.06

Enhancement will not be backported to 23.05.x

Created attachment 169286 [details] [review]
Bug 36480: (follow-up) Add missing library_id parameter

The /libraries/{library_id}/desks endpoint was missing the
library_id parameter definition from the swagger specification.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Follow-up pushed

Beware there's two follow-ups in `main`

"Bug 36480: (follow-up) Add missing library_id parameter"
Backported to 23.11.x for 23.11.08

Not backporting to 23.05.x unless requested