Bug 29049: Simplify holds priority dropdown logic on request.pl

This patch makes a few changes:
1 - If we are not splitting the queue ( HoldsSplitQueue == 'nothing' ) then
    there are no changes for 'virtual' vs 'actual' numbering
2 - If we are splitting the queue you cannot use the dropdown, so we do not need to process the options
3 - If the hold is 'found' we do not need to process the options
4 - We can simply use the 'last priority' to build the options, we do not need to process in the script
5 - We can use a block to build the options
6 - Remove a stray holds.index

To test:
 1 - Place 5 holds on a bib
 2 - 'Find' two of the holds i.e. check them in to set status to 'transit' or 'waiting'
 3 - View the holds page for the biblio
 4 - Inspect the priorty dropdown for the found holds, note they have options for 1-5 but are disabled
 5 - Note the dropdowns for other holds have options 1-5
 6 - Note options 4&5 in the dropdowns have no effect
 7 - Apply patch
 8 - Inspect the priority dopdowns on the found holds
 9 - Note they only show their found status
10 - Note the other dropdowns only show options 1-3
11 - Test with other HoldsSplitQueue options and HoldsSplitQueueNumbering
12 - When holds queue is split, no dropdowns should be active and should show only their current priority
13 - When  HoldsSplitQueueNumbering is 'virtual' the dropdown should be disabled and correctly count the number of unfound holds in the list

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
JK: Fixed grammar in commit title
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
JD: Remove trailing 'i'

Bug 29049: (QA follow-up) Remove excessive whitespace

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
index 59f33cd..b2325a4 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
@@ -25,8 +25,29 @@
     [% SET first_priority = 0 %]
     [% SET last_priority  = holds.last.priority %]
 
+    [% BLOCK priority_dropdown %]
+        [% SET loop_priority = 1 %]
+        [% WHILE loop_priority <= last_priority %]
+            [% IF this_priority == loop_priority %]
+                <option value="[% loop_priority | html %]" selected="selected">[% loop_priority | html %]</option>
+            [% ELSE %]
+                <option value="[% loop_priority | html %]">[% loop_priority | html %]</option>
+            [% END %]
+            [% loop_priority = loop_priority + 1 %]
+        [% END %]
+    [% END %]
+
+
     [% FOREACH hold IN holds %]
-    [% IF !hold.found && first_priority == 0 %][% first_priority = hold.priority %][% END %]
+    [% IF !hold.found && first_priority == 0 %]
+        [% first_priority = hold.priority %]
+        [% found_holds = loop.index() %]
+    [% END %]
+    [% IF Koha.Preference('HoldsSplitQueueNumbering') == 'actual' %]
+        [% this_priority = hold.priority %]
+    [% ELSE %]
+        [% this_priority = loop.count() - found_holds %]
+    [% END %]
         <tr>
             <td>
                 <input type="hidden" name="reserve_id" value="[% hold.reserve_id | html %]" />
@@ -34,10 +55,17 @@
                 <input type="hidden" name="biblionumber" value="[% hold.biblionumber | html %]" />
                 [% IF Koha.Preference('HoldsSplitQueue') == "nothing" && !hold.found %]
                     <select name="rank-request" class="rank-request" data-hold-id="[% hold.reserve_id | html %]">
+                    [% IF ( CAN_user_reserveforothers_modify_holds_priority ) %]
+                        [% PROCESS priority_dropdown %]
+                    [% ELSE %]
+                        <option value="[% hold.priority | html %]" selected="selected">[% this_priority | html %]</option>
+                    [% END %]
+
+                        <option value="del">del</option>
+                    </select>
                 [% ELSE %]
                     <input type="hidden" name="rank-request" class="rank-request" value="[% hold.priority | html %]" data-hold-id="[% hold.reserve_id | html %]">
                     <select name="rank-request" class="rank-request" disabled="disabled" data-hold-id="[% hold.reserve_id | html %]">
