Bug 14695

Summary: Add ability to place multiple item holds on a given record per patron
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: Hold requestsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: abl, alex.arnaud, amy, arve.soreide, bdaeuber, benjamin.rokseth, black23, boutrosboutrosboutros, cgdavis, chris, clackman, dcook, edischer, fiona.borthwick, gmcharlt, gwilliams, jonathan.druart, josef.moravec, jpalmer, jsasse, jschmidt, julian.maurice, katrin.fischer, kholt, kyle, lisettepalouse+koha, m.de.rooy, marjorie.barry-vila, melia, mtj, nick, nicolas.legrand, nicole, rkuiper, sally.healey, techservspec, tmisilo
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11999
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15565
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23957
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15565, 15567, 16260, 17327, 17431, 17561, 17758, 17766, 17940, 18076, 18150, 19135, 20567    
Attachments: Bug 14695 - Tidy CanItemBeReserved and CanBookBeReserved
Bug 14695 - Add new issuing rule
Bug 14695 - - Add ability to place multiple item holds on a given record per patron
Bug 14695 - Tidy CanItemBeReserved and CanBookBeReserved
Bug 14695 - Add new issuing rule
Bug 14695 - - Add ability to place multiple item holds on a given record per patron
[SIGNED-OFF] Bug 14695 - Tidy CanItemBeReserved and CanBookBeReserved
[SIGNED-OFF] Bug 14695 - Add new issuing rule
[SIGNED-OFF] Bug 14695 - - Add ability to place multiple item holds on a given record per patron
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 [QA Followup] - Fix display issue for in transit holds
Bug 14695 [QA Followup] - Fix display issue for in transit holds
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked
Bug 14695 [QA Followup] - Tell librarian if item doesn't allow or forces item level holds in the OPAC
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked
Bug 14695 [QA Followup] - Tell librarian if item doesn't allow or forces item level holds in the OPAC
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked
Bug 14695 [QA Followup] - Tell librarian if item doesn't allow or forces item level holds in the OPAC
Bug 14695 [QA Followup] - Unit Tests
Bug 14695 - Update database
Bug 14695 - Update DB Schema file
Bug 14695 - Add new circulation rule
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved
Bug 14695 - Add ability to place multiple item holds on a given record per patron
Bug 14695 [QA Followup] - Filling one hold fills all of them
Bug 14695 [QA Followup] - Make existing unit tests pass
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
Bug 14695 - Warn librarian if max_holds_per_record is reached
Bug 14695 - Don't use holds_to_place_count when checking an item.
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'
Bug 14695: Fix typo in template
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked
Bug 14695 [QA Followup] - Tell librarian if item doesn't allow or forces item level holds in the OPAC
Bug 14695 [QA Followup] - Unit Tests
Bug 14695 [QA Followup] - Fix issues found by QA script
Bug 14695: Fix GetHardDueDate.t tests
Bug 14695 [QA Followup]
screenshot of my circulation rule
'place hold' screen in staff

Description Kyle M Hall 2015-08-19 14:51:10 UTC
Currently Koha only allows one hold to be placed on a given bib per patron, even it the record has multiple items. This enhancement will give Koha the ability to place a hold on at least two items on the same bibliographic record. For example, in the case of a television season DVD which has one record but many unique items.

Work to be done:
1) Locate all code in Koha that checks for a patron already having a hold on a record
2) Add a new system preference to define the number of holds allowed per record
3) Create new subroutine/method to check for this
4) Implement call to new sub in areas defined in part 1
5) Ensure no code in C4::Reserves assumes that it can modify holds based on borrowernumber only

This is not the first attempt at such a feature, but previous iterations are far out of date. It's time for a fresh start.
Comment 1 Kyle M Hall 2015-10-29 11:07:40 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-10-29 11:07:48 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-10-29 11:07:51 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-11-20 16:04:32 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2015-11-20 16:04:41 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-11-20 16:04:44 UTC Comment hidden (obsolete)
Comment 7 Nicole C. Engard 2015-11-20 16:54:01 UTC Comment hidden (obsolete)
Comment 8 Nicole C. Engard 2015-11-20 16:54:15 UTC Comment hidden (obsolete)
Comment 9 Nicole C. Engard 2015-11-20 16:54:24 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens 2015-11-20 17:21:13 UTC
Checking out 1 item on hold for the patron resolved all the other waiting holds  on that record
Comment 11 Kyle M Hall 2016-01-12 16:21:44 UTC
*** Bug 7710 has been marked as a duplicate of this bug. ***
Comment 12 Kyle M Hall 2016-01-12 16:24:33 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2016-01-12 16:24:40 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2016-01-12 16:24:43 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2016-01-12 16:24:45 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2016-01-12 16:24:48 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2016-01-12 16:30:49 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2016-01-12 16:30:57 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2016-01-12 16:30:59 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2016-01-12 16:31:03 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2016-01-12 16:31:06 UTC Comment hidden (obsolete)
Comment 22 Nick Clemens 2016-01-12 16:56:16 UTC
Same issue as before

Place 3 item specific holds
Checkout 1 item
All holds are resolved
Comment 23 Kyle M Hall 2016-01-19 17:50:39 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2016-01-19 17:50:46 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2016-01-19 17:50:49 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2016-01-19 17:50:52 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2016-01-19 17:50:55 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2016-01-19 17:50:59 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2016-01-19 17:51:02 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2016-01-19 20:52:35 UTC
Hi Kyle, can you please check status and patches? Patches are missing a sign-off or status is wrong :)
Comment 31 Kyle M Hall 2016-01-20 11:27:09 UTC
(In reply to Katrin Fischer from comment #30)
> Hi Kyle, can you please check status and patches? Patches are missing a
> sign-off or status is wrong :)

Indeed, my bad!
Comment 32 Nick Clemens 2016-01-22 13:37:43 UTC
Still losing all holds when checking 1 item out

Also got and odd mix of waiting/transfer once one hold is confirmed

