Bug 14695 allows to place multiple holds for the same record. It would be nice to choose the numbers of items (or select several items) at once instead of making X times the same operation. We want this behavior on staff and OPAC.

/home/vagrant/kohaclone/.git/rebase-apply/patch:432: trailing whitespace.
my @biblionumbers = ref($query->param('biblionumbers')) eq 'Array' 
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.

Rebased against master

Fails in intra on placing any number of holds with:

/cgi-bin/koha/reserve/placerequest.pl
DBIx::Class::Storage::DBI::_dbh_execute(): Column 'biblionumber' cannot be null at /kohadev/kohaclone/Koha/Object.pm line 109

I notice that in the request, param multi_holds is not present

(In reply to Benjamin Rokseth from comment #9)
> Fails in intra on placing any number of holds with:
> 
> /cgi-bin/koha/reserve/placerequest.pl
> DBIx::Class::Storage::DBI::_dbh_execute(): Column 'biblionumber' cannot be
> null at /kohadev/kohaclone/Koha/Object.pm line 109
attachment 50392 [details] [review] fixes that

1 out of 7 hunks FAILED -- saving rejects to file opac/opac-reserve.pl.rej
...
2 out of 3 hunks FAILED -- saving rejects to file reserve/placerequest.pl.rej

seems to conflict with Bug 15533 - Allow patrons and librarians to select itemtype when placing holds pushed recently?

Alex, could you confirm that you will abandon your patches in favor of the ones on bug 14695?

(In reply to Jonathan Druart from comment #14)
> Alex, could you confirm that you will abandon your patches in favor of the
> ones on bug 14695?

Sorry, misread, forget that.

Patches have been rebased on master. Please test.

There is no alert when exceeding the maximum of holds per record, but Koha places holds only in the number of items fixed in the circ rules.
I think it should react like when maximum holds allowed is reached : checkbox can't be selected.

Sorry, devbox trouble with multiple sign-offs.  I did test and received the alert when I tried too many holds per record, but was able to place holds on multiple items per bib.

Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>

This still applies, could we get a QA to look at it please? Really good feature.

Cheers,
Liz

Undefined subroutine &CGI::Compile::ROOT::home_vagrant_kohaclone_opac_opac_2dreserve_2epl::GetBiblionumberFromItemnumber called at /home/vagrant/kohaclone/opac/opac-reserve.pl line 244.


This subroutine has been removed on bug 18277.

Hi,

Lots of conflicts applying this now.

Cheers,
Liz

We'd really like this patch in, does Biblibre plan to work on it more, if not please let us know so we can perhaps try to get it fixed to pass QA ourselves

Hi Alex,

patch applies, QA script passes. Bit baffled there are no changes to C4/Koha and no unit tests.

Can you please amend the first patch with a proper test plan and fix the commit messages? (Bug XXXX:)

Ping?

I have done the following changes to the patches on this bug report to try and get it moving along again:

(#1 and #2 address Katrin's notes in comment 80)
 
1. Amended the first patch to include a proper test plan

2. Fixed the commit messages of all commits to be in the format: (Bug: XXXX)

3. Rebased all commits against master

4. Reworked the css in the second patch because since that commit was written we have switched to scss.

5. Added an additional patch which fixes a few bugs when placing an item level hold from the OPAC. 

I will now attach these commits to the bug report.

Patch failed on PTFS sanbdbox 0:

Some problems occurred applying patches from bug 15565:
<h1>Something went wrong !</h1>Applying: Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
Applying: Bug 15565: CSS changes
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M       koha-tmpl/opac-tmpl/bootstrap/css/opac.css
M       koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss
Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/opac.css
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/css/opac.css
Patch failed at 0001 Bug 15565: CSS changes
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 15565 - Place multiple holds at once for the same record

78242 - Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
78243 - Bug 15565: CSS changes
78244 - Bug 15565: Use item's biblionumber only on item-level request
78245 - Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off
78246 - Bug 15565: Alert librarian that maximum holds per record is reached
78247 - Bug 15565: Remove use of GetBiblionumberFromItemnumber
78248 - Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC
78266 - Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl
78305 - Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-15565-CSS-changes-i7sD5P.patch .

I am going to go through obsoleting the existing patches and attach rebased patches onto this bug report.

The rebased patches should now apply cleanly on master (as of 5/9/2018), as shown below:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
Applying: Bug 15565: CSS changes
Applying: Bug 15565: Use item's biblionumber only on item-level request
Applying: Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off
Applying: Bug 15565: Alert librarian that maximum holds per record is reached
Applying: Bug 15565: Remove use of GetBiblionumberFromItemnumber
Applying: Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC
Applying: Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl
Applying: Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds

Applying: Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
M       koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
M       opac/opac-reserve.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-reserve.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-15565-Place-multiple-holds-at-once-for-the-sam-kXB0M4.patch


Lisette

Trying to work through this:

- I've rebased the patches (generated updated CSS file for the second patch)
- I've tested the current behaviour and am a little confused: It appears this feature is already available for the staff side, so this is about adding it to the OPAC side. I've adjusted the bug title. Correct? 

TOOD:

1) QA script fails:

- Tons of missing filters :(

2) Code review

Left over debug code:
use Data::Dumper;
warn $itemnumber;
warn $canreserve;
if ($canreserve) {
 warn $canreserve;
...

Little typo:
+        # if we have an item selectionned,
selected :)

3) Add text for release notes (not blocker, can be done later)

Please clean up the patches quickly for testing this week! First tests are looking good.

Hi there

I have fixed the apply conflict on the second patch, and have attached a follow-up patch which:

* Adds the missing filters to request.tt, opac-reserve.tt 
* Corrects typo in code comment
* Removes left-over debug code 

Cheers,
Alex

Treating last patch as a QA follow-up ;)

QA script is green now with the latest follow-up.

There is a problem in the way that item level holds are handled:

1) Circulation condition: Item level holds not allowed

Without this patch, the radio buttons for "next available" and "item specific" are not shown, which is the correct behavior.
With this patch, they always show.

2) Circulation condition: Enforce item level holds

It should only be showing "specific hold" here. The radio boxes are greyed out, but it might be nicer to restore former behaviour.

3) Holds per record allowed: 2, forced item holds as above

Go to a detail page for a record with more than 2 items.
Select 3 items for hold (item level)
Save
You get redirected to the account, but no holds have been placed.

(In reply to Katrin Fischer in comment 137):

Hi Katrin

I have addressed points 1 and 2 in the follow-up patch just attached. 

I have been unable to replicate issue 3; when 'Holds per record (count)'=2 and 'Item level hold'=Force I have found on opac-reserve.pl that after selecting 2 item checkboxes in the OPAC reservation interface the remaining checkboxes are disabled by JS preventing you from placing more than set in 'Holds per record (count)'. 

Are you able to elaborate a bit more on what you are seeing?

Cheers,
Alex

CSS changes do not apply - should not include the css for staff side?

Staff side:
1 - Warning box when excedding maxreserves, however, I can place holds beyond the warning whether  AllowHoldPolicyOverride  is set or not
2 - Holds Per day setting not taken into account while placing holds (item level or next available)

OPAC:
1 - When placing item level holds the user is warned of exceeding maxreserves, however, if holds per day is less there is no warning, but holds are not placed beyond the limit with no warning (maxreserves=5 holds per day = 3, check 5 boxes, only 3 holds are placed)

Just a note: holds per day was not in yet when I started testing this.

Hi Alex, 
any chance to fix this before the final freeze?

Hi Katrin,

Sure thing working on fix right now.

(In reply to Katrin Fischer in comment 154 and Nick Clemens in comment 152):

Hi both,

I have written a follow-up patch which does the following:

OPAC: 
The MaxReserves syspref, holds per day and holds per record circ setting values are all respected in determining how many items can be reserved. 

* Once the lowest of these 3 values is reached all un-checked item checkboxes on opac-reserve are disabled.

Staff client:

Similar to the OPAC in that once the lowest of the 3 values is reached all un-checked item checkboxes on request.tt are disabled except is the AllowHoldPolicyOverride is set to 'Allow'.

If AllowHoldPolicyOverride is enabled then additional checkboxes can be selected with the warning modal boxes loading on each checkbox click. 

Also added function to C4::Reserves named GetAllowedHoldsForPatronToday() which returns the holds per day value for the patron and item type combination. 

I have added unit tests for this new function in t/db_dependent/Reserves/MultiplePerRecord.t

Can you please test this?

I'm not happy with adding things to the C4:: namespace, what about adding

my $biblio = Koha::Biblios->find( $biblio_id );
my $holds_allowed_on_record_today = $biblio->allowed_holds({ patron => $patron_obj });

holds are usually added in context in which we already have the $patron object, and a biblionumber.

Thanks Tomas I will try that in a follow-up patch a bit later today when I have some time.

Hi all,

I have just added another follow-up which moves the function out of C4/Reserves into Koha/Biblio (as Tomás suggested in comment 169)

The caller for this function is now: my $holds_allowed_on_record_today = $biblio->allowed_holds( $patron_obj );

I have amended the t/db_dependent/Reserves/MultiplePerRecord.t unit tests. 

Can this please be tested and if you could let me know of further required changes that would be great.

Many thanks,
Alex

Some first comments here:

1) CSS
Something is off here, there is no more less directory on master:

http://git.koha-community.org/gitweb/?p=koha.git;a=tree;f=koha-tmpl/opac-tmpl/bootstrap;hb=e112c6f117d4b6fd6c6160e8be9cba4186db1279


koha-tmpl/opac-tmpl/bootstrap/less/opac.less               | 2644 ++++++++++++++++++++++++++++++++++++++++++++

Also I think generated .css files should not be included.


2) Asking Joubu to glance at the code, 2 more things (non-blocker):

