I propose to add an endpoint for fetching a library's list of desks. Context is bug 36237.
Created attachment 164226 [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
Created attachment 164227 [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
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!