Bug 23116 - Cannot place overridden holds
Summary: Cannot place overridden holds
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 23107 (view as bug list)
Depends on: 17978 22650
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-13 10:38 UTC by Kyle M Hall
Modified: 2019-07-23 14:51 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.02


Attachments
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (3.40 KB, patch)
2019-06-13 10:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (3.53 KB, patch)
2019-06-13 10:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (3.53 KB, patch)
2019-06-18 15:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (4.60 KB, patch)
2019-06-19 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (4.65 KB, patch)
2019-06-19 15:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (4.75 KB, patch)
2019-06-24 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2019-06-13 10:38:24 UTC
A library appears to be able to place a second item level hold on an item a patron already has on hold if
A) AllowHoldPolicyOverride is enabled
and
B) the circ rule allow for multple item level holds.

Once the patron submits the hold requests though, the hold does not get stored in the database.

Because allowing two item level holds for the same item makes no sense, we should not allow
this attempt to take place, even if AllowHoldPolicyOverride is enabled.
Comment 1 Kyle M Hall 2019-06-13 10:41:26 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2019-06-13 10:44:20 UTC
Created attachment 90566 [details] [review]
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

A library appears to be able to place a second item level hold on an item a patron already has on hold if
A) AllowHoldPolicyOverride is enabled
and
B) the circ rule allow for multple item level holds.

Once the patron submits the hold requests though, the hold does not get stored in the database.

Because allowing two item level holds for the same item makes no sense, we should not allow
this attempt to take place, even if AllowHoldPolicyOverride is enabled.

Test Plan:
1) Enable AllowHoldPolicyOverride
2) Set up circ rules to allow for multiple item level holds on one record
3) Place an item level hold on a record
4) Note you can force placing a second item hold on that reocrd
5) Attempt to do so, it will not actually work
6) Apply this patch
7) Note you can no longer place another item level hold for the same item
   you just placed an item-level hold on
8) Note you can still force holds that contravene the circ rules for
   any and all other reasons
Comment 3 Nick Clemens 2019-06-13 11:04:53 UTC
*** Bug 23107 has been marked as a duplicate of this bug. ***
Comment 4 Owen Leonard 2019-06-18 14:23:09 UTC
I'm getting an error when I try to place an item-level hold:

Undefined subroutine &CGI::Compile::ROOT::home_vagrant_kohaclone_reserve_placerequest_2epl::canItemBeReserved called at /home/vagrant/kohaclone/reserve/placerequest.pl line 100
Comment 5 Kyle M Hall 2019-06-18 15:04:09 UTC
Created attachment 90754 [details] [review]
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

A library appears to be able to place a second item level hold on an item a patron already has on hold if
A) AllowHoldPolicyOverride is enabled
and
B) the circ rule allow for multple item level holds.

Once the patron submits the hold requests though, the hold does not get stored in the database.

Because allowing two item level holds for the same item makes no sense, we should not allow
this attempt to take place, even if AllowHoldPolicyOverride is enabled.

Test Plan:
1) Enable AllowHoldPolicyOverride
2) Set up circ rules to allow for multiple item level holds on one record
3) Place an item level hold on a record
4) Note you can force placing a second item hold on that reocrd
5) Attempt to do so, it will not actually work
6) Apply this patch
7) Note you can no longer place another item level hold for the same item
   you just placed an item-level hold on
8) Note you can still force holds that contravene the circ rules for
   any and all other reasons
Comment 6 Kyle M Hall 2019-06-18 15:04:35 UTC
Fixed! Can you give this one another try? Thanks!

(In reply to Owen Leonard from comment #4)
> I'm getting an error when I try to place an item-level hold:
> 
> Undefined subroutine
> &CGI::Compile::ROOT::home_vagrant_kohaclone_reserve_placerequest_2epl::
> canItemBeReserved called at /home/vagrant/kohaclone/reserve/placerequest.pl
> line 100
Comment 7 Nick Clemens 2019-06-19 15:15:14 UTC
Created attachment 90800 [details] [review]
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

A library appears to be able to place a second item level hold on an item a patron already has on hold if
A) AllowHoldPolicyOverride is enabled
and
B) the circ rule allow for multple item level holds.