foreach my $item ( $self->items()->as_list() ) {

would be better as:

while ( my $item = $items->next ) {

And tests should be in their own subtest.

3) The logic appears not correct too:
I haven't placed any hold today.
I allow one daily hold, 2 holds per record.

Record level hold: let's me select 2 from pull down. On saving I am redirected to the patron account but no hold has been placed.

Item level hold: When switching to specific item I immediately get the error:

Maximum number of reserves allowed today reached.
(reserves has to be holds here!)

One checkbox is selected and I can save the hold -> so the warning should not have displayed.

Agreed with Alex that this will need some more work and clean-up before going in. Will come back to this!

Created attachment 83331 [details] [review]
Bug 15565: Place multiple holds at once for the same record on staff and OPAC.

Test plan:
1. Create an circulation rule to use for testing the patches on this bug
report. Ensure it has a value greater than 1 for 'Holds
allowed', 'Holds per record (count)', and 'Item level holds' is enabled.

2. Also ensure the 'RequestOnOpac' syspref is set to 'Allow'.

3. Go to the reservation page (request.pl) of a biblio in the staff client with more
than one item.

4. Notice beside each item a radio button is displayed preventing you
from being able to select multiple items to place a hold on.

5. Go to the reservation page (opac-reserve.pl) of the same biblio in the OPAC
also notice a radio button is displayed beside every item.

6. Apply patches

7. Repeat step 3 and 5 and notice in both the staff client and OPAC
checkboxes rather than radio buttons are displayed beside each item so
you can select multiple items for reservation at a time.

8. From staff interface, place several item-level holds for the same user
on the same record at once:

  - check that you are warned if you exceed "Holds per record (count)"
(circulation rules) for this record.
  - check also maxReserves system preference.

9. From opac, place several item-level and record level holds
for the same user on the same record at once:

  - check that can't exceed "Holds per record (count)" for
  this record.
  This is enforced by all unticked item checkboxes being
  disabled after the number of ticked checkboxes equals the 'Holds per record (count)' (circulation rules) value.

  - check also maxReserves system preference.

Do the same in OPAC and staff by selecting several
record (multi-hold).

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83332 [details] [review]
Bug 15565: Use item's biblionumber only on item-level request

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83333 [details] [review]
Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off

Signed-off-by: Danielle Elder <danielle@bywatersolution.com>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83334 [details] [review]
Bug 15565: Alert librarian that maximum holds per record is reached

Signed-off-by: Dani Elder <dani@bywatersolutions.com>

Signed-off-by: Danielle Elder <danielle@bywatersolution.com>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83335 [details] [review]
Bug 15565: Remove use of GetBiblionumberFromItemnumber

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83336 [details] [review]
Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC

This commit fixes two bugs:

1. C4::Reserves->CanItemBeReserved() returns an output in a hash in the format: status => 'output'

However opac-reserve.pl is not accessing th output of this function
correctly consequently no item is ever reservable.

And so after a patron submits the reservation form on opac-reserve.pl it
looks like the item-level reservation has successfully been placed
however it has not and no error message is displayed to them.

This commit stores the output of the aforementioned function in a hash
and then checks the value of the status key. As a result opac-reserve.pl
now correctly accesses the value returned and item level holds are
successfully placed from the OPAC.

2. If a biblio has previously been reserved then the opac-reserve.pl page
shows the 'Next available item' and 'A specific item' radio buttons to be
disabled to ensure patrons cannot place a reservation of a different
type on the biblio.

With both of these radio buttons being disabled it results in item level holds
placed in the OPAC on a previously reserved item not being stored in the
database. This is very confusing for users.

I have implemented a conditional which checks what the forced_hold_type
of the previous hold was. If it was 'item' then the $reqtype =
'Specific' and if it was 'record' then $reqtype is 'Any'.

This means there will always be a value for $reqtype, and this variable
is checked if it is equal to 'Specific' and if it is then an item level
hold is placed.

Test plan:
1. Ensure in your circulation rules that 'Item level holds' is enabled

2. Apply all patches on this bug report except this commit

3. In the OPAC try placing a item level hold (and notice you can submit
the opac-reserve.pl page and no error messages are displayed. However
the hold(s) are not being placed.

4. Place an item level hold(s) on the biblio from staff client interface
and then return to the opac-reserve.pl page for the biblio in the OPAC
and notice both the 'Next available' and 'A specific item' radio buttons
are disabled.

5. Again try placing an item level hold in the OPAC and notice dispite
no error/warning message being displayed the hold is not being placed.

6. Apply this patch

7. Restart memcached, and plack

8. In the staff client remove the reservation you placed from the staff
client.

9. Repeat step 3 and notice when you go to your opac summary
(opac-user.pl) page the holds are displayed. .i.e. You can now
successfully place item level holds in the OPAC.

10. Return to the opac-reserve.pl page for the biblio and notice that
because you have already placed a hold on this biblio the 'Next
available item' and 'A specific item' radiobuttons are disabled.

11. Select several of the items checkboxes to place more item level
holds and press the 'Confirm hold' button.

12. Go to your opac summary (opac-user.pl) page again and notice the
holds you placed in step 11 are shown there.

i.e. you can place item level holds from the OPAC on biblios which have
previously had reserves placed on them.

Sponsored-By: Brimbank Library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83337 [details] [review]
Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl

Previously the selected branch dropdown value was not being successfully retrieved in
opac-reserve.pl, this resulted in opac holds being placed with no pickup
branch being set.

Consequently as no branch was associated with the hold an Ajax error was
thrown when viewing the holds-table in the patron page in the staff
client.

This commit successfully retrieves the selected branch value and so
fixing the Ajax error.

Test plan:
1. Apply all patches on bug report 15565 except this commit

2. Place a hold(s) from opac-reserve.pl.

3. Go to your opac patron summary page (opac-user.pl) and notice
although the holds are successfully displayed they have no 'Pickup
location' value

4. Go to the patron page in the staff client and click on the 'Hold(s)'
tab and notice a popup explaining the datatable Ajax error is loaded

5. Apply this patch

6. Repeat step 3 noticing this time in the
opac-user.pl a 'Pickup location' is displayed for the hold you just
created.

6. Repeat step 4 and notice the hold datatable is successfully loaded
and populated without an Ajax error popup being loaded.

Sponsored-By: Brimbank Library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83338 [details] [review]
Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds

This commit implements changes to opac-reserve.tt and opac-reserve.pl.

1. The change to opac-reserve.tt JS fixes the total_requested() function which returns an output which is always 2 digits higher than the actual number of selected item checkboxes.

i.e. if the 'maxreserves' syspref (limit for the overall
number of reserves allowed by a user) is 4 and a user has no previous
holds and ticks 3 item checkboxes and submits the form.

The 'Maximum number of reserve exceded.' popup message is displayed,
however as they have selected 3 items for reservation this is less than
the maxreserves value of 4.

The popup is displayed because total_requested() JS function is
returning 5 (i.e. 2 on top of the actual number of requested holds).

This commit usbtracts 2 from the number of holds calculated by
total_requested() so that the accurate number of requested holds is
being used to determine whether or not to display the excess holds
popup.

2. When the 'Holds per record (count)' (circulation rule) and
'maxreserves' syspref are the same value (e.g. 4) you can tick 4 item
checkboxes in opac-reserve.pl and submit the form.

The form will submit successfully and to the user everything looks to
have worked however the holds will not be
successfully created.

This is because after the form is submitted to opac-reserve.pl a check is
done:

If 'number of submitted hold requests' + 'number of previous existing holds'
greater than or equal to maxreserves syspref then set the variable
'$canreserve' to 0.

If the $canreserve variable is 0 the form will submit, no warning/error
message is displayed to the user but the holds are not created.

Users should be able to place 4 holds if the maxreserves syspref is 4
(just not more than 4 holds). This commit changes this check to:

If 'number of submitted hold requests' + 'number of previous existing
holds' greater than maxreserves syspref then set the variable '$canreserve' to 0..

i.e. notice we only check for greater than (not equal to).

Which solves the bug of the form submitting and no holds being created
when the maxreserves syspref and 'Holds per record (count)' (circulation
rule) are the same.

Test plan (in two parts to test fix #1 and #2 (above)):

Test plan for fix #1:
1. Apply all patches on this bug report (15565) and follow and confirm
the feature works as described in the test plan in the first commit

2. Set the 'maxreserves' syspref to 3 (and 'Holds per record (count)'
(circulation rule) to 5)

3. Ensure your logged in patron account has no existing reserves

4. In the OPAC go to a biblio page with 3 or more items

5. In the reservation page (opac-reserve.pl) for that biblio select 2
item checkboes to place 2 item level holds and notice the 'Maximum
number of reserve exceded.' popup message is displayed.

As the maxreserves syspref value is 3 you should be able to place 2
holds without exceeding that value

6. Apply this patch

7. Repeat step 5 and notice the form submits without the exceeding popup
message and the holds are successfully placed.

Test plan for fix #2:
1. Apply all patches attached to this bug report (bug 15565) and perform
the test plan in the first commit and verify it works.

2. Set your 'Holds per record (count)' (circulation rule) and
'maxreserves' syspref to the same value 3

3. Go to a biblio in the OPAC with 3 or more items

4. In the opac-reserve.pl reservation interface for that biblio tick 3
item checkboxes to place 3 item level holds and submit the form.

5. Notice the form submits and no warning/error message is displayed

6. In the opac user summary page that is displayed after the reserve
form is submitted notice the holds are not displayed this is because
they have not been placed

7. Apply this patch

8. Repeat step 4 and 5 and notice the holds have been successfully
placed and are displayed in your opac user summary page.

Sponsored-By: Brimbank Library
Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83339 [details] [review]
Bug 15565: (follow-up) Removed leftover debug code and added missing filters

Sponsord-By: Brimbank Library, Australia

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83340 [details] [review]
Bug 15565: (QA follow-up) Fix some last filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83341 [details] [review]
Bug 15565: (follow-up) Moved function form C4/Reserves to Koha/Biblio

Also updated t/db_dependen/Reserves/MultiplePerRecord.t test to reflect
moving function.

Added the display of the JS modal box informing OPAC borrower that
they have exceeded hold limit after they click 'A specific item'
radiobutton on opac-reserve if holds per day, holds per record or
maxreserves is 1

Test plan:
1. Apply patches
2. Restart memcached and plack
3. Run t/db_dependent/Reserves/MultiplePerRecord.t
4. Set your maxreserves syspref to 3, and a circ rules holds per record
to 2 and holds per day to 1
5. In the OPAC visit the 'Place hold' interface of item. Select the 'A
specific item' radiobutton and observe the first item checkbox is
checked and other unchecked checkboxes are disabled.
6. Repeat step 4 this time setting holds per day to 5
7. Now reloading the OPAC 'Place hold' page notice after clicking 'A
specific item' no modal displays
8. Select a second item checkbox and observe a modal saying you have reached
the maximum number of holds for the record is loaded - notice the
wording 'hold' not 'reserve' in use in the modal box
9. Click 'OK' on modal and observe all unchecked item checkboxes are
automatically disabled to prevent additional holds being placed
10. Select 'Confirm hold'
11. On your holds summary page confirm both holds are placed
12. In the staff client set the 'AllowHoldPolicyOverride' syspref to
"Don't allow"
13. Visit a biblio reservation interface in the staff client
14. Select 2 item checkboxes and observe the modal box explaining you
have reached maximum holds for the record is displayed - notice the
wording of 'hold' not 'reserve' is used in the modal box.
15. Select 'OK' on the modal and notice all un-checked item checkboxes
are automatically disabled
16. Repeat steps 12-15 this time with the value of
'AllowHoldPolicyOverride' syspref set to 'Allow' and notice that once
you have clicked on the second item checkbox although the modal still
loads the item checkboxes do not disable - you are able to override and
continue selecting checkboxes each time with modal warning loading.

17. In both the OPAC and staff client try placing a record level hold,
selecting a value from the 'Holds to place (count)' dropdown. Notice
that the maximum value of this is the lowest value out of the 'maximum
number of holds per record', 'maximum number of holds per day' and the
'maxreserves' system preference so in this case it will be 2
18. Save and notice 2 holds are placed

19. Observe the allow_holds() function is in the Koha/Biblio.pm file and
the GetAllowedHoldsForPatronToday() function has been removed from
C4/Reserves.pm

Sponsored-By: Brimbank Library, Australia

Created attachment 83342 [details] [review]
Bug 15565: (follow-up) Changes to maximum hold allowed logic

Implemented changes to the logic to address Katrin Fischer's points in
comment #175, also changed the modal box wording to use 'hold' not
'reserve'

Also updated t/db_dependent/Reserves/MultiplePerRecord.t test to reflect
moving function.

Added the display of the JS modal box informing OPAC borrower that
they have exceeded hold limit after they click 'A specific item'
radiobutton on opac-reserve if holds per day, holds per record or
maxreserves is 1

Test plan:
1. Apply patches
2. Restart memcached and plack
3. Run t/db_dependent/Reserves/MultiplePerRecord.t
4. Set your maxreserves syspref to 3, and a circ rules holds per record
to 2 and holds per day to 1
5. In the OPAC visit the 'Place hold' interface of item. Select the 'A
specific item' radiobutton and observe the first item checkbox is
checked and other unchecked checkboxes are disabled.
6. Repeat step 4 this time setting holds per day to 5
7. Now reloading the OPAC 'Place hold' page notice after clicking 'A
specific item' no modal displays
8. Select a second item checkbox and observe a modal saying you have reached
the maximum number of holds for the record is loaded - notice the
wording 'hold' not 'reserve' in use in the modal box
9. Click 'OK' on modal and observe all unchecked item checkboxes are
automatically disabled to prevent additional holds being placed
10. Select 'Confirm hold'
11. On your holds summary page confirm both holds are placed
12. In the staff client set the 'AllowHoldPolicyOverride' syspref to
"Don't allow"
13. Visit a biblio reservation interface in the staff client
14. Select 2 item checkboxes and observe the modal box explaining you
have reached maximum holds for the record is displayed - notice the
wording of 'hold' not 'reserve' is used in the modal box.
15. Select 'OK' on the modal and notice all un-checked item checkboxes
are automatically disabled
16. Repeat steps 12-15 this time with the value of
'AllowHoldPolicyOverride' syspref set to 'Allow' and notice that once
you have clicked on the second item checkbox although the modal still
loads the item checkboxes do not disable - you are able to override and
continue selecting checkboxes each time with modal warning loading.

17. In both the OPAC and staff client try placing a record level hold,
selecting a value from the 'Holds to place (count)' dropdown. Notice
that the maximum value of this is the lowest value out of the 'maximum
number of holds per record', 'maximum number of holds per day' and the
'maxreserves' system preference so in this case it will be 2
18. Save and notice 2 holds are placed

19. Observe the allow_holds() function is in the Koha/Biblio.pm file and
the GetAllowedHoldsForPatronToday() function has been removed from
C4/Reserves.pm

Sponsored-By: Brimbank Library, Australia

Created attachment 83378 [details] [review]
Bug 15565: (follow-up) Place multiple holds from opac-search.pl

Previously before this patch if you selected multiple bibliographic
results from opac-search.pl and selected 'Place hold' and confirmed the
hold only 1 hold on the first of the selected biblios would be placed.

This patch fixes this bug and enables users to be able to place holds on
multiple bibliographic records in opac-search.pl, select 'Place hold', select if they want
each of these holds to be on 'Next available', 'Next available from
group', or 'A specific item' and place their holds and all the holds
they placed will be placed correctly.

Test plan:
1. Follow test plan on the last patch - (Don't apply this patch)
2. Try to select multiple bibliographic record checkboxes and select
'Place hold'
3. Confirm holds and notice you are re-directed to opac-user.pl and only
the first hold has been placed

4. Apply this patch
5. Restart memcached and plack if running
6. Repeat steps 2 and 3 and notice all holds were placed
7. Repeat step 2 and then select a variety of 'A specific item' and 'Next
available item' holds for each of the bibliographic holds and
confirm they are all placed correctly
8. Confirm you can still place multiple holds correctly in opac-reserve
accessing from a OPAC biblio detail page

Sponsored-By: Brimbank Library, Australia

Created attachment 83380 [details] [review]
Bug 15565: (follow-up) Changes to maximum hold allowed logic

Implemented changes to the logic to address Katrin Fischer's points in
comment #175, also changed the modal box wording to use 'hold' not
'reserve'

Also updated t/db_dependent/Reserves/MultiplePerRecord.t test to reflect
moving function.

Added the display of the JS modal box informing OPAC borrower that
they have exceeded hold limit after they click 'A specific item'
radiobutton on opac-reserve if holds per day, holds per record or
maxreserves is 1

Test plan:
1. Apply patches
2. Restart memcached and plack
3. Run t/db_dependent/Reserves/MultiplePerRecord.t
4. Set your maxreserves syspref to 3, and a circ rules holds per record
to 2 and holds per day to 1
5. In the OPAC visit the 'Place hold' interface of item. Select the 'A
specific item' radiobutton and observe the first item checkbox is
checked and other unchecked checkboxes are disabled.
6. Repeat step 4 this time setting holds per day to 5
7. Now reloading the OPAC 'Place hold' page notice after clicking 'A
specific item' no modal displays
8. Select a second item checkbox and observe a modal saying you have reached
the maximum number of holds for the record is loaded - notice the
wording 'hold' not 'reserve' in use in the modal box
9. Click 'OK' on modal and observe all unchecked item checkboxes are
automatically disabled to prevent additional holds being placed
10. Select 'Confirm hold'
11. On your holds summary page confirm both holds are placed
12. In the staff client set the 'AllowHoldPolicyOverride' syspref to
"Don't allow"
13. Visit a biblio reservation interface in the staff client
14. Select 2 item checkboxes and observe the modal box explaining you
have reached maximum holds for the record is displayed - notice the
wording of 'hold' not 'reserve' is used in the modal box.
15. Select 'OK' on the modal and notice all un-checked item checkboxes
are automatically disabled
16. Repeat steps 12-15 this time with the value of
'AllowHoldPolicyOverride' syspref set to 'Allow' and notice that once
you have clicked on the second item checkbox although the modal still
loads the item checkboxes do not disable - you are able to override and
continue selecting checkboxes each time with modal warning loading.

17. In both the OPAC and staff client try placing a record level hold,
selecting a value from the 'Holds to place (count)' dropdown. Notice
that the maximum value of this is the lowest value out of the 'maximum
number of holds per record', 'maximum number of holds per day' and the
'maxreserves' system preference so in this case it will be 2
18. Save and notice 2 holds are placed

19. Observe the allow_holds() function is in the Koha/Biblio.pm file and
the GetAllowedHoldsForPatronToday() function has been removed from
C4/Reserves.pm

Sponsored-By: Brimbank Library, Australia

Created attachment 83381 [details] [review]
Bug 15565: (follow-up) Place multiple holds from opac-search.pl

Previously before this patch if you selected multiple bibliographic
results from opac-search.pl and selected 'Place hold' and confirmed the
hold only 1 hold on the first of the selected biblios would be placed.

This patch fixes this bug and enables users to be able to place holds on
multiple bibliographic records in opac-search.pl, select 'Place hold', select if they want
each of these holds to be on 'Next available', 'Next available from
group', or 'A specific item' and place their holds and all the holds
they placed will be placed correctly.

Test plan:
1. Follow test plan on the last patch - (Don't apply this patch)
2. Try to select multiple bibliographic record checkboxes and select
'Place hold'
3. Confirm holds and notice you are re-directed to opac-user.pl and only
the first hold has been placed

4. Apply this patch
5. Restart memcached and plack if running
6. Repeat steps 2 and 3 and notice all holds were placed
7. Repeat step 2 and then select a variety of 'A specific item' and 'Next
available item' holds for each of the bibliographic holds and
confirm they are all placed correctly
8. Confirm you can still place multiple holds correctly in opac-reserve
accessing from a OPAC biblio detail page

Sponsored-By: Brimbank Library, Australia

Over the last little while in amongst lots of other stuff I have made changes to fix the issues Katrin outlined in comments 174 and #175.

Please retest :)

Aleisha is going to manage/work on the upstreaming of the patches on this bug report, as I am unlikely to have enough time to do this for the next while.

Created attachment 84541 [details] [review]
Bug 15565: Place multiple holds at once for the same record on staff and OPAC.

Test plan:
1. Create an circulation rule to use for testing the patches on this bug
report. Ensure it has a value greater than 1 for 'Holds
allowed', 'Holds per record (count)', and 'Item level holds' is enabled.

2. Also ensure the 'RequestOnOpac' syspref is set to 'Allow'.

3. Go to the reservation page (request.pl) of a biblio in the staff client with more
than one item.

4. Notice beside each item a radio button is displayed preventing you
from being able to select multiple items to place a hold on.

5. Go to the reservation page (opac-reserve.pl) of the same biblio in the OPAC
also notice a radio button is displayed beside every item.

6. Apply patches

7. Repeat step 3 and 5 and notice in both the staff client and OPAC
checkboxes rather than radio buttons are displayed beside each item so
you can select multiple items for reservation at a time.

8. From staff interface, place several item-level holds for the same user
on the same record at once:

  - check that you are warned if you exceed "Holds per record (count)"
(circulation rules) for this record.
  - check also maxReserves system preference.

9. From opac, place several item-level and record level holds
for the same user on the same record at once:

  - check that can't exceed "Holds per record (count)" for
  this record.
  This is enforced by all unticked item checkboxes being
  disabled after the number of ticked checkboxes equals the 'Holds per record (count)' (circulation rules) value.

  - check also maxReserves system preference.

Do the same in OPAC and staff by selecting several
record (multi-hold).

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84542 [details] [review]
Bug 15565: Use item's biblionumber only on item-level request

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84543 [details] [review]
Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off

Signed-off-by: Danielle Elder <danielle@bywatersolution.com>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84544 [details] [review]
Bug 15565: Alert librarian that maximum holds per record is reached

Signed-off-by: Dani Elder <dani@bywatersolutions.com>

Signed-off-by: Danielle Elder <danielle@bywatersolution.com>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84545 [details] [review]
Bug 15565: Remove use of GetBiblionumberFromItemnumber

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84546 [details] [review]
Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC

This commit fixes two bugs:

1. C4::Reserves->CanItemBeReserved() returns an output in a hash in the format: status => 'output'

However opac-reserve.pl is not accessing th output of this function
correctly consequently no item is ever reservable.

And so after a patron submits the reservation form on opac-reserve.pl it
looks like the item-level reservation has successfully been placed
however it has not and no error message is displayed to them.

This commit stores the output of the aforementioned function in a hash
and then checks the value of the status key. As a result opac-reserve.pl
now correctly accesses the value returned and item level holds are
successfully placed from the OPAC.

2. If a biblio has previously been reserved then the opac-reserve.pl page
shows the 'Next available item' and 'A specific item' radio buttons to be
disabled to ensure patrons cannot place a reservation of a different
type on the biblio.

With both of these radio buttons being disabled it results in item level holds
placed in the OPAC on a previously reserved item not being stored in the
database. This is very confusing for users.

I have implemented a conditional which checks what the forced_hold_type
of the previous hold was. If it was 'item' then the $reqtype =
'Specific' and if it was 'record' then $reqtype is 'Any'.

This means there will always be a value for $reqtype, and this variable
is checked if it is equal to 'Specific' and if it is then an item level
hold is placed.

Test plan:
1. Ensure in your circulation rules that 'Item level holds' is enabled

2. Apply all patches on this bug report except this commit

3. In the OPAC try placing a item level hold (and notice you can submit
the opac-reserve.pl page and no error messages are displayed. However
the hold(s) are not being placed.

4. Place an item level hold(s) on the biblio from staff client interface
and then return to the opac-reserve.pl page for the biblio in the OPAC
and notice both the 'Next available' and 'A specific item' radio buttons
are disabled.

5. Again try placing an item level hold in the OPAC and notice dispite
no error/warning message being displayed the hold is not being placed.

6. Apply this patch

7. Restart memcached, and plack

8. In the staff client remove the reservation you placed from the staff
client.

9. Repeat step 3 and notice when you go to your opac summary
(opac-user.pl) page the holds are displayed. .i.e. You can now
successfully place item level holds in the OPAC.

10. Return to the opac-reserve.pl page for the biblio and notice that
because you have already placed a hold on this biblio the 'Next
available item' and 'A specific item' radiobuttons are disabled.

11. Select several of the items checkboxes to place more item level
holds and press the 'Confirm hold' button.

12. Go to your opac summary (opac-user.pl) page again and notice the
holds you placed in step 11 are shown there.

i.e. you can place item level holds from the OPAC on biblios which have
previously had reserves placed on them.

Sponsored-By: Brimbank Library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84547 [details] [review]
Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl

Previously the selected branch dropdown value was not being successfully retrieved in
opac-reserve.pl, this resulted in opac holds being placed with no pickup
branch being set.

Consequently as no branch was associated with the hold an Ajax error was
thrown when viewing the holds-table in the patron page in the staff
client.

This commit successfully retrieves the selected branch value and so
fixing the Ajax error.

Test plan:
1. Apply all patches on bug report 15565 except this commit

2. Place a hold(s) from opac-reserve.pl.

3. Go to your opac patron summary page (opac-user.pl) and notice
although the holds are successfully displayed they have no 'Pickup
location' value

4. Go to the patron page in the staff client and click on the 'Hold(s)'
tab and notice a popup explaining the datatable Ajax error is loaded

5. Apply this patch

6. Repeat step 3 noticing this time in the
opac-user.pl a 'Pickup location' is displayed for the hold you just
created.

6. Repeat step 4 and notice the hold datatable is successfully loaded
and populated without an Ajax error popup being loaded.

Sponsored-By: Brimbank Library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84548 [details] [review]
Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds

This commit implements changes to opac-reserve.tt and opac-reserve.pl.

1. The change to opac-reserve.tt JS fixes the total_requested() function which returns an output which is always 2 digits higher than the actual number of selected item checkboxes.

i.e. if the 'maxreserves' syspref (limit for the overall
number of reserves allowed by a user) is 4 and a user has no previous
holds and ticks 3 item checkboxes and submits the form.

The 'Maximum number of reserve exceded.' popup message is displayed,
however as they have selected 3 items for reservation this is less than
the maxreserves value of 4.

The popup is displayed because total_requested() JS function is
returning 5 (i.e. 2 on top of the actual number of requested holds).

This commit usbtracts 2 from the number of holds calculated by
total_requested() so that the accurate number of requested holds is
being used to determine whether or not to display the excess holds
popup.

2. When the 'Holds per record (count)' (circulation rule) and
'maxreserves' syspref are the same value (e.g. 4) you can tick 4 item
checkboxes in opac-reserve.pl and submit the form.

The form will submit successfully and to the user everything looks to
have worked however the holds will not be
successfully created.

This is because after the form is submitted to opac-reserve.pl a check is
done:

If 'number of submitted hold requests' + 'number of previous existing holds'
greater than or equal to maxreserves syspref then set the variable
'$canreserve' to 0.

If the $canreserve variable is 0 the form will submit, no warning/error
message is displayed to the user but the holds are not created.

Users should be able to place 4 holds if the maxreserves syspref is 4
(just not more than 4 holds). This commit changes this check to:

If 'number of submitted hold requests' + 'number of previous existing
holds' greater than maxreserves syspref then set the variable '$canreserve' to 0..

i.e. notice we only check for greater than (not equal to).

Which solves the bug of the form submitting and no holds being created
when the maxreserves syspref and 'Holds per record (count)' (circulation
rule) are the same.

Test plan (in two parts to test fix #1 and #2 (above)):

Test plan for fix #1:
1. Apply all patches on this bug report (15565) and follow and confirm
the feature works as described in the test plan in the first commit

2. Set the 'maxreserves' syspref to 3 (and 'Holds per record (count)'
(circulation rule) to 5)

3. Ensure your logged in patron account has no existing reserves

4. In the OPAC go to a biblio page with 3 or more items

5. In the reservation page (opac-reserve.pl) for that biblio select 2
item checkboes to place 2 item level holds and notice the 'Maximum
number of reserve exceded.' popup message is displayed.

As the maxreserves syspref value is 3 you should be able to place 2
holds without exceeding that value

6. Apply this patch

7. Repeat step 5 and notice the form submits without the exceeding popup
message and the holds are successfully placed.

Test plan for fix #2:
1. Apply all patches attached to this bug report (bug 15565) and perform
the test plan in the first commit and verify it works.

2. Set your 'Holds per record (count)' (circulation rule) and
'maxreserves' syspref to the same value 3

3. Go to a biblio in the OPAC with 3 or more items

4. In the opac-reserve.pl reservation interface for that biblio tick 3
item checkboxes to place 3 item level holds and submit the form.

5. Notice the form submits and no warning/error message is displayed

6. In the opac user summary page that is displayed after the reserve
form is submitted notice the holds are not displayed this is because
they have not been placed

7. Apply this patch

8. Repeat step 4 and 5 and notice the holds have been successfully
placed and are displayed in your opac user summary page.

Sponsored-By: Brimbank Library
Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84549 [details] [review]
Bug 15565: (follow-up) Removed leftover debug code and added missing filters

Sponsord-By: Brimbank Library, Australia

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84550 [details] [review]
Bug 15565: (QA follow-up) Fix some last filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84551 [details] [review]
Bug 15565: (follow-up) Changes to maximum hold allowed logic

Implemented changes to the logic to address Katrin Fischer's points in
comment #175, also changed the modal box wording to use 'hold' not
'reserve'

Also updated t/db_dependent/Reserves/MultiplePerRecord.t test to reflect
moving function.

Added the display of the JS modal box informing OPAC borrower that
they have exceeded hold limit after they click 'A specific item'
radiobutton on opac-reserve if holds per day, holds per record or
maxreserves is 1

Test plan:
1. Apply patches
2. Restart memcached and plack
3. Run t/db_dependent/Reserves/MultiplePerRecord.t
4. Set your maxreserves syspref to 3, and a circ rules holds per record
to 2 and holds per day to 1
5. In the OPAC visit the 'Place hold' interface of item. Select the 'A
specific item' radiobutton and observe the first item checkbox is
checked and other unchecked checkboxes are disabled.
6. Repeat step 4 this time setting holds per day to 5
7. Now reloading the OPAC 'Place hold' page notice after clicking 'A
specific item' no modal displays
8. Select a second item checkbox and observe a modal saying you have reached
the maximum number of holds for the record is loaded - notice the
wording 'hold' not 'reserve' in use in the modal box
9. Click 'OK' on modal and observe all unchecked item checkboxes are
automatically disabled to prevent additional holds being placed
10. Select 'Confirm hold'
11. On your holds summary page confirm both holds are placed
12. In the staff client set the 'AllowHoldPolicyOverride' syspref to
"Don't allow"
13. Visit a biblio reservation interface in the staff client
14. Select 2 item checkboxes and observe the modal box explaining you
have reached maximum holds for the record is displayed - notice the
wording of 'hold' not 'reserve' is used in the modal box.
15. Select 'OK' on the modal and notice all un-checked item checkboxes
are automatically disabled
16. Repeat steps 12-15 this time with the value of
'AllowHoldPolicyOverride' syspref set to 'Allow' and notice that once
you have clicked on the second item checkbox although the modal still
loads the item checkboxes do not disable - you are able to override and
continue selecting checkboxes each time with modal warning loading.

17. In both the OPAC and staff client try placing a record level hold,
selecting a value from the 'Holds to place (count)' dropdown. Notice
that the maximum value of this is the lowest value out of the 'maximum
number of holds per record', 'maximum number of holds per day' and the
'maxreserves' system preference so in this case it will be 2
18. Save and notice 2 holds are placed

19. Observe the allow_holds() function is in the Koha/Biblio.pm file and
the GetAllowedHoldsForPatronToday() function has been removed from
C4/Reserves.pm

Sponsored-By: Brimbank Library, Australia

Created attachment 84552 [details] [review]
Bug 15565: (follow-up) Place multiple holds from opac-search.pl

Previously before this patch if you selected multiple bibliographic
results from opac-search.pl and selected 'Place hold' and confirmed the
hold only 1 hold on the first of the selected biblios would be placed.

This patch fixes this bug and enables users to be able to place holds on
multiple bibliographic records in opac-search.pl, select 'Place hold', select if they want
each of these holds to be on 'Next available', 'Next available from
group', or 'A specific item' and place their holds and all the holds
they placed will be placed correctly.

Test plan:
1. Follow test plan on the last patch - (Don't apply this patch)
2. Try to select multiple bibliographic record checkboxes and select
'Place hold'
3. Confirm holds and notice you are re-directed to opac-user.pl and only
the first hold has been placed

4. Apply this patch
5. Restart memcached and plack if running
6. Repeat steps 2 and 3 and notice all holds were placed
7. Repeat step 2 and then select a variety of 'A specific item' and 'Next
available item' holds for each of the bibliographic holds and
confirm they are all placed correctly
8. Confirm you can still place multiple holds correctly in opac-reserve
accessing from a OPAC biblio detail page

Sponsored-By: Brimbank Library, Australia

Patches have been rebased. Working on improving the code and including design/layout changes that were requested by Brimbank.

Created attachment 84553 [details] [review]
Bug 15565: (follow-up) fixing typo that breaks placing a hold

Created attachment 87979 [details] [review]
Bug 15565: Place multiple holds at once for the same record on staff and OPAC.

Test plan:
1. Create an circulation rule to use for testing the patches on this bug
report. Ensure it has a value greater than 1 for 'Holds
allowed', 'Holds per record (count)', and 'Item level holds' is enabled.

2. Also ensure the 'RequestOnOpac' syspref is set to 'Allow'.

3. Go to the reservation page (request.pl) of a biblio in the staff client with more
than one item.

4. Notice beside each item a radio button is displayed preventing you
from being able to select multiple items to place a hold on.

5. Go to the reservation page (opac-reserve.pl) of the same biblio in the OPAC
also notice a radio button is displayed beside every item.

6. Apply patches

7. Repeat step 3 and 5 and notice in both the staff client and OPAC
checkboxes rather than radio buttons are displayed beside each item so
you can select multiple items for reservation at a time.

8. From staff interface, place several item-level holds for the same user
on the same record at once:

  - check that you are warned if you exceed "Holds per record (count)"
(circulation rules) for this record.
  - check also maxReserves system preference.

9. From opac, place several item-level and record level holds
for the same user on the same record at once:

  - check that can't exceed "Holds per record (count)" for
  this record.
  This is enforced by all unticked item checkboxes being
  disabled after the number of ticked checkboxes equals the 'Holds per record (count)' (circulation rules) value.

  - check also maxReserves system preference.

Do the same in OPAC and staff by selecting several
record (multi-hold).

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87980 [details] [review]
Bug 15565: Use item's biblionumber only on item-level request

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87981 [details] [review]
Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off

Signed-off-by: Danielle Elder <danielle@bywatersolution.com>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87982 [details] [review]
Bug 15565: Alert librarian that maximum holds per record is reached

Signed-off-by: Dani Elder <dani@bywatersolutions.com>

Signed-off-by: Danielle Elder <danielle@bywatersolution.com>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87983 [details] [review]
Bug 15565: Remove use of GetBiblionumberFromItemnumber

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87984 [details] [review]
Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC

This commit fixes two bugs:

1. C4::Reserves->CanItemBeReserved() returns an output in a hash in the format: status => 'output'

However opac-reserve.pl is not accessing th output of this function
correctly consequently no item is ever reservable.

And so after a patron submits the reservation form on opac-reserve.pl it
looks like the item-level reservation has successfully been placed
however it has not and no error message is displayed to them.

This commit stores the output of the aforementioned function in a hash
and then checks the value of the status key. As a result opac-reserve.pl
now correctly accesses the value returned and item level holds are
successfully placed from the OPAC.

2. If a biblio has previously been reserved then the opac-reserve.pl page
shows the 'Next available item' and 'A specific item' radio buttons to be
disabled to ensure patrons cannot place a reservation of a different
type on the biblio.

With both of these radio buttons being disabled it results in item level holds
placed in the OPAC on a previously reserved item not being stored in the
database. This is very confusing for users.

I have implemented a conditional which checks what the forced_hold_type
of the previous hold was. If it was 'item' then the $reqtype =
'Specific' and if it was 'record' then $reqtype is 'Any'.

This means there will always be a value for $reqtype, and this variable
is checked if it is equal to 'Specific' and if it is then an item level
hold is placed.

Test plan:
1. Ensure in your circulation rules that 'Item level holds' is enabled

2. Apply all patches on this bug report except this commit

3. In the OPAC try placing a item level hold (and notice you can submit
the opac-reserve.pl page and no error messages are displayed. However
the hold(s) are not being placed.

4. Place an item level hold(s) on the biblio from staff client interface
and then return to the opac-reserve.pl page for the biblio in the OPAC
and notice both the 'Next available' and 'A specific item' radio buttons
are disabled.

5. Again try placing an item level hold in the OPAC and notice dispite
no error/warning message being displayed the hold is not being placed.

6. Apply this patch

7. Restart memcached, and plack

8. In the staff client remove the reservation you placed from the staff
client.

9. Repeat step 3 and notice when you go to your opac summary
(opac-user.pl) page the holds are displayed. .i.e. You can now
successfully place item level holds in the OPAC.

10. Return to the opac-reserve.pl page for the biblio and notice that
because you have already placed a hold on this biblio the 'Next
available item' and 'A specific item' radiobuttons are disabled.

11. Select several of the items checkboxes to place more item level
holds and press the 'Confirm hold' button.

12. Go to your opac summary (opac-user.pl) page again and notice the
holds you placed in step 11 are shown there.

i.e. you can place item level holds from the OPAC on biblios which have
previously had reserves placed on them.

Sponsored-By: Brimbank Library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87985 [details] [review]
Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl

Previously the selected branch dropdown value was not being successfully retrieved in
opac-reserve.pl, this resulted in opac holds being placed with no pickup
branch being set.

Consequently as no branch was associated with the hold an Ajax error was
thrown when viewing the holds-table in the patron page in the staff
client.

This commit successfully retrieves the selected branch value and so
fixing the Ajax error.

Test plan:
1. Apply all patches on bug report 15565 except this commit

2. Place a hold(s) from opac-reserve.pl.

3. Go to your opac patron summary page (opac-user.pl) and notice
although the holds are successfully displayed they have no 'Pickup
location' value

4. Go to the patron page in the staff client and click on the 'Hold(s)'
tab and notice a popup explaining the datatable Ajax error is loaded

5. Apply this patch

6. Repeat step 3 noticing this time in the
opac-user.pl a 'Pickup location' is displayed for the hold you just
created.

6. Repeat step 4 and notice the hold datatable is successfully loaded
and populated without an Ajax error popup being loaded.

Sponsored-By: Brimbank Library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87986 [details] [review]
Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds

This commit implements changes to opac-reserve.tt and opac-reserve.pl.

1. The change to opac-reserve.tt JS fixes the total_requested() function which returns an output which is always 2 digits higher than the actual number of selected item checkboxes.

i.e. if the 'maxreserves' syspref (limit for the overall
number of reserves allowed by a user) is 4 and a user has no previous
holds and ticks 3 item checkboxes and submits the form.

The 'Maximum number of reserve exceded.' popup message is displayed,
however as they have selected 3 items for reservation this is less than
the maxreserves value of 4.

The popup is displayed because total_requested() JS function is
returning 5 (i.e. 2 on top of the actual number of requested holds).

This commit usbtracts 2 from the number of holds calculated by
total_requested() so that the accurate number of requested holds is
being used to determine whether or not to display the excess holds
popup.

2. When the 'Holds per record (count)' (circulation rule) and
'maxreserves' syspref are the same value (e.g. 4) you can tick 4 item
checkboxes in opac-reserve.pl and submit the form.

The form will submit successfully and to the user everything looks to
have worked however the holds will not be
successfully created.

This is because after the form is submitted to opac-reserve.pl a check is
done:

If 'number of submitted hold requests' + 'number of previous existing holds'
greater than or equal to maxreserves syspref then set the variable
'$canreserve' to 0.

If the $canreserve variable is 0 the form will submit, no warning/error
message is displayed to the user but the holds are not created.

Users should be able to place 4 holds if the maxreserves syspref is 4
(just not more than 4 holds). This commit changes this check to:

If 'number of submitted hold requests' + 'number of previous existing
holds' greater than maxreserves syspref then set the variable '$canreserve' to 0..

i.e. notice we only check for greater than (not equal to).

Which solves the bug of the form submitting and no holds being created
when the maxreserves syspref and 'Holds per record (count)' (circulation
rule) are the same.

Test plan (in two parts to test fix #1 and #2 (above)):

Test plan for fix #1:
1. Apply all patches on this bug report (15565) and follow and confirm
the feature works as described in the test plan in the first commit

2. Set the 'maxreserves' syspref to 3 (and 'Holds per record (count)'
(circulation rule) to 5)

3. Ensure your logged in patron account has no existing reserves

4. In the OPAC go to a biblio page with 3 or more items

5. In the reservation page (opac-reserve.pl) for that biblio select 2
item checkboes to place 2 item level holds and notice the 'Maximum
number of reserve exceded.' popup message is displayed.

As the maxreserves syspref value is 3 you should be able to place 2
holds without exceeding that value

6. Apply this patch

7. Repeat step 5 and notice the form submits without the exceeding popup
message and the holds are successfully placed.

Test plan for fix #2:
1. Apply all patches attached to this bug report (bug 15565) and perform
the test plan in the first commit and verify it works.

2. Set your 'Holds per record (count)' (circulation rule) and
'maxreserves' syspref to the same value 3

3. Go to a biblio in the OPAC with 3 or more items

4. In the opac-reserve.pl reservation interface for that biblio tick 3
item checkboxes to place 3 item level holds and submit the form.

5. Notice the form submits and no warning/error message is displayed

6. In the opac user summary page that is displayed after the reserve
form is submitted notice the holds are not displayed this is because
they have not been placed

7. Apply this patch

8. Repeat step 4 and 5 and notice the holds have been successfully
placed and are displayed in your opac user summary page.

Sponsored-By: Brimbank Library
Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87987 [details] [review]
Bug 15565: (follow-up) Removed leftover debug code and added missing filters

Sponsord-By: Brimbank Library, Australia

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87988 [details] [review]
Bug 15565: (QA follow-up) Fix some last filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 87989 [details] [review]
Bug 15565: (follow-up) Changes to maximum hold allowed logic

Implemented changes to the logic to address Katrin Fischer's points in
comment #175, also changed the modal box wording to use 'hold' not
'reserve'

Also updated t/db_dependent/Reserves/MultiplePerRecord.t test to reflect
moving function.

Added the display of the JS modal box informing OPAC borrower that
they have exceeded hold limit after they click 'A specific item'
radiobutton on opac-reserve if holds per day, holds per record or
maxreserves is 1

Test plan:
1. Apply patches
2. Restart memcached and plack
3. Run t/db_dependent/Reserves/MultiplePerRecord.t
4. Set your maxreserves syspref to 3, and a circ rules holds per record
to 2 and holds per day to 1
5. In the OPAC visit the 'Place hold' interface of item. Select the 'A
specific item' radiobutton and observe the first item checkbox is
checked and other unchecked checkboxes are disabled.
6. Repeat step 4 this time setting holds per day to 5
7. Now reloading the OPAC 'Place hold' page notice after clicking 'A
specific item' no modal displays
8. Select a second item checkbox and observe a modal saying you have reached
the maximum number of holds for the record is loaded - notice the
wording 'hold' not 'reserve' in use in the modal box
9. Click 'OK' on modal and observe all unchecked item checkboxes are
automatically disabled to prevent additional holds being placed
10. Select 'Confirm hold'
11. On your holds summary page confirm both holds are placed
12. In the staff client set the 'AllowHoldPolicyOverride' syspref to
"Don't allow"
13. Visit a biblio reservation interface in the staff client
14. Select 2 item checkboxes and observe the modal box explaining you
have reached maximum holds for the record is displayed - notice the
wording of 'hold' not 'reserve' is used in the modal box.
15. Select 'OK' on the modal and notice all un-checked item checkboxes
are automatically disabled
16. Repeat steps 12-15 this time with the value of
'AllowHoldPolicyOverride' syspref set to 'Allow' and notice that once
you have clicked on the second item checkbox although the modal still
loads the item checkboxes do not disable - you are able to override and
continue selecting checkboxes each time with modal warning loading.

17. In both the OPAC and staff client try placing a record level hold,
selecting a value from the 'Holds to place (count)' dropdown. Notice
that the maximum value of this is the lowest value out of the 'maximum
number of holds per record', 'maximum number of holds per day' and the
'maxreserves' system preference so in this case it will be 2
18. Save and notice 2 holds are placed

19. Observe the allow_holds() function is in the Koha/Biblio.pm file and
the GetAllowedHoldsForPatronToday() function has been removed from
C4/Reserves.pm

Sponsored-By: Brimbank Library, Australia

Created attachment 87990 [details] [review]
Bug 15565: (follow-up) Place multiple holds from opac-search.pl

Previously before this patch if you selected multiple bibliographic
results from opac-search.pl and selected 'Place hold' and confirmed the
hold only 1 hold on the first of the selected biblios would be placed.

This patch fixes this bug and enables users to be able to place holds on
multiple bibliographic records in opac-search.pl, select 'Place hold', select if they want
each of these holds to be on 'Next available', 'Next available from
group', or 'A specific item' and place their holds and all the holds
they placed will be placed correctly.

Test plan:
1. Follow test plan on the last patch - (Don't apply this patch)
2. Try to select multiple bibliographic record checkboxes and select
'Place hold'
3. Confirm holds and notice you are re-directed to opac-user.pl and only
the first hold has been placed

4. Apply this patch
5. Restart memcached and plack if running
6. Repeat steps 2 and 3 and notice all holds were placed
7. Repeat step 2 and then select a variety of 'A specific item' and 'Next
available item' holds for each of the bibliographic holds and
confirm they are all placed correctly
8. Confirm you can still place multiple holds correctly in opac-reserve
accessing from a OPAC biblio detail page

Sponsored-By: Brimbank Library, Australia

Created attachment 87991 [details] [review]
Bug 15565: (follow-up) fixing typo that breaks placing a hold

Rebased, ready for testing

Looks like Owen set this to "Patch doesn't apply" back on 2019-07-31, and I can confirm that is the case now in 2020-12-18.

Would it be possible to get squash these patches? I think rebasing 13 patches where the first one is failing is going to be not a very pleasurable task.

It looks like these are the files changed:

Biblio.pm
MultiplePerRecord.t
Reserves.pm
opac-reserve.pl
opac-reserve.tt
opac.scss
placerequest.pl
request.pl
request.tt

(In reply to David Cook from comment #223)
> Would it be possible to get squash these patches? I think rebasing 13
> patches where the first one is failing is going to be not a very pleasurable
> task.
> 
> It looks like these are the files changed:
> 
> Koha::Biblio.pm
> MultiplePerRecord.t
> Reserves.pm
> opac-reserve.pl
> opac-reserve.tt
> opac.scss
> placerequest.pl
> request.pl
> request.tt

Trying to find a base to merge these into easily so I can squash them and it's tough. 18.11, 19.05, 19.11... I never knew that Koha's holds code was changed so frequently. 

Aleisha, even if you don't squash them, could you let me know the parent commit for these patches from your last rebase?

(In reply to David Cook from comment #225)
> Aleisha, even if you don't squash them, could you let me know the parent
> commit for these patches from your last rebase?

See https://gist.github.com/jajm/f0b77da1a41b6b1c5ab1

(In reply to Jonathan Druart from comment #226)
> (In reply to David Cook from comment #225)
> > Aleisha, even if you don't squash them, could you let me know the parent
> > commit for these patches from your last rebase?
> 
> See https://gist.github.com/jajm/f0b77da1a41b6b1c5ab1

I've reviewed it based on your suggestion, but it doesn't actually work. At least not with git 2.16.4. 

The git rev-list appears to be random, so returning the first result is like picking a name out of a hat. This can be verified by comparing the results of the following command:

`git rev-list -n 15 --before="'Tue, 16 Apr 2019 23:00:05 +0000'" origin/master | less`

Even if the list weren't random, the dates are from patch creation time rather than merge time, so it would still be a shot in the dark.

It's actually quite interesting...

I just tried `git log --before="2019-04-16"`, which looks good at a glance, but it doesn't include f668fecec3771b633068f89dcead2312471f9425. Yet, f668fecec3771b633068f89dcead2312471f9425 should be before "2019-04-16".

Something weird going on there.

We can trust that commit 1b5635bebd75c418fdda68036e1e91f857cb1e4d was probably made and applied on May 30 2019 as it includes the release notes for 19.05.00.

Aleisha posted a comment about patches being rebased on April 15 2019, but the patches conflict with commits from April 11 2019...

But then if I go before those I'm still getting lots of conflicts.

Without that parent commit or a tonne of work, I don't think anyone can rebase these except Aleisha.

I don't have time to get to this right now and probably won't for a while, but it is on my list.

(In reply to Aleisha Amohia from comment #230)
> I don't have time to get to this right now and probably won't for a while,
> but it is on my list.

I'm happy to collaborate on it as it's increased in priority here. 

I was just being grumpy before, but I can try rebasing it now against master. 

I'm off from Dec 25 through Jan 3, so I suppose there is no rush, but I'm hoping to tick this one off sooner rather than later.

I've put in some effort into rebasing these patches (and skipping many of them), but there's so many different changes that it's difficult to keep it all straight and the existing test plans are chaos.

Rather than proceeding with this patch, I reckon that it's smarter to split this patch into separate patches that deal with 1 change at a time (e.g. adding checkboxes to the staff client, adding checkboxes to the OPAC, changing maximum hold logic, etc.)

I think that Bug 23457 is a good candidate for adding checkboxes to the staff client, and I think that's probably where I'm going to focus my energy.

On the plus side, rebasing actually exposed a bug in master! So yay!

(Apologies if my above comments came across as rude or disrespectful. I'm not trying to criticize anyone. I'm just really keen to move forward, as I have a library that really wants multiple holds at once functionality ASAP, and I think breaking up the patches might be a good way to move forward.)

Patches applied correctly for me:

% git log --before="2019-04-16" --oneline|head -n1
7e218b81e6 Bug 21953: (follow-up) Fix test count

% git checkout -b bug_15565 7e218b81e6
% yes|git bz apply 15565

Bug 15565 - Place multiple holds at once for the same record in OPAC

87979 - Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
87980 - Bug 15565: Use item's biblionumber only on item-level request
87981 - Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off
87982 - Bug 15565: Alert librarian that maximum holds per record is reached
87983 - Bug 15565: Remove use of GetBiblionumberFromItemnumber
87984 - Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC
87985 - Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl
87986 - Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds
87987 - Bug 15565: (follow-up) Removed leftover debug code and added missing filters
87988 - Bug 15565: (QA follow-up) Fix some last filters
87989 - Bug 15565: (follow-up) Changes to maximum hold allowed logic
87990 - Bug 15565: (follow-up) Place multiple holds from opac-search.pl
87991 - Bug 15565: (follow-up) fixing typo that breaks placing a hold

Applying: Bug 15565: Place multiple holds at once for the same record on staff and OPAC.
Applying: Bug 15565: Use item's biblionumber only on item-level request
Applying: Bug 15565: Set 'found' parameter correctly when ReservesNeedReturns is off
Applying: Bug 15565: Alert librarian that maximum holds per record is reached
Applying: Bug 15565: Remove use of GetBiblionumberFromItemnumber
Applying: Bug 15565: Fixed issues preventing any item-level holds being placed in OPAC
Applying: Bug 15565: Changed the retrieval of selected hold pickup branch in opac-reserve.pl
Applying: Bug 15565: Allow up to (and including) maxreserves number of OPAC item levelholds
Applying: Bug 15565: (follow-up) Removed leftover debug code and added missing filters
Applying: Bug 15565: (QA follow-up) Fix some last filters
Applying: Bug 15565: (follow-up) Changes to maximum hold allowed logic
Applying: Bug 15565: (follow-up) Place multiple holds from opac-search.pl
Applying: Bug 15565: (follow-up) fixing typo that breaks placing a hold
Apply? [(y)es, (n)o, (i)nteractive] %             


Pushed to a remote branch if needed: https://gitlab.com/joubu/Koha/-/tree/bug_15565

Thanks, Jonathan. I haven't looked at it yet, but I'm hoping I'll get the time to do so at some point.

I have a bit of time now, tackling this again. I'll try and fix the patches so they are easier to work with.

Thanks, Aleisha! We're very interested in this.

-Martu Fuerst, Huntsville-Madison County Public Library

(In reply to Aleisha Amohia from comment #237)
> I have a bit of time now, tackling this again. I'll try and fix the patches
> so they are easier to work with.

Awesome! Feel free to ping me when you're ready and I'll happily test.

Created attachment 117128 [details] [review]
Bug 15565: Koha::Biblio->allowed_holds and tests

Created attachment 117129 [details] [review]
Bug 15565: Place multiple holds on one record on staff client

Test plan:
1) Apply patch and restart services
2) Set the maxreserves system preference to 3
3) Set the following circulation rules:
holds allowed (total) = 10
holds allowed (daily) = 10
holds per record (count) = 10
3) Set up a record with 4 items
4) Go to place a hold on this record
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox unchecks itself.
7) Try setting different values for the above circulation rules that are
lower than the number of items on this record. Confirm the alert pops up
as expected.
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Confirm that item-level holds are placed on all selected items.

Sponsored-by: Catalyst IT

Created attachment 117130 [details] [review]
Bug 15565: Place multiple holds on one record on OPAC

Test plan:
1) Apply patch and restart services
2) Set the maxreserves system preference to 3
3) Set the following circulation rules:
holds allowed (total) = 10
holds allowed (daily) = 10
holds per record (count) = 10
3) Set up a record with 4 items
4) Go to place a hold on this record
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox unchecks itself.
7) Try setting different values for the above circulation rules that are
lower than the number of items on this record. Confirm the alert pops up
as expected.
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Confirm that item-level holds are placed on all selected items.

Sponsored-by: Catalyst IT

Created attachment 117131 [details] [review]
Bug 15565: Koha::Biblio->allowed_holds and tests

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 117132 [details] [review]
Bug 15565: Place multiple holds on one record on staff client

Test plan:
1) Apply patch and restart services
2) Set the maxreserves system preference to 3
3) Set the following circulation rules:
holds allowed (total) = 10
holds allowed (daily) = 10
holds per record (count) = 10
3) Set up a record with 4 items
4) Go to place a hold on this record
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox unchecks itself.
7) Try setting different values for the above circulation rules that are
lower than the number of items on this record. Confirm the alert pops up
as expected.
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Confirm that item-level holds are placed on all selected items.

