Bug 20167

Summary: Item hold is set to bibliographic hold when changing pickup location
Product: Koha Reporter: Stefan Berndtsson <stefan.berndtsson>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, josef.moravec, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 20167: Regression test
Bug 20167: Make changing a hold pass the itemnumber to ModReserve
Bug 20167: Regression test
Bug 20167: Make changing a hold pass the itemnumber to ModReserve
Bug 20167: Regression test
Bug 20167: Make changing a hold pass the itemnumber to ModReserve

Description Stefan Berndtsson 2018-02-09 10:36:25 UTC
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.
Comment 1 Tomás Cohen Arazi 2018-03-05 15:43:57 UTC
(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.
Comment 2 Tomás Cohen Arazi 2018-03-05 16:41:49 UTC
Created attachment 72436 [details] [review]
Bug 20167: Regression test
Comment 3 Tomás Cohen Arazi 2018-03-05 16:41:55 UTC
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
Comment 4 Claire Gravely 2018-03-06 14:19:11 UTC
Created attachment 72479 [details] [review]
Bug 20167: Regression test

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 5 Claire Gravely 2018-03-06 14:19:14 UTC
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>
Comment 6 Josef Moravec 2018-03-08 12:45:00 UTC
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>
Comment 7 Josef Moravec 2018-03-08 12:45:03 UTC
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>
Comment 8 Nick Clemens 2018-03-15 09:22:23 UTC
Pushed to master for 18.05, thanks to all for your hard work.
Comment 9 Nick Clemens 2018-03-23 12:51:34 UTC
Awesome work all! Pushed to stable for 17.11.04
Comment 10 Fridolin Somers 2018-03-27 11:41:43 UTC
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