Once the patron submits the hold requests though, the hold does not get stored in the database.

Because allowing two item level holds for the same item makes no sense, we should not allow
this attempt to take place, even if AllowHoldPolicyOverride is enabled.

Test Plan:
 1) Enable AllowHoldPolicyOverride
 2) Set up circ rules to allow for multiple item level holds on one record
 3) Place an item level hold on a record
 4) Note you can force placing a second item hold on that reocrd
 5) Attempt to do so, it will not actually work
 6) Apply this patch
 7) Note you can no longer place another item level hold for the same item
   you just placed an item-level hold on
 8) Note you can still force holds that contravene the circ rules for
   any and all other reasons
 9) Test with record level holds
10) Test by placing multiple holds from search results
Comment 8 Nick Clemens 2019-06-19 15:53:26 UTC
Created attachment 90802 [details] [review]
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

A library appears to be able to place a second item level hold on an item a patron already has on hold if
A) AllowHoldPolicyOverride is enabled
and
B) the circ rule allow for multple item level holds.

Once the patron submits the hold requests though, the hold does not get stored in the database.

Because allowing two item level holds for the same item makes no sense, we should not allow
this attempt to take place, even if AllowHoldPolicyOverride is enabled.

Test Plan:
 1) Enable AllowHoldPolicyOverride
 2) Set up circ rules to allow for multiple item level holds on one record
 3) Place an item level hold on a record
 4) Note you can force placing a second item hold on that reocrd
 5) Attempt to do so, it will not actually work
 6) Apply this patch
 7) Note you can no longer place another item level hold for the same item
   you just placed an item-level hold on
 8) Note you can still force holds that contravene the circ rules for
   any and all other reasons
 9) Test with record level holds
10) Test by placing multiple holds from search results

Signed-off-by: Martha Fuerst <mfurest@hmcpl.org>
Comment 9 Jonathan Druart 2019-06-19 17:38:41 UTC
Naive question reading the diff: should not we have that check in CanItemBeReserved?
Comment 10 Nick Clemens 2019-06-21 10:16:38 UTC
(In reply to Jonathan Druart from comment #9)
> Naive question reading the diff: should not we have that check in
> CanItemBeReserved?

Holds overrides are only possible from the staff interface and on this page so I believe this change is sufficient to fix the regression.

In the future this code could be improved and moved to modules for more testing, but this is a serious regression affecting many users and must be fixed.
Comment 11 Tomás Cohen Arazi 2019-06-24 18:56:38 UTC
Created attachment 90974 [details] [review]
Bug 23116: AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

A library appears to be able to place a second item level hold on an item a patron already has on hold if
A) AllowHoldPolicyOverride is enabled
and
B) the circ rule allow for multple item level holds.

Once the patron submits the hold requests though, the hold does not get stored in the database.

Because allowing two item level holds for the same item makes no sense, we should not allow
this attempt to take place, even if AllowHoldPolicyOverride is enabled.

Test Plan:
 1) Enable AllowHoldPolicyOverride
 2) Set up circ rules to allow for multiple item level holds on one record
 3) Place an item level hold on a record
 4) Note you can force placing a second item hold on that reocrd
 5) Attempt to do so, it will not actually work
 6) Apply this patch
 7) Note you can no longer place another item level hold for the same item
   you just placed an item-level hold on
 8) Note you can still force holds that contravene the circ rules for
   any and all other reasons
 9) Test with record level holds
10) Test by placing multiple holds from search results

Signed-off-by: Martha Fuerst <mfurest@hmcpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Simple fix for a regression. Works as expected.
Comment 12 Martin Renvoize 2019-06-25 16:02:13 UTC
Nice work!

Pushed to master for 19.11.00
Comment 13 Fridolin SOMERS 2019-07-23 12:47:45 UTC
Pushed to 19.05.x for 19.05.02
Comment 14 Lucas Gass 2019-07-23 14:51:34 UTC
backported to 18.11.x for 18.11.08