Bug 17327 - Item level holds no longer enforced
Summary: Item level holds no longer enforced
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 14695
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-21 21:35 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17327 - Add unit test for regression (1.53 KB, patch)
2016-09-21 21:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17327 - Item level holds no longer enforced (2.14 KB, patch)
2016-09-21 21:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17327 - Add unit test for regression (1.59 KB, patch)
2016-09-22 22:49 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17327 - Item level holds no longer enforced (2.21 KB, patch)
2016-09-22 22:49 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17327: Add unit test for regression (1.65 KB, patch)
2016-09-26 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17327: Item level holds no longer enforced (2.26 KB, patch)
2016-09-26 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17327: (QA followup) Remove Carp::Always which is not used (1.43 KB, patch)
2016-09-26 18:00 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 2016-09-21 21:35:45 UTC
Reported by Katrin:

Example:
- on shelf holds allowed
- holds allowed 5
- holds on same record allowed 5
- FORCE item level holds

On a record with only one item, you can only place a record level hold in OPAC now - which is against your circulation conditions.

Another example:
- record with 2 items
- circulation rules as above, 1 hold allowed on the record
- Item level holds: forced

I can only place record level holds in OPAC and staff. At the moment, I am not able to place item level holds in any circumstances in my installation.
Comment 1 Kyle M Hall 2016-09-21 21:41:00 UTC
Created attachment 55744 [details] [review]
Bug 17327 - Add unit test for regression
Comment 2 Kyle M Hall 2016-09-21 21:41:08 UTC
Created attachment 55745 [details] [review]
Bug 17327 - Item level holds no longer enforced

Reported by Katrin:

Example:
- on shelf holds allowed
- holds allowed 5
- holds on same record allowed 5
- FORCE item level holds

On a record with only one item, you can only place a record level hold
in OPAC now - which is against your circulation conditions.

Another example:
- record with 2 items
- circulation rules as above, 1 hold allowed on the record
- Item level holds: forced

I can only place record level holds in OPAC and staff. At the moment, I
am not able to place item level holds in any circumstances in my
installation.

Test Plan:
1) Apply the unit test patch
2) prove t/db_dependent/Reserves/MultiplePerRecord.t
   should fail
3) Apply the second patch
4) prove t/db_dependent/Reserves/MultiplePerRecord.t
   should pass
5) Attempt to replicate one of the examples above, you should be unable to
Comment 3 Benjamin Rokseth 2016-09-22 22:49:44 UTC
Created attachment 55761 [details] [review]
Bug 17327 - Add unit test for regression

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 4 Benjamin Rokseth 2016-09-22 22:49:52 UTC
Created attachment 55762 [details] [review]
Bug 17327 - Item level holds no longer enforced

Reported by Katrin:

Example:
- on shelf holds allowed
- holds allowed 5
- holds on same record allowed 5
- FORCE item level holds

On a record with only one item, you can only place a record level hold
in OPAC now - which is against your circulation conditions.

Another example:
- record with 2 items
- circulation rules as above, 1 hold allowed on the record
- Item level holds: forced

I can only place record level holds in OPAC and staff. At the moment, I
am not able to place item level holds in any circumstances in my
installation.

Test Plan:
1) Apply the unit test patch
2) prove t/db_dependent/Reserves/MultiplePerRecord.t
   should fail
3) Apply the second patch
4) prove t/db_dependent/Reserves/MultiplePerRecord.t
   should pass
5) Attempt to replicate one of the examples above, you should be unable to

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 5 Benjamin Rokseth 2016-09-22 22:54:29 UTC
Tests ok.
patron without holds -> both item and record level allowed
patron with record level hold -> only record hold allowed
patron with item level hold -> only item hold allowed

How item/record level holds should be enforced by circulation rules is probably the scope of another bug, as this needs to be pushed to master quickly
Comment 6 Tomás Cohen Arazi 2016-09-26 18:00:39 UTC
Created attachment 55825 [details] [review]
Bug 17327: Add unit test for regression

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2016-09-26 18:00:48 UTC
Created attachment 55826 [details] [review]
Bug 17327: Item level holds no longer enforced

Reported by Katrin:

Example:
- on shelf holds allowed
- holds allowed 5
- holds on same record allowed 5
- FORCE item level holds

On a record with only one item, you can only place a record level hold
in OPAC now - which is against your circulation conditions.

Another example:
- record with 2 items
- circulation rules as above, 1 hold allowed on the record
- Item level holds: forced

I can only place record level holds in OPAC and staff. At the moment, I
am not able to place item level holds in any circumstances in my
installation.

Test Plan:
1) Apply the unit test patch
2) prove t/db_dependent/Reserves/MultiplePerRecord.t
   should fail
3) Apply the second patch
4) prove t/db_dependent/Reserves/MultiplePerRecord.t
   should pass
5) Attempt to replicate one of the examples above, you should be unable to

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2016-09-26 18:00:56 UTC
Created attachment 55827 [details] [review]
Bug 17327: (QA followup) Remove Carp::Always which is not used

This patch removes the need for Carp::Always in .../Reserves/MultiplePerRecord.t
which is not actually used.

It also removes 'undef' from Koha::Holds::forced_hold_level's last return, to comply with
our QA rules.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Kyle M Hall 2016-09-26 18:19:05 UTC
Pushed to master for 16.11, thanks for the followup Tomas!