It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype. To make it easier to understand which holds will be trapped in a given circumstance, it would be nice if we could optionally group holds by pickup library and/or itemtype.

Created attachment 68166 [details] [review]
Bug 19469 - Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 68167 [details] [review]
Bug 19469 - Add ability to split view of holds view on record by pickup library and/or itemtype

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 68168 [details] [review]
Bug 19469 - Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 68169 [details] [review]
Bug 19469 - Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 68347 [details] [review]
Bug 19469 - Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 68348 [details] [review]
Bug 19469 - Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 68609 [details] [review]
Bug 19469 - Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>

Created attachment 72304 [details] [review]
Bug 19469 - Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 72305 [details] [review]
Bug 19469 - Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>

Created attachment 72441 [details] [review]
Bug 19469: Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 72442 [details] [review]
Bug 19469: Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 72443 [details] [review]
Bug 19469: (followup) Use patron-title.inc as proposed by bug 18403

This patch makes the new template include file use the patron-title.inc
include to render the patron name.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Comment on attachment 72442 [details] [review]
Bug 19469: Add ability to split view of holds view on record by pickup library and/or itemtype

Review of attachment 72442 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ +1,1 @@
> +[% USE Dumper %]

debug?

(In reply to Josef Moravec from comment #14)
> Comment on attachment 72442 [details] [review] [review]
> Bug 19469: Add ability to split view of holds view on record by pickup
> library and/or itemtype
> 
> Review of attachment 72442 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
> @@ +1,1 @@
> > +[% USE Dumper %]
> 
> debug?

Yep! Followup coming!

Created attachment 72501 [details] [review]
Bug 19469: Remove use of Dumper on request.tt

I think I found few inconsistencies.

Try with HoldsSplitQueue = both and HoldsSplitQueueNumbering = actual.

From this starting point:
https://screenshots.firefox.com/TX15YNJl7t9SF3LD/pro.kohadev.org

1. Last lines of the 2 blocks have the "lowest priority" icon, I would expect only the first one (with priority=9)

2. item-level_itypes=item and all of the items.itype=BK, why do I have the item level holds in the "Any item type" block?

3. If I change one of the priority from x to 9 using the selects, nothing happens (same issue if I change 9 to x).


I will continue to test when this will be fixed.
It is certainly better for the readability to squash the patches.

Created attachment 74260 [details] [review]
Bug 19469: [DO NOT PUSH] Dump of the reserves table

(In reply to Jonathan Druart from comment #18)
> Created attachment 74260 [details] [review] [review]
> Bug 19469: [DO NOT PUSH] Dump of the reserves table

Here is the content of my table "reserves" (on top of sample data).

Created attachment 74328 [details] [review]
Bug 19469: Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 74329 [details] [review]
Bug 19469: Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 74330 [details] [review]
Bug 19469: Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 74331 [details] [review]
Bug 19469: (followup) Use patron-title.inc as proposed by bug 18403

This patch makes the new template include file use the patron-title.inc
include to render the patron name.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 74332 [details] [review]
Bug 19469: Remove use of Dumper on request.tt

Created attachment 74333 [details] [review]
Bug 19469: [DO NOT PUSH] Dump of the reserves table

Created attachment 74334 [details] [review]
Bug 19469 [QA Followup]: Use hold item's itemtype if available, fix priority changing

Thanks for the table data! It made fixing this up much easier.

> 1. Last lines of the 2 blocks have the "lowest priority" icon, I would
> expect only the first one (with priority=9)

This is good and correct behavior. Any number of holds can be set to "lowest priority" at once.
 
> 2. item-level_itypes=item and all of the items.itype=BK, why do I have the
> item level holds in the "Any item type" block?

Good idea! My new followup takes care of this.

> 3. If I change one of the priority from x to 9 using the selects, nothing
> happens (same issue if I change 9 to x).

I've updated the javascript to work with the multiple tables. I decided to let it re-flow the priorities in the affected tables, but we could also simply leave the priorities as they are if we have split holds views. Either solution seem good and correct in its own way. I chose to re-flow to keep consistency with the current behavior.

> I will continue to test when this will be fixed.
> It is certainly better for the readability to squash the patches.

If you would like to squash all the patches and re-upload them as one please do so! If you'd like me to do it instead just let me know!

I wanted to retest but stopped quickly because I either do not understand how it is supposed to work or it is buggy:

I am using the same reserves.sql data, with  HoldsSplitQueue=pickup, HoldsSplitQueueNumbering=actual
On the reserve page, I see that: https://screenshots.firefox.com/DDTe7dEmVOK4regX/pro.kohadev.org

First line of Franklin, I select 9, and I get: https://screenshots.firefox.com/r8FS1f2Q3SFw9Ngg/pro.kohadev.org (without resubmitting the form, just selecting a value in the dropdown list).


Moreover I am wondering why we have the column with "go lowest" and "go bottom" (before the red X to cancel). Is not it redundant with the other 4 arrows column?
Same behaviour in master, just wondering if we should not remove this column on a separate bug report.

There is something else I think:
If you change the order with the select, do not save but click on one of the arrows, it will not do what you expect (i.e. save what you saw just before saving).

> Moreover I am wondering why we have the column with "go lowest" and "go
> bottom" (before the red X to cancel). Is not it redundant with the other 4
> arrows column?
> Same behaviour in master, just wondering if we should not remove this column
> on a separate bug report.

The 'lowest priority' is a separate feature and is not the same as simply moving the hold to the bottom of the queue. It 'pins' the hold to the bottom so any new holds added are automatically put in front of those holds in the queue. We should not remove this column as it is a feature and is not redundant.

I need either follow-ups, second signoff or another QA.
Moving out of PQA, back to "Signed Off"

(In reply to Jonathan Druart from comment #28)
> I am using the same reserves.sql data, with  HoldsSplitQueue=pickup,
> HoldsSplitQueueNumbering=actual
> On the reserve page, I see that:
> https://screenshots.firefox.com/DDTe7dEmVOK4regX/pro.kohadev.org
> 
> First line of Franklin, I select 9, and I get:
> https://screenshots.firefox.com/r8FS1f2Q3SFw9Ngg/pro.kohadev.org (without
> resubmitting the form, just selecting a value in the dropdown list).


I'm a bit confused by these two. So you are using the same setting for the second screenshot, but when editing the first hold of Franklin, instead of showing the actual numbers it reverts to showing separate queue priorities? That does sound like a unwanted behavior, unless I'm missing something...

Created attachment 74909 [details] [review]
Bug 19469 [QA Followup]: When altering priorities, use the existing priority numbers of the given table

(In reply to Jonathan Druart from comment #31)
> I need either follow-ups, second signoff or another QA.
> Moving out of PQA, back to "Signed Off"

Followup posted!

Either it still does not work or it is very confusing (or I do not understand how it is supposed to work).

As we are in feature freeze I am not willing to push this one for 18.05 and would recommend another QA stamp.

I've added his one to my pile to do a second round of qa on.. see if I can verify or otherwise Jonathans concerns.

Kyle can you rebase this for SCSS and bug 13618?

Created attachment 78506 [details] [review]
Bug 19469: Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 78507 [details] [review]
Bug 19469: Clean up template whitespace

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 78508 [details] [review]
Bug 19469: Add ability to split view of holds view on record by pickup library and/or itemtype

It is possible to set up circulation rules to limit trapping of holds by pickup library and itemtype.
To make it easier to understand which holds will be trapped in a given circumstance,
it would be nice if we could optionally group holds by pickup library and/or itemtype.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Enable AllowHoldItemTypeSelection
4) Pick a record and create holds with various pickup libraries and itemtype combinations
5) Enable HoldsSplitQueueNumbering
6) Try the different combinations of HoldsSplitQueue
7) Ensure the hold "arrows" move the items correctly
   * Up and down arrows should move hold above or below the adjacent hold within a hold fieldset
   * Top and borrom arrows should move hold to the top or bottom within a hold fieldset