Sponsored-by: Catalyst IT

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 117133 [details] [review]
Bug 15565: Place multiple holds on one record on OPAC

Test plan:
1) Apply patch and restart services
2) Set the maxreserves system preference to 3
3) Set the following circulation rules:
holds allowed (total) = 10
holds allowed (daily) = 10
holds per record (count) = 10
3) Set up a record with 4 items
4) Go to place a hold on this record
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox unchecks itself.
7) Try setting different values for the above circulation rules that are
lower than the number of items on this record. Confirm the alert pops up
as expected.
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Confirm that item-level holds are placed on all selected items.

Sponsored-by: Catalyst IT

Signed-off-by: David Cook <dcook@prosentient.com.au>

Looking good, Aleisha!

If someone else signs this off, I could potentially QA it...

I've tried testing this using the Bywater sandboxes, but I can't get the pop-up to show up with the current test plan (step 6). When I set maxreserves to 3, I still don't get an alert when trying to place 4 holds, but only 3 holds are placed.

(In reply to Martha Fuerst from comment #248)
> I've tried testing this using the Bywater sandboxes, but I can't get the
> pop-up to show up with the current test plan (step 6). When I set
> maxreserves to 3, I still don't get an alert when trying to place 4 holds,
> but only 3 holds are placed.

I'll test this one again when I have a chance.

Created attachment 120365 [details] [review]
Bug 15565: Koha::Biblio->allowed_holds and tests

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 120366 [details] [review]
Bug 15565: Place multiple holds on one record on staff client

Test plan:
1) Apply patch and restart services
2) Set the maxreserves system preference to 3
3) Set the following circulation rules:
holds allowed (total) = 10
holds allowed (daily) = 10
holds per record (count) = 10
3) Set up a record with 4 items
4) Go to place a hold on this record
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox unchecks itself.
7) Try setting different values for the above circulation rules that are
lower than the number of items on this record. Confirm the alert pops up
as expected.
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Confirm that item-level holds are placed on all selected items.

