Bug 20167 - Item hold is set to bibliographic hold when changing pickup location
Summary: Item hold is set to bibliographic hold when changing pickup location
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-09 10:36 UTC by Stefan Berndtsson
Modified: 2018-06-04 20:15 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20167: Regression test (1.13 KB, patch)
2018-03-05 16:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20167: Make changing a hold pass the itemnumber to ModReserve (1.20 KB, patch)
2018-03-05 16:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20167: Regression test (1.18 KB, patch)
2018-03-06 14:19 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 20167: Make changing a hold pass the itemnumber to ModReserve (1.25 KB, patch)
2018-03-06 14:19 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 20167: Regression test (1.24 KB, patch)
2018-03-08 12:45 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20167: Make changing a hold pass the itemnumber to ModReserve (1.31 KB, patch)
2018-03-08 12:45 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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