Staff side was able to place one item level hold and then 2 record level holds
Comment 33 Kyle M Hall 2016-01-22 14:31:41 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2016-01-22 14:31:48 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-01-22 14:31:51 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-01-22 14:31:54 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2016-01-22 14:31:58 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2016-01-22 14:32:01 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2016-01-22 14:32:04 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2016-01-22 15:53:41 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2016-01-22 15:53:49 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2016-01-22 16:17:16 UTC Comment hidden (obsolete)
Comment 43 Nick Clemens 2016-01-22 22:29:36 UTC Comment hidden (obsolete)
Comment 44 Nick Clemens 2016-01-22 22:29:40 UTC Comment hidden (obsolete)
Comment 45 Nick Clemens 2016-01-22 22:29:43 UTC Comment hidden (obsolete)
Comment 46 Nick Clemens 2016-01-22 22:29:46 UTC Comment hidden (obsolete)
Comment 47 Nick Clemens 2016-01-22 22:29:50 UTC Comment hidden (obsolete)
Comment 48 Nick Clemens 2016-01-22 22:29:53 UTC Comment hidden (obsolete)
Comment 49 Nick Clemens 2016-01-22 22:29:56 UTC Comment hidden (obsolete)
Comment 50 Nick Clemens 2016-01-22 22:30:00 UTC Comment hidden (obsolete)
Comment 51 Nick Clemens 2016-01-22 22:30:55 UTC
Internal sign off only, needs community sign off
Comment 52 Alex Arnaud 2016-01-26 14:09:23 UTC
Hello Kyle,

Ok on the staff side (hold 3 times the same biblio with the same patron) but the code doesn't take in account reserves via ILSDI.

I got the code "NotHoldable" for the second hold.
Comment 53 Alex Arnaud 2016-01-26 15:48:42 UTC
In fact, it doesn't work either on the staff side. I have 3 holds per record on my circulation rules and i can place as many record-level holds as i want (in the limit of maxreserves syspref) on the same record for the same patron.

I have the same behavior with item-level holds and i can hold many times the same item.
Comment 54 Alex Arnaud 2016-01-27 15:48:29 UTC Comment hidden (obsolete)
Comment 55 Alex Arnaud 2016-01-27 16:00:22 UTC
The last patch is a proposed solution to warn librarian if placing hold when max_holds_per_record is reached.