Sponsored-by: Catalyst IT

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 120367 [details] [review]
Bug 15565: Place multiple holds on one record on OPAC

Test plan:
1) Apply patch and restart services
2) Set the maxreserves system preference to 3
3) Set the following circulation rules:
holds allowed (total) = 10
holds allowed (daily) = 10
holds per record (count) = 10
3) Set up a record with 4 items
4) Go to place a hold on this record
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox unchecks itself.
7) Try setting different values for the above circulation rules that are
lower than the number of items on this record. Confirm the alert pops up
as expected.
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Confirm that item-level holds are placed on all selected items.

Sponsored-by: Catalyst IT

Signed-off-by: David Cook <dcook@prosentient.com.au>

Slightly updated test plan. Everything is working well in koha-testing-docker.

Test plan:
0) git bz apply 15565
1) restart_all
2) Set the maxreserves system preference to 3 (from 50)
3) Set the following circulation rules:
holds allowed (total) = 10 (from unlimited)
holds allowed (daily) = 10 (from unlimited)
holds per record (count) = 10 (from 2)
3) Set up a record with 4 items
E.g. add 3 items to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29
4) Go to place a hold on this record in Staff Interface
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox is not checked.
7) Set the following circulation rules:
holds allowed (total) = 2 (from 10)
holds allowed (daily) = 2 (from 10)
holds per record (count) = 2 (from 10)
8) Perform the same above actions on the OPAC

