If a patron has an item level hold with pickup location set to library A and staff then (via the Holds tab in the patron view) changes pickup location to library B, itemnumber is lost from the hold. This is a definite problem when the items within a bibliographic record are different things (separate volumes for example). This can be fixed by adding some code in Koha/REST/V1/Hold.pm This bit before the ModReserve call: if($hold->itemnumber()) { $params->{itemnumber} = $hold->itemnumber(); } However, given the FIXME comment in C4/Reserves.pm for ModReserve, I'm wondering if this is the desired fix: "Note that the forgoing can have the effect of causing item-level hold requests to turn into title-level requests. This will be fixed once reserves has separate columns for requested itemnumber and supplying itemnumber." A different method of solving this issue could be to provide itemnumber as part of the pickup-location-change call instead and keep the rest of the REST-API and ModReserve the way it works now. I'm somewhat unfamiliar with the how the Mojolicious system works, so I haven't looked into that method yet.
(In reply to Stefan Berndtsson from comment #0) > If a patron has an item level hold with pickup location set to library A and > staff then (via the Holds tab in the patron view) changes pickup location to > library B, itemnumber is lost from the hold. This is a definite problem when > the items within a bibliographic record are different things (separate > volumes for example). > > This can be fixed by adding some code in Koha/REST/V1/Hold.pm > > This bit before the ModReserve call: > > if($hold->itemnumber()) { > $params->{itemnumber} = $hold->itemnumber(); > } > > > However, given the FIXME comment in C4/Reserves.pm for ModReserve, I'm > wondering if this is the desired fix: > "Note that the forgoing can have the effect of causing > item-level hold requests to turn into title-level requests. This > will be fixed once reserves has separate columns for requested > itemnumber and supplying itemnumber." > > > A different method of solving this issue could be to provide itemnumber as > part of the pickup-location-change call instead and keep the rest of the > REST-API and ModReserve the way it works now. I'm somewhat unfamiliar with > the how the Mojolicious system works, so I haven't looked into that method > yet. I think your proposed solution is the right onw.
Created attachment 72436 [details] [review] Bug 20167: Regression test
Created attachment 72437 [details] [review] Bug 20167: Make changing a hold pass the itemnumber to ModReserve If you have an item-level hold, changing an attribute (like priority, pickup location or suspended until date) makes the hold a biblio-level hold, because ModReserve is not passed the current itemnumber. This patch uses the hold's itemnumber and passes it to the ModReserve call. To test: - Run the regression tests from the previous patch: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail - Apply this patch - Run the tests: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! - Sign off :-D
Created attachment 72479 [details] [review] Bug 20167: Regression test Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Created attachment 72480 [details] [review] Bug 20167: Make changing a hold pass the itemnumber to ModReserve If you have an item-level hold, changing an attribute (like priority, pickup location or suspended until date) makes the hold a biblio-level hold, because ModReserve is not passed the current itemnumber. This patch uses the hold's itemnumber and passes it to the ModReserve call. To test: - Run the regression tests from the previous patch: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail - Apply this patch - Run the tests: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Created attachment 72534 [details] [review] Bug 20167: Regression test Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72535 [details] [review] Bug 20167: Make changing a hold pass the itemnumber to ModReserve If you have an item-level hold, changing an attribute (like priority, pickup location or suspended until date) makes the hold a biblio-level hold, because ModReserve is not passed the current itemnumber. This patch uses the hold's itemnumber and passes it to the ModReserve call. To test: - Run the regression tests from the previous patch: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail - Apply this patch - Run the tests: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Pushed to master for 18.05, thanks to all for your hard work.
Awesome work all! Pushed to stable for 17.11.04
Pushed to 17.05.x for v17.05.10 I had to adapt since 17.05.x is still using C4::Reserves::GetReserve instead of Koha::Holds->find