Sorry for my comment about ILSDI: It works well. And i think we should keep the ability to hold the same item several time insofar as we could ask it at different dates.
Comment 56 Alex Arnaud 2016-01-28 09:46:23 UTC Comment hidden (obsolete)
Comment 57 Owen Leonard 2016-02-02 13:23:06 UTC
Applying: Bug 14695 - Warn librarian if max_holds_per_record is reached
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 14695 - Warn librarian if max_holds_per_record is reached
Comment 58 Kyle M Hall 2016-02-03 14:30:53 UTC
(In reply to Owen Leonard from comment #57)
> Applying: Bug 14695 - Warn librarian if max_holds_per_record is reached
> fatal: sha1 information is lacking or useless
> (koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 14695 - Warn librarian if max_holds_per_record is
> reached

I had no problem applying this patch set to master!
Comment 59 Alex Arnaud 2016-02-03 16:35:40 UTC Comment hidden (obsolete)
Comment 60 Alex Arnaud 2016-02-03 16:35:49 UTC Comment hidden (obsolete)
Comment 61 Alex Arnaud 2016-02-03 16:35:57 UTC Comment hidden (obsolete)
Comment 62 Alex Arnaud 2016-02-03 16:36:05 UTC Comment hidden (obsolete)
Comment 63 Alex Arnaud 2016-02-03 16:36:13 UTC Comment hidden (obsolete)
Comment 64 Alex Arnaud 2016-02-03 16:36:19 UTC Comment hidden (obsolete)
Comment 65 Alex Arnaud 2016-02-03 16:36:27 UTC Comment hidden (obsolete)
Comment 66 Alex Arnaud 2016-02-03 16:36:33 UTC Comment hidden (obsolete)
Comment 67 Alex Arnaud 2016-02-03 16:36:40 UTC Comment hidden (obsolete)
Comment 68 Alex Arnaud 2016-02-03 16:36:47 UTC Comment hidden (obsolete)
Comment 69 Benjamin Daeuber 2016-02-12 16:57:36 UTC
We ran this through the sandbox and adjusted the test plan as follows:

a. We should probably update the test plan to make sure we’re all in the same place:
	1. Patron class needs at least the same number of holds allowed as you are going to allow per record
	2. Item level holds need to be enabled on the item type
b. Another note -- and this would vary by library – we adjusted the default for holds on available items from “waiting” to “on hold” so they were not active until they were checked in

That said, here’s what we noted:

1. If you start in the staff interface a patron can place holds on more than 3 items on a bib record, regardless of circ rules. But the behavior is kind of strange. If holds are placed from the staff interface, it lets us place 4 holds. When a patron places holds from the OPAC it stops at 3, and then prevents staff from placing additional holds in the staff interface.

2. Selecting an option from the drop down while “next available” is checked places multiple holds on the selected item.

3. Related, the dropdown could cause policy enforcement issues for some libraries. Say we allow users to put 10 items per record on hold (a realistic number if you consider periodicals records that may have hundreds of items) and a user selects it. If there is only 1 item, a patron could place it on hold 10 times and keep checking it out, bypassing renewal restrictions.

Some comments on the user experience

1. When you look at the holds queue, it’s not quite as expected. Say that a Patron A puts 3 items on a bib record on hold, then Patron B comes along and places 2 of the same items on that bib record on hold. Patron B will show up 4th and 5th in the hold queue, despite being 2nd for two different items.

2. I’m not sure that the drop down is necessary. If a customer wants to place holds, the natural desire would be to either have checkboxes for each individual item and a single hold button or individual hold buttons for each item that would trigger a javascript confirmation rather than a second screen

3. Related, the user has to go back into the record for each item they want to place on hold. This is cumbersome.

4. The actual workflow works more or less as expected. One issue would be when checking out a second item on a bib record to a patron, there is a box that asks to confirm checkout. I think we should have the option to suppress this.
Comment 70 Julian Maurice 2016-02-29 08:32:32 UTC
Patches don't apply on master

[...]
Applying: Bug 14695 - Add ability to place multiple item holds on a given record per patron
Using index info to reconstruct a base tree...
M       C4/Reserves.pm
M       Koha/Holds.pm
M       koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
M       opac/opac-reserve.pl
M       reserve/request.pl
Falling back to patching base and 3-way merge...
Auto-merging reserve/request.pl
Auto-merging opac/opac-reserve.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
Auto-merging Koha/Holds.pm
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 14695 - Add ability to place multiple item holds on a given record per patron
Comment 71 Kyle M Hall 2016-03-08 16:08:13 UTC Comment hidden (obsolete)
Comment 72 Kyle M Hall 2016-03-08 16:08:32 UTC Comment hidden (obsolete)
Comment 73 Kyle M Hall 2016-03-08 16:08:36 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2016-03-08 16:08:40 UTC Comment hidden (obsolete)
Comment 75 Kyle M Hall 2016-03-08 16:08:44 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2016-03-08 16:08:48 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2016-03-08 16:08:53 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2016-03-08 16:08:57 UTC Comment hidden (obsolete)
Comment 79 Kyle M Hall 2016-03-08 16:09:01 UTC Comment hidden (obsolete)
Comment 80 Kyle M Hall 2016-03-08 16:09:05 UTC Comment hidden (obsolete)
Comment 81 Michal Denar 2016-03-17 12:35:22 UTC
Could You rebase?
Comment 82 Benjamin Rokseth 2016-04-08 10:26:22 UTC Comment hidden (obsolete)
Comment 83 Benjamin Rokseth 2016-04-08 10:26:31 UTC Comment hidden (obsolete)
Comment 84 Benjamin Rokseth 2016-04-08 10:26:40 UTC Comment hidden (obsolete)
Comment 85 Benjamin Rokseth 2016-04-08 10:26:47 UTC Comment hidden (obsolete)
Comment 86 Benjamin Rokseth 2016-04-08 10:26:53 UTC Comment hidden (obsolete)
Comment 87 Benjamin Rokseth 2016-04-08 10:27:00 UTC Comment hidden (obsolete)
Comment 88 Benjamin Rokseth 2016-04-08 10:27:08 UTC Comment hidden (obsolete)
Comment 89 Benjamin Rokseth 2016-04-08 10:27:15 UTC Comment hidden (obsolete)
Comment 90 Benjamin Rokseth 2016-04-08 10:27:21 UTC Comment hidden (obsolete)
Comment 91 Benjamin Rokseth 2016-04-08 10:27:28 UTC Comment hidden (obsolete)
Comment 92 Benjamin Rokseth 2016-04-08 10:29:21 UTC
Rebased to fix t::lib::Mocks::mock_preference conflict

Getting failed test though:

ok 65 - Bug 14464 - Fine applied after cancelling reserve with charge desired and configured
DBD::mysql::st execute failed: Duplicate entry '21' for key 'PRIMARY' [for Statement "INSERT INTO `reserves` ( `biblionumber`, `borrowernumber`, `branchcode`, `cancellationdate`, `expirationdate`, `found`, `itemnumber`, `lowestPriority`, `notificationdate`, `priority`, `reminderdate`, `reserve_id`, `reservedate`, `reservenotes`, `suspend`, `suspend_until`, `timestamp`, `waitingdate`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='1', 1='1', 2='CPL', 3=undef, 4=undef, 5='F', 6=undef, 7='0', 8=undef, 9=0, 10=undef, 11='21', 12='2016-04-08', 13='', 14='0', 15=undef, 16='2016-04-08 10:12:53', 17=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBIx::Class::Storage::DBI::_execute(): DBD::mysql::st execute failed: Duplicate entry '21' for key 'PRIMARY' [for Statement "INSERT INTO `reserves` ( `biblionumber`, `borrowernumber`, `branchcode`, `cancellationdate`, `expirationdate`, `found`, `itemnumber`, `lowestPriority`, `notificationdate`, `priority`, `reminderdate`, `reserve_id`, `reservedate`, `reservenotes`, `suspend`, `suspend_until`, `timestamp`, `waitingdate`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='1', 1='1', 2='CPL', 3=undef, 4=undef, 5='F', 6=undef, 7='0', 8=undef, 9=0, 10=undef, 11='21', 12='2016-04-08', 13='', 14='0', 15=undef, 16='2016-04-08 10:12:53', 17=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. at Koha/Object.pm line 109
# Looks like you planned 71 tests but ran 65.
Comment 93 Benjamin Rokseth 2016-04-08 12:07:21 UTC
Seems checkout breaks after:
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
trying to update/insert duplicate reserve_ids

DBD::mysql::st execute failed: Duplicate entry '21' for key 'PRIMARY [for Statement "INSERT INTO `reserves` 

checking out only until:
Bug 14695 [QA Followup] - Make existing unit tests pass

makes tests pass and checkout work again
Comment 94 Kyle M Hall 2016-04-18 14:18:51 UTC Comment hidden (obsolete)
Comment 95 Kyle M Hall 2016-04-18 14:20:50 UTC
(In reply to Benjamin Rokseth from comment #93)
> Seems checkout breaks after:
> Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout
> trying to update/insert duplicate reserve_ids
> 
> DBD::mysql::st execute failed: Duplicate entry '21' for key 'PRIMARY [for
> Statement "INSERT INTO `reserves` 
> 
> checking out only until:
> Bug 14695 [QA Followup] - Make existing unit tests pass
> 
> makes tests pass and checkout work again

The patches need only be tested all together. This latest patch should fix the unit test issue!
Comment 96 Julian Maurice 2016-04-19 10:07:06 UTC Comment hidden (obsolete)
Comment 97 Benjamin Rokseth 2016-04-19 13:41:46 UTC
Works as advertised, though one thing I notice:

/cgi-bin/koha/circ/pendingreserves.pl

reports only 1 for "Pull this many items" even though 3 holds are placed on the same biblio. When checking in one item, the number for "Pull this many items" is still 1, and 2 holds show on biblio details.

Is that a different bug?
Comment 98 Owen Leonard 2016-04-19 14:07:41 UTC
It doesn't look like any of these patches have a test plan.
Comment 99 Owen Leonard 2016-04-19 14:40:29 UTC
I don't see any changes to opac-reserve.tt. How can one place holds on multiple items if the form in the OPAC hasn't been updated to allow for multiple selections?

Also, I get an error in the staff client:

1. Search for a title to place a hold on.
2. Click the "Place hold" link in search results.
3. Submit a patron barcode to place the hold for.

Error: Template process failed: undef error - WHILE loop terminated (> 1000 iterations)
Comment 100 Kyle M Hall 2016-04-19 15:36:55 UTC
Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Edit your circulation rules and set "Holds per record (count)" to 3
4) Find or create a record with 4 items
5) Place a record level hold
6) Place another hold for the same patron, note you can now only place more record level holds
7) Delete those holds
8) Place an item level hold
9) Place another hold for the same patron, note you can now only place more item level holds and you cannot place a hold on an item you've already placed a hold on
10) Place a third item hold
11) Attempt to place a fourth hold, not that you cannot
12) Fill holds, note that the correct hold is filled.
Comment 101 Kyle M Hall 2016-04-19 15:40:04 UTC
13) Repeat steps 5-11 on the opac
Comment 102 Kyle M Hall 2016-04-19 15:44:55 UTC
(In reply to Owen Leonard from comment #99)
> I don't see any changes to opac-reserve.tt. How can one place holds on
> multiple items if the form in the OPAC hasn't been updated to allow for
> multiple selections?

No changes to opac-reserve.tt were needed. The changes in the Reserves module percolate down to the template!

> 
> Also, I get an error in the staff client:
> 
> 1. Search for a title to place a hold on.
> 2. Click the "Place hold" link in search results.
> 3. Submit a patron barcode to place the hold for.
> 
> Error: Template process failed: undef error - WHILE loop terminated (> 1000
> iterations)

I have been completely unable to recreate this error. Does it happen for you on master as well?
Comment 103 Owen Leonard 2016-04-19 15:58:53 UTC
(In reply to Kyle M Hall from comment #102)

> No changes to opac-reserve.tt were needed. The changes in the Reserves
> module percolate down to the template!

So the feature allows you to place a hold on multiple items, but not at once?
Comment 104 Kyle M Hall 2016-04-19 16:03:18 UTC
(In reply to Owen Leonard from comment #103)
> (In reply to Kyle M Hall from comment #102)
> 
> > No changes to opac-reserve.tt were needed. The changes in the Reserves
> > module percolate down to the template!
> 
> So the feature allows you to place a hold on multiple items, but not at once?

That is correct. I'd love to add that, but this is the first step towards that goal. Once this is in, I'll be working on that.
Comment 105 Kyle M Hall 2016-04-29 15:03:22 UTC Comment hidden (obsolete)
Comment 106 Kyle M Hall 2016-04-29 15:03:34 UTC Comment hidden (obsolete)
Comment 107 Kyle M Hall 2016-04-29 15:03:39 UTC Comment hidden (obsolete)
Comment 108 Kyle M Hall 2016-04-29 15:03:43 UTC Comment hidden (obsolete)
Comment 109 Kyle M Hall 2016-04-29 15:03:48 UTC Comment hidden (obsolete)
Comment 110 Kyle M Hall 2016-04-29 15:03:53 UTC Comment hidden (obsolete)
Comment 111 Kyle M Hall 2016-04-29 15:03:58 UTC Comment hidden (obsolete)
Comment 112 Kyle M Hall 2016-04-29 15:04:02 UTC Comment hidden (obsolete)
Comment 113 Kyle M Hall 2016-04-29 15:04:07 UTC Comment hidden (obsolete)
Comment 114 Kyle M Hall 2016-04-29 15:04:12 UTC Comment hidden (obsolete)
Comment 115 Kyle M Hall 2016-04-29 15:04:16 UTC Comment hidden (obsolete)
Comment 116 Kyle M Hall 2016-04-29 15:04:21 UTC Comment hidden (obsolete)
Comment 117 Kyle M Hall 2016-04-29 17:35:38 UTC Comment hidden (obsolete)
Comment 118 Nicole C. Engard 2016-04-29 17:41:07 UTC Comment hidden (obsolete)
Comment 119 Nicole C. Engard 2016-04-29 17:41:15 UTC Comment hidden (obsolete)
Comment 120 Nicole C. Engard 2016-04-29 17:41:25 UTC Comment hidden (obsolete)
Comment 121 Nicole C. Engard 2016-04-29 17:41:31 UTC Comment hidden (obsolete)
Comment 122 Nicole C. Engard 2016-04-29 17:41:44 UTC Comment hidden (obsolete)
Comment 123 Nicole C. Engard 2016-04-29 17:41:51 UTC Comment hidden (obsolete)
Comment 124 Nicole C. Engard 2016-04-29 17:41:58 UTC Comment hidden (obsolete)
Comment 125 Nicole C. Engard 2016-04-29 17:42:11 UTC Comment hidden (obsolete)
Comment 126 Nicole C. Engard 2016-04-29 17:42:23 UTC Comment hidden (obsolete)
Comment 127 Nicole C. Engard 2016-04-29 17:42:31 UTC Comment hidden (obsolete)
Comment 128 Nicole C. Engard 2016-04-29 17:42:38 UTC Comment hidden (obsolete)
Comment 129 Nicole C. Engard 2016-04-29 17:42:47 UTC Comment hidden (obsolete)
Comment 130 Nicole C. Engard 2016-04-29 17:42:57 UTC Comment hidden (obsolete)
Comment 131 Benjamin Daeuber 2016-04-30 22:17:08 UTC
Quick feedback on a few quirks. I don't know if any of these would prevent closing this and pushing it to master, but nonetheless:

In Circ Client

1.	It appears staff can place item level holds when this feature is on, regardless of whether item level holds are allowed. We tested on Wednesday, and it appears Nicole has already identified this.

2.	The holds to place count (up to three) is set to 3. Select one barcode, set “holds to place” dropdown to 3, and it only does one hold on the item. This seems to be unexpected, but we don’t really know why that dropdown box is there or what it is for.

3.	Put three items on hold on one record. Once you check in one of the three items you placed on hold, and it is now “waiting” for the patron, you can place the fourth hold. You don’t need to check it out to the patron before being able to place the fourth hold. Is this expected behavior?

In the OPAC

1.	Related to point 1 above, this does not appear to affect the OPAC. Users are blocked from placing item level holds if they are not allowed.

In both

1.	Item level holds seem to be disregarded by the hold limit. So if you have a limit of 10, with 9 items on hold, you can still place 2 item level holds. We haven’t tested this in more detail.

2.	A patron can’t place a bib level and item level hold for the same item. I’m actually struggling to think what scenario this might be used in, but we’re nothing if not thorough.
Comment 132 Kyle M Hall 2016-05-02 10:59:07 UTC
(In reply to Benjamin Daeuber from comment #131)
> Quick feedback on a few quirks. I don't know if any of these would prevent
> closing this and pushing it to master, but nonetheless:
> 
> In Circ Client
> 
> 1.	It appears staff can place item level holds when this feature is on,
> regardless of whether item level holds are allowed. We tested on Wednesday,
> and it appears Nicole has already identified this.

I'll look into this!

> 
> 2.	The holds to place count (up to three) is set to 3. Select one barcode,
> set “holds to place” dropdown to 3, and it only does one hold on the item.
> This seems to be unexpected, but we don’t really know why that dropdown box
> is there or what it is for.

The "Holds to place" does not do anything when placing item specific holds. The last patch in this series should hide that pulldown when the record-level hold checkbox is unchecked.

> 
> 3.	Put three items on hold on one record. Once you check in one of the three
> items you placed on hold, and it is now “waiting” for the patron, you can
> place the fourth hold. You don’t need to check it out to the patron before
> being able to place the fourth hold. Is this expected behavior?

Yes. That is standard Koha behavior.

> 
> In the OPAC
> 
> 1.	Related to point 1 above, this does not appear to affect the OPAC. Users
> are blocked from placing item level holds if they are not allowed.
> 
> In both
> 
> 1.	Item level holds seem to be disregarded by the hold limit. So if you have
> a limit of 10, with 9 items on hold, you can still place 2 item level holds.
> We haven’t tested this in more detail.

This is also standard Koha behavior. If you want more accurate hold limits, you need to add a record level itemtype to each record so that Koha will have a fallback itemtype to check if the hold is a record level hold and not item level.

> 
> 2.	A patron can’t place a bib level and item level hold for the same item.
> I’m actually struggling to think what scenario this might be used in, but
> we’re nothing if not thorough.

That is also intentional and expected behavior.
Comment 133 Benjamin Rokseth 2016-05-06 10:44:59 UTC Comment hidden (obsolete)
Comment 134 Benjamin Rokseth 2016-05-06 10:45:10 UTC Comment hidden (obsolete)
Comment 135 Benjamin Rokseth 2016-05-06 10:45:18 UTC Comment hidden (obsolete)
Comment 136 Benjamin Rokseth 2016-05-06 10:45:28 UTC Comment hidden (obsolete)
Comment 137 Benjamin Rokseth 2016-05-06 10:45:36 UTC Comment hidden (obsolete)
Comment 138 Benjamin Rokseth 2016-05-06 10:45:46 UTC Comment hidden (obsolete)
Comment 139 Benjamin Rokseth 2016-05-06 10:45:55 UTC Comment hidden (obsolete)
Comment 140 Benjamin Rokseth 2016-05-06 10:46:03 UTC Comment hidden (obsolete)
Comment 141 Benjamin Rokseth 2016-05-06 10:46:12 UTC Comment hidden (obsolete)
Comment 142 Benjamin Rokseth 2016-05-06 10:46:19 UTC Comment hidden (obsolete)
Comment 143 Benjamin Rokseth 2016-05-06 10:46:27 UTC Comment hidden (obsolete)
Comment 144 Benjamin Rokseth 2016-05-06 10:46:34 UTC Comment hidden (obsolete)
Comment 145 Benjamin Rokseth 2016-05-06 10:46:42 UTC Comment hidden (obsolete)
Comment 146 Jonathan Druart 2016-05-06 14:24:28 UTC
(In reply to Kyle M Hall from comment #132)
> (In reply to Benjamin Daeuber from comment #131)
> > 1.	It appears staff can place item level holds when this feature is on,
> > regardless of whether item level holds are allowed. We tested on Wednesday,
> > and it appears Nicole has already identified this.
> 
> I'll look into this!

What about this point?
Comment 147 Kyle M Hall 2016-06-07 16:20:05 UTC
(In reply to Jonathan Druart from comment #146)
> (In reply to Kyle M Hall from comment #132)
> > (In reply to Benjamin Daeuber from comment #131)
> > > 1.	It appears staff can place item level holds when this feature is on,
> > > regardless of whether item level holds are allowed. We tested on Wednesday,
> > > and it appears Nicole has already identified this.
> > 
> > I'll look into this!
> 
> What about this point?

I had not considered that a bug. If you do let me know and I'll change that behavior.
Comment 148 Jonathan Druart 2016-06-08 14:53:06 UTC
Kyle,
Is this feature ready? I mean, are all concerns raised for the last 4 years taken into account in this patch set?
If so, it would be great to have some of the people involved in the previous discussion in the signoff process.

Note that this patch set is not ready to be pushed as it, there is a big lack of tests (for new modules, new subroutines/methods and modified subroutines).
Comment 149 Jonathan Druart 2016-06-08 14:54:16 UTC
And it would have been easier not to tidy stuffs in the same patch set, it does not ease the readability.
Comment 150 Kyle M Hall 2016-06-08 15:07:58 UTC
(In reply to Jonathan Druart from comment #148)
> Kyle,
> Is this feature ready? I mean, are all concerns raised for the last 4 years
> taken into account in this patch set?

I cannot guarantee that but I think so! The problem with the last 3 (?) attempts has been that so many people want so many things. We need to work to limit scope creep. We need to take an iterative approach if more features are wanted.

> If so, it would be great to have some of the people involved in the previous
> discussion in the signoff process.

Absolutely! This patch set has been available for over half a year now. I expect anyone still interested in the feature has seen it at this point, but if you want to ping anyone in particular, please do!

> Note that this patch set is not ready to be pushed as it, there is a big
> lack of tests (for new modules, new subroutines/methods and modified
> subroutines).

Agreed, I'll take a look and see what I can do about that!
Comment 151 Benjamin Daeuber 2016-06-08 18:58:20 UTC
> I had not considered that a bug. If you do let me know and I'll change that
> behavior.

I would argue that at the very least it's inconsistent. I can't seem to spin up a sandbox right now and I'm not going to clone this branch to look, but if I recall correctly this is global system preference? If I allow item level holds for certain item types and then have a global preference override that, it may be a problem. Perhaps if it prompted the staff person first?

That said, this isn't really a serious problem from our perspective.

And I also agree with Kyle on iterative approach. There are definitely different things we want here (mostly on the UI side), but this is feature that seems to have been knocking around for a long time. Any step forward is good.
Comment 152 Benjamin Daeuber 2016-06-08 20:07:40 UTC
(In reply to Benjamin Daeuber from comment #151)
> > I had not considered that a bug. If you do let me know and I'll change that
> > behavior.
> 
> If I allow item
> level holds for certain item types and then have a global preference
> override that, it may be a problem.

If I *disallowed* item level holds for a certain item type that should say.
Comment 153 Kyle M Hall 2016-06-14 15:59:22 UTC
(In reply to Benjamin Daeuber from comment #152)
> (In reply to Benjamin Daeuber from comment #151)
> > > I had not considered that a bug. If you do let me know and I'll change that
> > > behavior.
> > 
> > If I allow item
> > level holds for certain item types and then have a global preference
> > override that, it may be a problem.
> 
> If I *disallowed* item level holds for a certain item type that should say.

Yes, I think I need to deal with that in the code. Setting to Failed QA for now.
Comment 154 Kyle M Hall 2016-06-16 11:55:06 UTC Comment hidden (obsolete)
Comment 155 Kyle M Hall 2016-06-16 11:59:33 UTC
(In reply to Benjamin Daeuber from comment #152)
> (In reply to Benjamin Daeuber from comment #151)
> > > I had not considered that a bug. If you do let me know and I'll change that
> > > behavior.
> > 
> > If I allow item
> > level holds for certain item types and then have a global preference
> > override that, it may be a problem.
> 
> If I *disallowed* item level holds for a certain item type that should say.

So, what I had forgotten and had to remind myself was those rules are not meant for the staff side, only for the opac. It has nothing to do with this enhancement. However, I've added a small followup that lets the librarian know if a given item on a record would be forced to be an item level hold or would be disallowed as an item level hold! It seems like a good datum for the librarian to know.
Comment 156 Benjamin Daeuber 2016-06-16 21:06:19 UTC
(In reply to Kyle M Hall from comment #155)
> (In reply to Benjamin Daeuber from comment #152)
> > (In reply to Benjamin Daeuber from comment #151)
> > > > I had not considered that a bug. If you do let me know and I'll change that
> > > > behavior.
> > > 
> > > If I allow item
> > > level holds for certain item types and then have a global preference
> > > override that, it may be a problem.
> > 
> > If I *disallowed* item level holds for a certain item type that should say.
> 
> So, what I had forgotten and had to remind myself was those rules are not
> meant for the staff side, only for the opac. It has nothing to do with this
> enhancement. However, I've added a small followup that lets the librarian
> know if a given item on a record would be forced to be an item level hold or
> would be disallowed as an item level hold! It seems like a good datum for
> the librarian to know.

I see, Kyle. I was tripped up because the "Item Level Holds" dropdown is *not* enforced on the staff side, but the "Holds Per Record (count)" *is* enforced on the staff side.

Either way, I do see the message now. I was thinking something more along the line of the yellow message box and an "Override" button after making the attempt, but I am of a mind with you and don't want to drag this farther. Do you feel that should be addressed later with another bug ticket?
Comment 157 Kyle M Hall 2016-06-27 15:26:51 UTC
(In reply to Benjamin Daeuber from comment #156)
> Either way, I do see the message now. I was thinking something more along
> the line of the yellow message box and an "Override" button after making the
> attempt, but I am of a mind with you and don't want to drag this farther. Do
> you feel that should be addressed later with another bug ticket?

Yes, I'd be more than happy to work on that in a followup bug! If you want to create the bug yourself please go ahead and file the report which you can set to depending on this bug.
Comment 158 Katrin Fischer 2016-08-10 17:37:22 UTC
*** Bug 2532 has been marked as a duplicate of this bug. ***
Comment 159 Kyle M Hall 2016-08-17 17:21:34 UTC Comment hidden (obsolete)
Comment 160 Kyle M Hall 2016-08-17 17:21:47 UTC Comment hidden (obsolete)
Comment 161 Kyle M Hall 2016-08-17 17:21:52 UTC Comment hidden (obsolete)
Comment 162 Kyle M Hall 2016-08-17 17:21:58 UTC Comment hidden (obsolete)
Comment 163 Kyle M Hall 2016-08-17 17:22:04 UTC Comment hidden (obsolete)
Comment 164 Kyle M Hall 2016-08-17 17:22:10 UTC Comment hidden (obsolete)
Comment 165 Kyle M Hall 2016-08-17 17:22:16 UTC Comment hidden (obsolete)
Comment 166 Kyle M Hall 2016-08-17 17:22:22 UTC Comment hidden (obsolete)
Comment 167 Kyle M Hall 2016-08-17 17:22:28 UTC Comment hidden (obsolete)
Comment 168 Kyle M Hall 2016-08-17 17:22:33 UTC Comment hidden (obsolete)
Comment 169 Kyle M Hall 2016-08-17 17:22:39 UTC Comment hidden (obsolete)
Comment 170 Kyle M Hall 2016-08-17 17:22:45 UTC Comment hidden (obsolete)
Comment 171 Kyle M Hall 2016-08-17 17:22:51 UTC Comment hidden (obsolete)
Comment 172 Kyle M Hall 2016-08-17 17:22:57 UTC Comment hidden (obsolete)
Comment 173 Jonathan Druart 2016-08-25 08:05:58 UTC
(In reply to Kyle M Hall from comment #150)
> > Note that this patch set is not ready to be pushed as it, there is a big
> > lack of tests (for new modules, new subroutines/methods and modified
> > subroutines).
> 
> Agreed, I'll take a look and see what I can do about that!

That is still valid.
Comment 174 Kyle M Hall 2016-09-02 13:12:00 UTC Comment hidden (obsolete)
Comment 175 Kyle M Hall 2016-09-02 13:12:54 UTC Comment hidden (obsolete)
Comment 176 Kyle M Hall 2016-09-02 13:13:00 UTC Comment hidden (obsolete)
Comment 177 Kyle M Hall 2016-09-02 13:13:06 UTC Comment hidden (obsolete)
Comment 178 Kyle M Hall 2016-09-02 13:13:11 UTC Comment hidden (obsolete)
Comment 179 Kyle M Hall 2016-09-02 13:13:17 UTC Comment hidden (obsolete)
Comment 180 Kyle M Hall 2016-09-02 13:13:23 UTC Comment hidden (obsolete)
Comment 181 Kyle M Hall 2016-09-02 13:13:29 UTC Comment hidden (obsolete)
Comment 182 Kyle M Hall 2016-09-02 13:13:35 UTC Comment hidden (obsolete)
Comment 183 Kyle M Hall 2016-09-02 13:13:41 UTC Comment hidden (obsolete)
Comment 184 Kyle M Hall 2016-09-02 13:13:47 UTC Comment hidden (obsolete)
Comment 185 Kyle M Hall 2016-09-02 13:13:53 UTC Comment hidden (obsolete)
Comment 186 Kyle M Hall 2016-09-02 13:13:59 UTC Comment hidden (obsolete)
Comment 187 Kyle M Hall 2016-09-02 13:14:05 UTC Comment hidden (obsolete)
Comment 188 Kyle M Hall 2016-09-02 13:14:12 UTC Comment hidden (obsolete)
Comment 189 Kyle M Hall 2016-09-03 01:30:09 UTC
Created attachment 55148 [details] [review]
Bug 14695 - Update database

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 190 Kyle M Hall 2016-09-03 01:30:24 UTC
Created attachment 55149 [details] [review]
Bug 14695 - Update DB Schema file

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 191 Kyle M Hall 2016-09-03 01:30:31 UTC
Created attachment 55150 [details] [review]
Bug 14695 - Add new circulation rule

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 192 Kyle M Hall 2016-09-03 01:30:37 UTC
Created attachment 55151 [details] [review]
Bug 14695 - Tidy C4::Reserves::CanItemBeReserved

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 193 Kyle M Hall 2016-09-03 01:30:43 UTC
Created attachment 55152 [details] [review]
Bug 14695 - Add ability to place multiple item holds on a given record per patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 194 Kyle M Hall 2016-09-03 01:30:50 UTC
Created attachment 55153 [details] [review]
Bug 14695 [QA Followup] - Filling one hold fills all of them

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 195 Kyle M Hall 2016-09-03 01:30:57 UTC
Created attachment 55154 [details] [review]
Bug 14695 [QA Followup] - Make existing unit tests pass

* Removes tests no longer needed
* Updates rules to work with existing tests
* Fixes code issues revealed by unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 196 Kyle M Hall 2016-09-03 01:31:04 UTC
Created attachment 55155 [details] [review]
Bug 14695 [QA Followup] - Fix clearing of all holds by patron at checkout

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 197 Kyle M Hall 2016-09-03 01:31:11 UTC
Created attachment 55156 [details] [review]
Bug 14695 - Warn librarian if max_holds_per_record is reached

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 198 Kyle M Hall 2016-09-03 01:31:17 UTC
Created attachment 55157 [details] [review]
Bug 14695 - Don't use holds_to_place_count when checking an item.

This fix the following bug:
If you select 2 in "Holds to place (count)" and finally
check an item, so you get 2 holds for the same item.

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 199 Kyle M Hall 2016-09-03 01:31:24 UTC
Created attachment 55158 [details] [review]
Bug 14695 [QA Followup] - Fix method names 'type' to '_type'

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 200 Kyle M Hall 2016-09-03 01:31:30 UTC
Created attachment 55159 [details] [review]
Bug 14695: Fix typo in template

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 201 Kyle M Hall 2016-09-03 01:31:36 UTC
Created attachment 55160 [details] [review]
Bug 14695 [QA Followup] - Disable "Holds to place (count)" unless "Hold next available item" is checked

Signed-off-by: Jason M. Burds <JBurds@dubuque.lib.ia.us>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 202 Kyle M Hall 2016-09-03 01:31:43 UTC
Created attachment 55161 [details] [review]
Bug 14695 [QA Followup] - Tell librarian if item doesn't allow or forces item level holds in the OPAC
Comment 203 Kyle M Hall 2016-09-03 01:31:49 UTC
Created attachment 55162 [details] [review]
Bug 14695 [QA Followup] - Unit Tests
Comment 204 Kyle M Hall 2016-09-03 01:31:55 UTC
Created attachment 55163 [details] [review]
Bug 14695 [QA Followup] - Fix issues found by QA script
Comment 205 Jonathan Druart 2016-09-07 13:20:20 UTC
Created attachment 55299 [details] [review]
Bug 14695: Fix GetHardDueDate.t tests
Comment 206 Jonathan Druart 2016-09-07 13:22:46 UTC
Please add tests for:
  CanItemBeReserved returns 'itemAlreadyOnHold'
to t/db_dependent/Holds.t
Comment 207 Jonathan Druart 2016-09-07 13:29:17 UTC
TODO Later: 

+    my $old_hold = Koha::Old::Hold->new( $hold->unblessed() )->store();
+
+    $hold->delete();

This should be done in a transaction or at least make sure the entry has been correctly added to old_reserves before removing it from the reserves table.
Comment 208 Jonathan Druart 2016-09-07 13:45:43 UTC
C4::Reserves::GetReserveId

2333     my $hold = Koha::Holds->search( $params )->next();
2334               
2335     return $hold->id();

This will explode if no hold is matching the params.
Comment 209 Jonathan Druart 2016-09-07 13:52:23 UTC
t/db_dependent/Reserves/MultiplePerRecord.t
What's the difference between rule3 and rule4?
Comment 210 Jonathan Druart 2016-09-07 14:14:04 UTC
Koha::Holds->forced_hold_level
+        map { $has_item_level_holds ||= $_->itemnumber } $self->as_list();

Would not be easier to search for hold with itemnumber defined?
Comment 211 Jonathan Druart 2016-09-07 14:17:50 UTC
2 occurrences of 'resbarcode' in circ/returns.tt but removed from pl
Comment 212 Jonathan Druart 2016-09-07 14:32:10 UTC
request.tt l.259, li tag not correctly closed.
Comment 213 Jonathan Druart 2016-09-07 14:38:14 UTC
In opac-user.tt
 68                             <p>One or more holds were not placed due to existing holds.</p>

Is it still valid?
Comment 214 Jonathan Druart 2016-09-07 14:44:15 UTC
(In reply to Jonathan Druart from comment #149)
> And it would have been easier not to tidy stuffs in the same patch set, it
> does not ease the readability.

And yes, please try to avoid that, it's really painful to review.
Comment 215 Katrin Fischer 2016-09-08 05:56:50 UTC
This appears to have been pushed to master now? But is still failed QA?
Comment 216 Marcel de Rooy 2016-09-08 06:39:09 UTC
(In reply to Katrin Fischer from comment #215)
> This appears to have been pushed to master now? But is still failed QA?

Yes, that is weird. Maybe Kyle missed some statuses here :)
Comment 217 Jonathan Druart 2016-09-08 07:09:50 UTC
(In reply to Katrin Fischer from comment #215)
> This appears to have been pushed to master now? But is still failed QA?

These patches have been pushed by mistake to master.
2 solutions were possible: either revert them or QA them.
I talked with Kyle yesterday and I decided to have a look at the patches.
That's why the QA comments are a bit strewn...
Comment 218 Kyle M Hall 2016-09-08 10:50:19 UTC
(In reply to Jonathan Druart from comment #209)
> t/db_dependent/Reserves/MultiplePerRecord.t
> What's the difference between rule3 and rule4?

They have different itemtypes to demonstrate the behavior that if a record has mixed itemtypes the highest max holds per record is used.
Comment 219 Kyle M Hall 2016-09-08 11:01:43 UTC
Created attachment 55332 [details] [review]
Bug 14695 [QA Followup]

* Add tests for CanItemBeReserved returns 'itemAlreadyOnHold' to t/db_dependent/Holds.t
* Don't let GetReserveId explode
* Use search instead of map
* Remove instances of resbarcode
* Fix badly formed li closing tag
Comment 220 Jonathan Druart 2016-09-08 13:40:11 UTC
Kyle,
My review is not 100% complete but consider it as ok.
Please push the last 2 patches, update the schema files and move the atomic update to updatedatabase.sql

I'll let you know if I find something wrong later.
Comment 221 Kyle M Hall 2016-09-08 13:47:21 UTC
Pushed to master for 16.11. Thanks for the follow-up and the quick qa work Jonathan!
Comment 222 Katrin Fischer 2016-09-10 08:00:57 UTC
It's impossible for me to place an item level hold on a record with one item in staff. The message shown is:  Hold must be record level 
The max holds per record is set to 1. This is just after updating to latest master without any changes to configuration. Item level holds are allowed.
Comment 223 Katrin Fischer 2016-09-10 08:05:16 UTC
Also: checkin in an item on hold - the hold is not triggered...? Something seems very odd here.
Comment 224 Katrin Fischer 2016-09-10 08:09:31 UTC
(In reply to Katrin Fischer from comment #223)
> Also: checkin in an item on hold - the hold is not triggered...? Something
> seems very odd here.

Figured out the reason for that (item was marked not for loan after hold was placed) - above problem remains.
Comment 225 Benjamin Rokseth 2016-09-20 11:11:17 UTC
Seems the behaviour is intentional:
method forced_hold_level is implemented in Koha/Holds.pm and used in reserve/request.pl by this patch set, and is also commented:
Koha/Holds.pm:

>If a patron has multiple holds for a single record,
>those holds must be either all record level holds,
>or they must all be item level holds.
>
>This method should be used with Hold sets where all
>Hold objects share the same patron and record.
>
>This method will return 'item' if the patron has
>at least one item level hold. It will return 'record'
>if the patron has holds but none are item level,
>Finally, if the patron has no holds, it will return
>undef which indicateds the patron may select either
>record or item level holds, barring any other rules
>that would prevent one or the other.

but I'm not sure of the implementation since it seems to favour 'record' in any other case than existing item level hold?
Comment 226 Benjamin Daeuber 2016-09-20 14:33:32 UTC
(In reply to Katrin Fischer from comment #222)
> It's impossible for me to place an item level hold on a record with one item
> in staff. The message shown is:  Hold must be record level 
> The max holds per record is set to 1. This is just after updating to latest
> master without any changes to configuration. Item level holds are allowed.

Just brainstorming on this, but we wouldn't want to be in a position where a user accidentally placed an item level hold and a new copy comes in that goes on the shelf instead of to the user. If there is one item on a record chances are it's a hold that should be filled at the record level. But it's not guaranteed.

I think the best way to fix this problem is to eventually allow item level holds to be filled by identical items (say we get a second issue of a serial or another set of dvds) rather than forcing a record level hold.

Still, I'm happy with this as a first step.
Comment 227 Katrin Fischer 2016-09-21 05:42:18 UTC
If this is an intended behaviour change - I am not in favor of it and it's unusual to push things in like this without discussion.

I think we should not force staff to do a record level hold if they really want to do an item level hold. For the OPAC, this might be debatable.

Also: I didn't have any other hold on that record when encountering this.

One scenario I can imagine for staff: Put a hold on the damaged item, that is still out to the user, while waiting for the already ordered new item (assuming th library is using AcqCreateItem with 'on receive' or 'cataloguing')
Comment 228 Katrin Fischer 2016-09-21 06:00:05 UTC
This also breaks the option to enforce item holds!

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.
Comment 229 Katrin Fischer 2016-09-21 06:07:53 UTC
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.

Please, can someone investigate this?
Comment 230 Katrin Fischer 2016-09-21 06:10:20 UTC
Created attachment 55730 [details]
screenshot of my circulation rule
Comment 231 Katrin Fischer 2016-09-21 06:10:52 UTC
Created attachment 55731 [details]
'place hold' screen in staff
Comment 232 Chris Cormack 2016-09-21 20:52:08 UTC
I confirm this regression Katrin has spotted. I realise this a problematic bug, that was pushed before its time, but we really need to fix this issue, or figure out how to revert it back out.
Comment 233 Kyle M Hall 2016-09-21 21:03:09 UTC
(In reply to Katrin Fischer from comment #229)
> 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.
> 
> Please, can someone investigate this?

I will definitely look into this!
Comment 234 Kyle M Hall 2016-09-21 21:53:29 UTC
Fix has been posted as bug 17327
Comment 235 Marcel de Rooy 2016-11-25 11:44:43 UTC
Messages: If you go over the number of holds per record, the message is: No items available. Which is/may be untrue. Why not: you reached the max ?
Comment 236 Petter Goksøyr Åsen 2016-12-12 08:40:22 UTC
See bug #17758
Comment 237 Mason James 2017-01-25 00:40:02 UTC
New feature, skipping for 16.05.x
Comment 238 Jonathan Druart 2017-02-08 16:50:52 UTC
Sounds like this patchset caused bug 17940.
Comment 239 Jonathan Druart 2017-02-22 09:29:11 UTC
And bug 18150.
Comment 240 Kyle M Hall 2017-02-23 11:15:46 UTC
(In reply to Marcel de Rooy from comment #235)
> Messages: If you go over the number of holds per record, the message is: No
> items available. Which is/may be untrue. Why not: you reached the max ?

Good thought Marcel. File a bug report for that and ping me about it :)
Comment 241 David Cook 2022-02-28 03:01:21 UTC
Was this feature reverted? I've never actually seen it operational in Koha, which is why bug 15565 exists in its present all-encompassing state?
Comment 242 David Cook 2022-02-28 03:02:38 UTC
If it is possible to make multiple item holds, it looks like they're hard-coded to use the bib-level Pickup location and not the item-level Pickup locations, which seems odd... and I'd be surprised no one has picked that up yet...
Comment 243 Katrin Fischer 2022-02-28 11:07:32 UTC
(In reply to David Cook from comment #241)
> Was this feature reverted? I've never actually seen it operational in Koha,
> which is why bug 15565 exists in its present all-encompassing state?

It's possible in staff, but you need to allow multiple holds on one record in circulation rules. OPAC implementation is pending.
Comment 244 David Cook 2023-05-23 23:34:11 UTC
(In reply to Katrin Fischer from comment #243)
> (In reply to David Cook from comment #241)
> > Was this feature reverted? I've never actually seen it operational in Koha,
> > which is why bug 15565 exists in its present all-encompassing state?
> 
> It's possible in staff, but you need to allow multiple holds on one record
> in circulation rules. OPAC implementation is pending.

I think maybe I've always misunderstood this one. I thought it was supposed to allow you to place multiple item level holds at the same time. 

"At the same time" meaning during the "placing" of the hold rather than the "having" of the hold. 

And that's why we still need bug 15565