-                [% END %]
                     [% IF ( hold.found ) %]
                         [% IF ( hold.intransit ) %]
                             <option value="T" selected="selected">In transit</option>
@@ -46,42 +74,17 @@
                         [% ELSE %]
                             <option value="W" selected="selected">Waiting</option>
                         [% END %]
+                    [% ELSE %]
+                        <option value="[% hold.priority | html %]" selected="selected">[% this_priority | html %]</option>
                     [% END %]
-
-                    [% IF ( CAN_user_reserveforothers_modify_holds_priority ) %]
-                        [% IF Koha.Preference('HoldsSplitQueueNumbering') == 'actual' %]
-                            [% FOREACH optionloo IN hold.optionloop %]
-                                [% IF ( optionloo.selected ) %]
-                                    <option value="[% optionloo.num | html %]" selected="selected">[% optionloo.num | html %]</option>
-                                [% ELSE %]
-                                    <option value="[% optionloo.num | html %]">[% optionloo.num | html %]</option>
-                                [% END %]
-                            [% END %]
-                        [% ELSE %]
-                            [% SET ranker = 1 %]
-                            [% FOREACH h IN holds %]
-                                [% NEXT IF h.found %]
-                                [% IF ( h.priority == hold.priority ) %]
-                                    <option value="[% h.priority | html %]" selected="selected">[% ranker | html %]</option>
-                                [% ELSE %]
-                                    <option value="[% h.priority | html %]">[% ranker | html %]</option>
-                                [% END %]
-                                [% ranker = ranker + 1 %]
-                            [% END %]
-                        [% END %]
-                    [% ELSIF !hold.found %]
-                        <option value="[% hold.priority | html %]" selected="selected">[% hold.priority | html %]</option>
-                    [% END %]
-
-                    <option value="del">del</option>
-                </select>
+                    </select>
+                [% END %]
             </td>
 
             [% IF ( CAN_user_reserveforothers_modify_holds_priority ) %]
             [% UNLESS hold.found %]
                     [% SET prev_priority  = loop.prev.priority %]
                     [% SET next_priority  = loop.next.priority %]
-                    [% holds.index | html %]
 
                     <td style="white-space:nowrap;">
                         <a title="Move hold up" href="request.pl?action=move&amp;where=up&amp;first_priority=[% first_priority | html %]&amp;last_priority=[% last_priority | html %]&amp;prev_priority=[% prev_priority | html %]&amp;next_priority=[% next_priority | html %]&amp;borrowernumber=[% hold.borrowernumber | html %]&amp;biblionumber=[% hold.biblionumber | html %]&amp;reserve_id=[% hold.reserve_id | html %]&amp;date=[% hold.date | html %]">

diff --git a/reserve/request.pl b/reserve/request.pl
index 6ab8ba7..60f9d83 100755 (executable)
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -659,17 +659,6 @@ foreach my $biblionumber (@biblionumbers) {
     {
         my $priority = $res->priority();
         my %reserve;
-        my @optionloop;
-        for ( my $i = 1 ; $i <= $totalcount ; $i++ ) {
-            push(
-                @optionloop,
-                {
-                    num      => $i,
-                    selected => ( $i == $priority ),
-                }
-            );
-        }
-
         if ( $res->is_found() ) {
             $reserve{'holdingbranch'} = $res->item()->holdingbranch();
             $reserve{'biblionumber'}  = $res->item()->biblionumber();
@@ -702,7 +691,6 @@ foreach my $biblionumber (@biblionumbers) {
         $reserve{'barcode'}        = $res->item() ? $res->item()->barcode() : undef;
         $reserve{'priority'}       = $res->priority();
         $reserve{'lowestPriority'} = $res->lowestPriority();
-        $reserve{'optionloop'}     = \@optionloop;
         $reserve{'suspend'}        = $res->suspend();
         $reserve{'suspend_until'}  = $res->suspend_until();
         $reserve{'reserve_id'}     = $res->reserve_id();