Going to set this back to "Needs Signoff" so it's clearer that someone other than me should test it.

(In reply to David Cook from comment #254)
> Going to set this back to "Needs Signoff" so it's clearer that someone other
> than me should test it.

But FYI it is passing the QA tools:

root@kohadevbox:koha(bug_15565)$ qa
testing 3 commit(s) (applied to 640ba47 'cf Bug 28288: Prevent XSISBN.t to fai')

Processing files before patches
|========================>| 7 / 7 (100.00%)
Processing files after patches
|========================>| 7 / 7 (100.00%)

 OK     Koha/Biblio.pm
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
 OK     koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
 OK     opac/opac-reserve.pl
 OK     reserve/placerequest.pl
 OK     reserve/request.pl
 OK     t/db_dependent/Reserves/MultiplePerRecord.t

Processing additional checks OK!

Created attachment 120639 [details] [review]
Bug 15565: Koha::Biblio->allowed_holds and tests

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 120640 [details] [review]
Bug 15565: Place multiple holds on one record on OPAC

Test plan: See previous commit about the staff interface

Sponsored-by: Catalyst IT

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 120641 [details] [review]
Bug 15565: Place multiple holds on one record on staff client

Test plan:
0) git bz apply 15565
1) restart_all
2) Set the maxreserves system preference to 3 (from 50)
3) Set the following circulation rules:
holds allowed (total) = 10 (from unlimited)
holds allowed (daily) = 10 (from unlimited)
holds per record (count) = 10 (from 2)
3) Set up a record with 4 items
E.g. add 3 items to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29
4) Go to place a hold on this record in Staff Interface
5) Notice that the items table under 'Place a hold on a specific item'
now uses checkboxes instead of radio buttons
6) Check all 4 checkboxes. Confirm that when you check the 4th checkbox,
    an alert pops up warning the you've reached the maximum. Confirm
    that when you close the alert, the checkbox is not checked.