Sponsored-by: Stockholm University Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 78509 [details] [review]
Bug 19469: (followup) Use patron-title.inc as proposed by bug 18403

This patch makes the new template include file use the patron-title.inc
include to render the patron name.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 78510 [details] [review]
Bug 19469: Remove use of Dumper on request.tt

Created attachment 78511 [details] [review]
Bug 19469: [DO NOT PUSH] Dump of the reserves table

Created attachment 78512 [details] [review]
Bug 19469 [QA Followup]: Use hold item's itemtype if available, fix priority changing

Created attachment 78513 [details] [review]
Bug 19469 [QA Followup]: When altering priorities, use the existing priority numbers of the given table

Created attachment 78526 [details] [review]
Bug 19469: (RM follow-up) Add filters and fix variable name

(In reply to Jonathan Druart from comment #29)
> There is something else I think:
> If you change the order with the select, do not save but click on one of the
> arrows, it will not do what you expect (i.e. save what you saw just before
> saving).

This is a bug in master too, set last hold to priority one via select - then click an arrow

Created attachment 78529 [details] [review]
(RM follow-up) Add filters and fix variable name

Created attachment 78707 [details] [review]
Bug 19469: (QA followup) Disable priority pulldowns when using

Awesome work all!

Pushed to master for 18.11

Enhancement, will not be backported to 18.05.x series.

Links on the "multi holds" view are broken, please fix.

To recreate:
- Search
- Select few records
- Click "Place hold"
=> On "Home › Catalog › Confirm holds" the link of the record does not have a biblionumber value, the link is "/cgi-bin/koha/catalogue/detail.pl?biblionumber="

Created attachment 81828 [details] [review]
Bug 19469: Fix record links on multi holds

(In reply to Jonathan Druart from comment #52)
> Links on the "multi holds" view are broken, please fix.

Ok done, it was an easy fix.

Created attachment 81829 [details] [review]
Bug 19469: Fix record links on multi holds

(In reply to Jonathan Druart from comment #55)
> Created attachment 81829 [details] [review] [review]
> Bug 19469: Fix record links on multi holds

Follow-up pushed to master