Bug 23449: Get all available pickup locations from REST API #100
Conversation
… item search availability There seems to be confusion on what the availability status of an item should be in search context when there is a hold placed on that item. This is outside the scope of my current work, but this patch adds a TODO for further work.
…e translated When authorised values have a translated value in lib-column, some tests seem to be failing. This simple patch makes api/v1/availability.t to query the lib value instead of having it hardcoded.
This patch adds logic to check available item pickup locations. A pickup location is available if 1. branches.pickup_location is set to 1 2. There are no branch transfer limits between item's holdingbranch and the desired pickup location To test: 1. prove t/db_dependent/Koha/Availability/Checks/Item.t
…lity::Hold
This patch adds item's available hold pickup locations to availability object.
This functionality is optimal - provide "query_pickup_locations" parameter
with a true boolean value in order to have your Koha::Item::Availability::Hold
object gather the list of available pickup locations.
When this functionality is used, your item availability object will contain
a Koha::Exceptions::Item::PickupLocations as an availability note as follows:
(example PERL HASH with irrelevant keys deleted)
{
"itemnumber": 123,
"availability": {
"available": true,
"notes": {
"Koha::Exceptions::Item::PickupLocations": {
"from_library": "CPL",
"to_libraries": ["FPL", "MPL"]
}
}
}
}
(NOTE! In JSON representation via REST API, "Koha::Exceptions::" is removed
from the note type, so that only "Item::PickupLocations" remain)
To test:
1. prove t/db_dependent/Koha/Item/Availability/Hold/Opac/ItemStatus.t
…via holdability API endpoints This patch adds OpenAPI definitions for pickup locations check. Since pickup locations check is optional, you have to provide "query_pickup_locations=1" parameter in your HTTP request's query parameters in order to enable this functionality. This patch adds no logic, just OpenAPI definitions.
This patch adds REST API controllers and tests for requesting available pickup locations for a hold. See previous patches of this Bug number for more documentation. Usage is as follows: GET http://lib/api/v1/availability/item/hold?itemnumber=123&query_pickup_locations=1 To test: 1. prove t/db_dependent/api/v1/availability.t
This patch adds biblio-level pickup location information to both Koha::Biblio::Availability::Hold class and REST API via /api/v1/availability/biblio/hold?biblionumber=123&query_pickup_locations=1 To test: 1. prove t/db_dependent/Koha/Biblio/Availability/Hold.t 2. prove t/db_dependent/api/v1/availability.t
taskula
force-pushed
the
Bug_23449-Get_available_pickup_locations_from_REST_API
branch
from
5633858
to
33bc5de
Compare
…nsfer_limit_code This patch adds a new method Koha::Item->branch_transfer_limit_code that returns the item's itemtype or ccode depending on system preference BranchTransferLimitsType. To test: 1. prove t/db_dependent/Koha/Items.t
… limits at once Have mercy on Koha's database. This patch reduces the amount of queries performed on Koha's database from n to 1 where n is the amount of branches with pickup_location => 1. On my dev machine, it improves performance by n>20: 1.5 times n>100: 3 times n>1000: 5 times To test: 1. prove t/db_dependent/Koha/Item/Availability/Hold/Opac/ItemStatus.t 2. prove t/db_dependent/Koha/Biblio/Availability/Hold.t 3. prove t/db_dependent/api/v1/availability.t
|
Added some patches for optimization (reducing the number of queries performed on Koha's database from this feature) and code cleanup. |
Adds Biblio::PickupLocations to availability reasons of Swagger.
|
It seems that without query_pickup_locations=1 there's an empty Biblio::PickupLocations entry in notes. That seems kind of misleading. Would it be possible to omit it completely unless pickup locations were requested? |
|
It would be very useful if the API returned branchname in addition to the code. Otherwise we need to look up the libraries separately, and while it's just one API call, it would be nice to be able to avoid it. |
I disagree, it would make this API call unnecessarily slower and bigger in size with those cases where we don't need the human readable name. |
Well, as far as I can see the data is already there, but it only takes the branchcode.. |
|
And I don't know who doesn't need the human-readable name, but Finna does. |
|
The size of data will still be increased. Also, I like the idea of one program / endpoint doing its thing well and fast rather than making it a complex thing that does everything and is hard to debug for the programmer. Do you agree? |
|
We can argue this to the world's end, but I don't think returning the branch name is out of scope for this API. From API consumer's point of view it's way easier to get the relevant information with a single call instead of multiple, and it also avoid a HTTP roundtrip. The size increase only happens when you indicate you want the pickup locations. I tested with a request for hold/biblio for a biblio with two items, and the size of the complete response was 1,4k. In this context adding descriptions for the pickup locations is a small fraction more. If the response size is an issue, you should definitely be worried about all the other stuff being returned if you just want the pickup locations. |
|
Is it possible to cache seldomly changing objects on VuFind's side? As far as I understand this is not the only endpoint returning only branchcodes.
…On 16 August 2019 15.51.55 EEST, Ere Maijala ***@***.***> wrote:
We can argue this to the world's end, but I don't think returning the
branch name is out of scope for this API. From API consumer's point of
view it's way easier to get the relevant information with a single call
instead of multiple, and it also avoid a HTTP roundtrip. The size
increase only happens when you indicate you want the pickup locations.
I tested with a request for hold/biblio for a biblio with two items,
and the size of the complete response was 1,4k. In this context adding
descriptions for the pickup locations is a small fraction more. If the
response size is an issue, you should definitely be worried about all
the other stuff being returned if you just want the pickup locations.
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#100 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
If we add branch description then we would have to make it a top level JSON object (instead of repeating the descriptions for item level pickup locations and biblio level pickup locations). I don't like that it makes it hard to learn the API because if it is a availability endpoint why would such a thing return branch descriptions? The round trip is not a problem here because one can request both pickup locations and branch descriptions in parallel. |
Biblio::PickupLocations is returned even when query_pickup_locations is not given. It shouldn't be there! To test: 1. prove t/db_dependent/Koha/Biblio/Availability/Hold.t 2. prove t/db_dependent/api/v1/availability.t 3. prove t/db_dependent/Koha/Item/Availability/Hold/Opac/ItemStatus.t Steps 1 and 2 should fail.
… requested When calling Koha::Biblio::Availability::Hold with query_pickup_locations disabled, the availability notes would still include the note Koha::Exceptions::Biblio::PickupLocations. We don't want it to be there in this case. This patch fixes a bug that caused it to be shown even when not requested. To test: 1. prove t/db_dependent/Koha/Biblio/Availability/Hold.t 2. prove t/db_dependent/api/v1/availability.t 3. prove t/db_dependent/Koha/Item/Availability/Hold/Opac/ItemStatus.t
|
If there are no pickup branches available then there is |
To test: 1. prove t/db_dependent/Koha/Biblio/Availability/Hold.t 2. Observe failing test
To test: 1. prove t/db_dependent/Koha/Biblio/Availability/Hold.t
This should now be fixed. |
To test: 1. prove t/db_dependent/api/v1/availability.t
…limit applies for all libraries "to_library" is defined as a string in OpenAPI definitions. When item cannot be transferred to any library, we would need an array for this field. But this could break current API clients. Simply drop the field. To test: 1. prove t/db_dependent/api/v1/availability.t
|
I assume this is ready to be merged? |
Yes, this seems to be working fine in one of our production servers. |
https://tiketti.koha-suomi.fi/issues/3375