7) Set the following circulation rules one by one and test that each
one alone can trigger the alert:
holds allowed (total) = 2 (from 10)
holds allowed (daily) = 2 (from 10)
holds per record (count) = 2 (from 10)
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Perform the same above actions on the OPAC

Sponsored-by: Catalyst IT

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

It works! :D

===

Is that expected that Holds allowed (total) isn't taken into account?
        my $lowestvalue = min( $maxreserves, $remaining_holds_for_record, $remaining_holds_allowed_today );

===

I my 1st test, I had a situation where
maxreserves 3
holds allowed (total) = 2
holds allowed (daily) = 2
holds per record (count) = 2
And when I ticked one tick, the alert came.
The patron had 0 holds and so the record.
I did a reset_all and couldn't reproduce.
So there might be something wrong, in case this symptom gives you an hint. If not, let's move on I would say. Can't be blocked by a ghost bug that might not actually exist.

===

Took David's test plan and put it in the commit.
And did the following changes:

7) Set the following circulation rules:
↓↓↓
7) Set the following circulation rules one by one and test that each
one alone can trigger the alert:

8) Perform the same above actions on the OPAC
↓↓↓
8) Set the circulation rules and syspref all to 4+. Check some items and
confirm the holds.
9) Perform the same above actions on the OPAC

===

oops, inverted two patches during a rebase. Anyway, still applies on top of master.

(In reply to Victor Grousset/tuxayo from comment #259)
> Is that expected that Holds allowed (total) isn't taken into account?
>         my $lowestvalue = min( $maxreserves, $remaining_holds_for_record,
> $remaining_holds_allowed_today );

Sounds like a bug to me.

Aleisha?

*** Bug 24309 has been marked as a duplicate of this bug. ***

Updating title from "Place multiple holds at once for the same record in OPAC" to "Place multiple item-level holds at once for the same record in OPAC and staff interface" since I think that's more accurate.

Marking as Failed QA due to Victor's comment about "Holds allowed (total)".

I think I should be more thorough in my testing of the different hold counts too, even if it means making the test plan longer.

Test plan:
0) git bz apply 15565
1) restart_all
2) Set up a record with 4 items
E.g. add 3 items to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29

Staff Interface:
3) Set the maxreserves system preference to 2 (from 50) 
4) Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?borrowernumber=51&biblionumber=29
5) Try to place a hold on all the items

Unfortunately, as soon as I choose 1 item, it says "Patron reached the maximum number of holds". 

It looks like these patches don't work with "Unlimited" holds, so this functionality is fundamentally broken...

Ok so there are 4 different settings that control the allowable number of holds:

maxreserves (global system preference)
"Holds allowed (total)" (circ rule: reservesallowed)
"Holds allowed (daily)" (circ rule: holds_per_day)
"Holds per record (count)" (circ rule: holds_per_record)

C4::Reserves is a mess...

If the following are blank/empty (notably not null), then 15565 doesn't work. 
select * from circulation_rules where rule_name IN ('reservesallowed', 'holds_per_day', 'holds_per_record');

I need to finish up for the day, but... I reckon we need something like this:

Koha::Holds->remaining_reserves_total({ patron => $patron, });
maxreserves (global system preference)
"Holds allowed (total)" (circ rule: reservesallowed)

Koha::Holds->remaining_reserves_record({ patron => $patron, record => $biblio, });
"Holds per record (count)" (circ rule: holds_per_record)

Koha::Holds->remaining_reserves_today({ patron => $patron, record => $biblio, });
"Holds allowed (daily)" (circ rule: holds_per_day)

And each needs to consider an unlimited number (although in theory maxreserves should always be a positive integer).

Then when you're checking boxes you check each of those totals in turn. (The reason for having separate functions is so that you can tell the user what limit they've exceeded...). 

An unlimited rule should probably return a null response, so that you only consider counts that are digits.

Created attachment 121941 [details] [review]
Bug 15565: Work in progress, calculating holds remaining

Not ready for testing!

I think I found another problem.

From the results page, it looks like you can place multiple item holds for multiple bibs, but it doesn't work. It just places a hold on the first item for each bib.

It looks like the "Unlimited" bug happens on the staff interface but not the OPAC...

In the OPAC, you can select multiple specific items for multiple records at the same time from the result page, but not all of your selections make it into the holds. It looks like just the first selection. 

I'm looking at patching this now.

Created attachment 122098 [details] [review]
Bug 15565: Allow multiple hold for multiple bib in OPAC

This patch allows an OPAC user to select multiple item holds
from multiple bibs when placing holds/reserves from the OPAC
search result page.

Getting a merge conflict for "Place multiple holds on one record on staff client"

I'll see if I can work it out..

I fixed the merge conflict but the Javascript in the staff interface doesn't work anymore. Probably due to other changes that have happened to the templates on master.

(In reply to David Cook from comment #273)
> I fixed the merge conflict but the Javascript in the staff interface doesn't
> work anymore. Probably due to other changes that have happened to the
> templates on master.

This is so weird... I swear that I successfully backported to 21.05 but now it's not working...

(In reply to David Cook from comment #274)
> (In reply to David Cook from comment #273)
> > I fixed the merge conflict but the Javascript in the staff interface doesn't
> > work anymore. Probably due to other changes that have happened to the
> > templates on master.
> 
> This is so weird... I swear that I successfully backported to 21.05 but now
> it's not working...

Ah, those Javascript issues are with hold limits. The problem I encountered was due to Bug 28338...

As per my comment at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14695#c242, it looks like multi-hold doesn't properly process Pickup location at the item-level...

Aleisha, do you have clients using this functionality?

(In reply to David Cook from comment #277)
> Aleisha, do you have clients using this functionality?

Catalyst had received sponsorship to work on this for a library years ago, they may still be using a version of this in production but I'm not sure as we don't work with that library anymore and sponsorship has run out.

Looking at this code again, and I think one of the reasons it never got accepted was that it was trying to fix too many sins in the Reserves/Holds area. 

We'd be better off getting rid of placerequest.pl all together. We do have the /api/v1/holds endpoint these days, so it is tempting to actually replace it with some Javascript.

I think I might try to do a quick and dirty server-side version locally, but I think the API-driven workflow is the future for the community.

(In reply to David Cook from comment #279)
> Looking at this code again, and I think one of the reasons it never got
> accepted was that it was trying to fix too many sins in the Reserves/Holds
> area. 
> 
> We'd be better off getting rid of placerequest.pl all together. We do have
> the /api/v1/holds endpoint these days, so it is tempting to actually replace
> it with some Javascript.
> 
> I think I might try to do a quick and dirty server-side version locally, but
> I think the API-driven workflow is the future for the community.

Of course, that's only for the staff-side. We don't have the API endpoint for the OPAC side.

I've rewritten the staff-interface code to be much more minimal yet still effective. Its error handling is patterned off the OPAC, which makes it consistent across the interfaces as well.

(In reply to David Cook from comment #281)
> I've rewritten the staff-interface code to be much more minimal yet still
> effective. Its error handling is patterned off the OPAC, which makes it
> consistent across the interfaces as well.

The difference with the past patches is that we don't get the warnings/restrictions in the UI *before* placing the hold. We only get the warnings about failures *after* trying to place the hold.

I think warning *before* the hold is actually a separate feature.

(In reply to David Cook from comment #282)
> The difference with the past patches is that we don't get the
> warnings/restrictions in the UI *before* placing the hold. We only get the
> warnings about failures *after* trying to place the hold.
> 
> I think warning *before* the hold is actually a separate feature.

I actually don't know if there is a practical bug-free way of doing it *before*, since each individual item can have different rules. There's a complex interplay of rules. 

For instance, item A could have 1 "Holds per record", while item B could have 10 "Holds per record". Trying to pre-emptively calculate holdability on selected holds rather than placed holds... is tough to do without potential for errors.

One could argue that it's unlikely a bib record would have items of different types, but I see it all the time. 

That said, only warning *after* trying to place the holds isn't great, because it might not be obvious which holds were placed and which weren't placed. 

But we can't do much about that while our hold placing scripts use redirects. Replacing those redirects with API calls would work better, because the user would be able to see immediately which holds were placed and which weren't. (But that also involves messing around with the DOM in unfun ways - unless we use Vue.)

Created attachment 162071 [details] [review]
Bug 15565: Place multiple holds on one record on staff interface

Created attachment 162072 [details] [review]
Bug 15565: Tidy reserve/placerequest.pl

Created attachment 162073 [details] [review]
Bug 15565: Place multiple holds on one or more records in OPAC

Created attachment 162074 [details] [review]
Bug 15565: Add 'DisplayMultiItemHolds' system preference to toggle display of multi item holds

These patches allow the display of checkboxes instead of radio buttons for placing specific
item holds in the OPAC and staff interface. This display is controlled by the system preference
'DisplayMultiItemHolds'. When disabled, the hold display is the same as in the past. When enabled,
the user can select multiple items to place holds on at the same time.

If there is an error placing one or more holds, an error will display on the following screen.

Test plan:
0. Apply the patches and koha-plack --reload kohadev
1. Enable 'DisplayMultiItemHolds' syspref
2. In staff interface, go to a record with multiple holdable items
3. Check the boxes next to 2+ items and click the button to place the holds
4. Note that the holds are all placed (unless there is a failure)
5. Cancel the holds, and try again (but this time make Holds per record (count) only
1 in the circ rules)
6. Note that one of the holds is placed, but the others are not and an error message
is displayed saying that one or more holds could not be placed because the holds per
record was exceeded

7. Perform the same above procedure on the OPAC
8. If DisplayMultiPlaceHold is enabled, you can test placing multiple specific item holds
across multiple bibs in the OPAC via the search results
9. If EnableItemGroups and EnableItemGroupHolds is enabled, you can test that you can
either place item group holds or specific item holds. Toggling the checkboxes in the
staff interface will uncheck the item group hold radio button.

I noticed there is a FAIL in the QA tools, but it's not related to this change:

--

testing 4 commit(s) (applied to d130a6c '6b Bug 35962: (bug 35843 follow-up 2)')

Processing files before patches
The PO directory has not been found. There is a problem in your Koha installation. at /kohadevbox/koha/Koha/I18N.pm line 197.
|========================>| 5 / 5 (100.00%)
Processing files after patches
|========================>| 5 / 5 (100.00%)

 FAIL   installer/data/mysql/mandatory/sysprefs.sql
   FAIL   sysprefs_order
                Not blocker: Sysprefs hidelostitems is bad placed (see bug 10610)

 OK     koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt

 OK     koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt

 OK     reserve/placerequest.pl

 OK     reserve/request.pl


Processing additional checks OK!

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt

Created attachment 163052 [details] [review]
Bug 15565: Place multiple holds on one record on staff interface

Created attachment 163053 [details] [review]
Bug 15565: Tidy reserve/placerequest.pl

Created attachment 163054 [details] [review]
Bug 15565: Place multiple holds on one or more records in OPAC

Created attachment 163055 [details] [review]
Bug 15565: Add 'DisplayMultiItemHolds' system preference to toggle display of multi item holds

These patches allow the display of checkboxes instead of radio buttons for placing specific
item holds in the OPAC and staff interface. This display is controlled by the system preference
'DisplayMultiItemHolds'. When disabled, the hold display is the same as in the past. When enabled,
the user can select multiple items to place holds on at the same time.

If there is an error placing one or more holds, an error will display on the following screen.

Test plan:
0. Apply the patches and koha-plack --reload kohadev
1. Enable 'DisplayMultiItemHolds' syspref
2. In staff interface, go to a record with multiple holdable items
3. Check the boxes next to 2+ items and click the button to place the holds
4. Note that the holds are all placed (unless there is a failure)
5. Cancel the holds, and try again (but this time make Holds per record (count) only
1 in the circ rules)
6. Note that one of the holds is placed, but the others are not and an error message
is displayed saying that one or more holds could not be placed because the holds per
record was exceeded

7. Perform the same above procedure on the OPAC
8. If DisplayMultiPlaceHold is enabled, you can test placing multiple specific item holds
across multiple bibs in the OPAC via the search results
9. If EnableItemGroups and EnableItemGroupHolds is enabled, you can test that you can
either place item group holds or specific item holds. Toggling the checkboxes in the
staff interface will uncheck the item group hold radio button.

Created attachment 163092 [details] [review]
Attachment to Bug 15565 - Place multiple item-level holds at once for the same record

Bug 15565: Place multiple holds on one record on staff interface

Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>

Created attachment 163093 [details] [review]
Bug 15565: Place multiple holds on one record on staff interface

Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>

Worked great in all cases. Thanks David!

Created attachment 166005 [details] [review]
Bug 15565: Place multiple holds on one record on staff interface

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

Created attachment 166006 [details] [review]
Bug 15565: Tidy reserve/placerequest.pl

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

Created attachment 166007 [details] [review]
Bug 15565: Place multiple holds on one or more records in OPAC

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

Created attachment 166008 [details] [review]
Bug 15565: Add 'DisplayMultiItemHolds' system preference to toggle display of multi item holds

These patches allow the display of checkboxes instead of radio buttons for placing specific
item holds in the OPAC and staff interface. This display is controlled by the system preference
'DisplayMultiItemHolds'. When disabled, the hold display is the same as in the past. When enabled,
the user can select multiple items to place holds on at the same time.

If there is an error placing one or more holds, an error will display on the following screen.

Test plan:
0. Apply the patches and koha-plack --reload kohadev
1. Enable 'DisplayMultiItemHolds' syspref
2. In staff interface, go to a record with multiple holdable items
3. Check the boxes next to 2+ items and click the button to place the holds
4. Note that the holds are all placed (unless there is a failure)
5. Cancel the holds, and try again (but this time make Holds per record (count) only
1 in the circ rules)
6. Note that one of the holds is placed, but the others are not and an error message
is displayed saying that one or more holds could not be placed because the holds per
record was exceeded

7. Perform the same above procedure on the OPAC
8. If DisplayMultiPlaceHold is enabled, you can test placing multiple specific item holds
across multiple bibs in the OPAC via the search results
9. If EnableItemGroups and EnableItemGroupHolds is enabled, you can test that you can
either place item group holds or specific item holds. Toggling the checkboxes in the
staff interface will uncheck the item group hold radio button.

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

Created attachment 166009 [details] [review]
Bug 15565: (QA follow-up) Add atomic update

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