From 807a787d527cc93b53e5fc953bbf054c0d670b5f Mon Sep 17 00:00:00 2001
From: Owen Leonard <oleonard@myacpl.org>
Date: Mon, 2 May 2022 12:48:32 +0000
Subject: [PATCH] Bug 30579: (follow-up) Update layout to make hold any vs.
 hold item clearer

This patch alters the structure of the hold for a little more in order
to make it clear that "Hold next available" and "Hold specific item" are
two mutually-exclusive options.

To test, apply the patch and rebuild the staff interface CSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client).

Perform the same tests as before:

- Confirm you can successfully place single or multiple items on hold
- Confirm that when placing a single hold the next available options
  are grouped
- Confirm pickup location, specific item type, and number of holds all
  work for next available
- Confirm item level holds save pickup location correctly
---
 .../prog/css/src/staff-global.scss            |   7 +
 .../prog/en/modules/reserve/request.tt        | 190 +++++++++---------
 2 files changed, 103 insertions(+), 94 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss b/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
index 49a5ce8767..b81dea3f1f 100644
--- a/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
+++ b/koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss
@@ -880,6 +880,13 @@ fieldset {
             font-size: 110%;
             font-weight: bold;
             margin-left: 1em;
+
+            label {
+                display: inline-block;
+                float: none;
+                margin: .2em;
+                width: auto;
+            }
         }
 
         li {
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
index a216bc58e4..30d47f645c 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -437,9 +437,9 @@
                 </div>
 
                 [% UNLESS ( multi_hold ) %]
-                    <fieldset class="rows">
-                        <legend>Hold details</legend>
-                        <form action="placerequest.pl" method="post" name="form" id="hold-request-form">
+                    <form action="placerequest.pl" method="post" name="form" id="hold-request-form">
+                        <fieldset class="rows">
+                            <legend>Hold details</legend>
 
                             <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]" />
                             <input type="hidden" name="type" value="str8" />
@@ -463,10 +463,6 @@
                                 <span class="label">Estimated priority:</span>
                                 <strong>[% fixedRank | html %]</strong>
                             </li>
-                            <li>
-                                <label for="holdnotes">Notes:</label>
-                                <textarea id="holdnotes" name="notes" cols="30" rows="1"></textarea>
-                            </li>
                             [% IF ( reserve_in_future ) %]
                                 <li>
                                     <label for="from">Hold starts on date:</label>
@@ -484,52 +480,57 @@
                                 <input name="non_priority" id="non_priority" type="checkbox" />
                                 <span class="hint">A non priority hold doesn't prevent a current checkout from renewing</span>
                             </li>
-                            <fieldset>
-                                <legend>
-                                    <label for="requestany">
-                                        Hold next available item
-                                        [% IF force_hold_level == 'item' %]
-                                            <input type="checkbox" id="requestany" name="request" disabled="true" />
-                                        [% ELSIF force_hold_level == 'record' %]
-                                            <input type="checkbox" id="requestany" checked="checked" value="Any" disabled="true"/>
-                                            <input type="hidden" name="request" value="Any"/>
-                                        [% ELSE %]
-                                            <input type="checkbox" id="requestany" name="request" checked="checked" value="Any" />
-                                        [% END %]
-                                        <input type="hidden" name="alreadyreserved" value="[% alreadyreserved | html %]" />
-                                    </label>
-                                </legend>
-                                <ol>
-                                    <li>
-                                        <label for="pickup">Pickup at:</label>
-                                        <select name="pickup" id="pickup"
-                                                data-biblio-id="[% biblio.biblionumber | html %]"
-                                                data-patron-id="[% patron.borrowernumber | html %]"
-                                                data-pickup-location-source="biblio">
-                                            [% PROCESS options_for_libraries libraries => Branches.pickup_locations({ search_params => { biblio => biblionumber, patron => patron }, selected => pickup }) %]
-                                        </select>
-                                    </li>
-                                    [% IF Koha.Preference('AllowHoldItemTypeSelection') %]
-                                        <li>
-                                            <label for="itemtype">Request specific item type:</label>
-                                            <select name="itemtype" id="itemtype">
-                                                <option value="">Any item type</option>
-                                                [%- FOREACH itemtype IN available_itemtypes %]
-                                                    <option value="[% itemtype | html %]">[% ItemTypes.GetDescription( itemtype ) | html %]</option>
-                                                [%- END %]
-                                            </select>
-                                        </li>
-                                    [% END %]
-                                    [% IF remaining_holds_for_record > 1 %]
-                                        <li>
-                                            <label for="holds_to_place_count">Holds to place (count)</label>
-                                            <input type="text" inputmode="numeric" pattern="[0-9]*" id="holds_to_place_count" name="holds_to_place_count" maxlength="[% remaining_holds_for_record | html %]" value="1" />
-                                        </li>
-                                    [% ELSE %]
-                                        <input type="hidden" name="holds_to_place_count" value="1" />
-                                    [% END %]
-                                </ol>
-                            </fieldset>
+                            <li>
+                                <label for="holdnotes">Notes:</label>
+                                <textarea id="holdnotes" name="notes" cols="30" rows="1"></textarea>
+                            </li>
+                        </ol>
+                    </fieldset>
+
+                    <fieldset class="rows">
+                        <legend>
+                            <label for="requestany">
+                                Hold next available item
+                                [% IF force_hold_level == 'item' %]
+                                    <input type="radio" class="requestany" id="requestany" name="request" disabled="true" />
+                                [% ELSIF force_hold_level == 'record' %]
+                                    <input type="radio" class="requestany" id="requestany" checked="checked" value="Any" disabled="true"/>
+                                    <input type="hidden" name="request" value="Any"/>
+                                [% ELSE %]
+                                    <input type="radio" class="requestany" id="requestany" name="request" checked="checked" value="Any" />
+                                [% END %]
+                                <input type="hidden" name="alreadyreserved" value="[% alreadyreserved | html %]" />
+                            </label>
+                        </legend>
+                        <ol>
+                            <li>
+                                <label for="pickup">Pickup at:</label>
+                                <select name="pickup" id="pickup"
+                                        data-biblio-id="[% biblio.biblionumber | html %]"
+                                        data-patron-id="[% patron.borrowernumber | html %]"
+                                        data-pickup-location-source="biblio">
+                                    [% PROCESS options_for_libraries libraries => Branches.pickup_locations({ search_params => { biblio => biblionumber, patron => patron }, selected => pickup }) %]
+                                </select>
+                            </li>
+                            [% IF Koha.Preference('AllowHoldItemTypeSelection') %]
+                                <li>
+                                    <label for="itemtype">Request specific item type:</label>
+                                    <select name="itemtype" id="itemtype">
+                                        <option value="">Any item type</option>
+                                        [%- FOREACH itemtype IN available_itemtypes %]
+                                            <option value="[% itemtype | html %]">[% ItemTypes.GetDescription( itemtype ) | html %]</option>
+                                        [%- END %]
+                                    </select>
+                                </li>
+                            [% END %]
+                            [% IF remaining_holds_for_record > 1 %]
+                                <li>
+                                    <label for="holds_to_place_count">Holds to place (count)</label>
+                                    <input type="text" inputmode="numeric" pattern="[0-9]*" id="holds_to_place_count" name="holds_to_place_count" maxlength="[% remaining_holds_for_record | html %]" value="1" />
+                                </li>
+                            [% ELSE %]
+                                <input type="hidden" name="holds_to_place_count" value="1" />
+                            [% END %]
                         </ol>
                         <fieldset class="action">
                             [% IF ( patron.borrowernumber ) %]
@@ -542,6 +543,20 @@
                                 [% END %]
                             [% END %]
                         </fieldset>
+                    </fieldset>
+
+                    <hr />
+
+                    <fieldset class="rows">
+                        <legend>
+                            <label>
+                                Place a hold on a specific item
+                                <input type="radio" name="request" class="requestany" />
+                            </label>
+                            [% IF force_hold_level == 'item' %]
+                                <span class="error"><em>Required</em></span>
+                            [% END %]
+                        </legend>
 
                         [% biblio = biblioloop.0 %]
 
@@ -561,13 +576,6 @@
                             [% END %]
                         </ol>
 
-                        <h2 style="padding: 0 1em;">
-                            Place a hold on a specific item
-                            [% IF force_hold_level == 'item' %]
-                                 <span class="error"><em>(Required)</em></span>
-                            [% END %]
-                        </h2>
-
                         <table id="requestspecific">
                             <thead>
                                 <tr>
@@ -602,9 +610,9 @@
                                                         Hold must be record level
                                                     </span>
                                                 [% ELSIF ( itemloo.available ) %]
-                                                    <input type="radio" name="checkitem" value="[% itemloo.itemnumber | html %]" />
+                                                    <input type="radio" name="checkitem" class="requestspecific" value="[% itemloo.itemnumber | html %]" />
                                                 [% ELSIF ( itemloo.override ) %]
-                                                    <input type="radio" name="checkitem" class="needsoverride" value="[% itemloo.itemnumber | html %]" />
+                                                    <input type="radio" name="checkitem" class="needsoverride requestspecific" value="[% itemloo.itemnumber | html %]" />
                                                     <i class="fa fa-exclamation-triangle fa-lg" style="color:gold" title="Requires override of hold policy"/></i>
                                                 [% ELSE %]
                                                     <span class="error">
@@ -647,7 +655,7 @@
                                             </td>
                                             <td>
                                             [% IF (itemloo.pickup_locations_count > 0) %]
-                                                <select name="item_pickup_[% itemloo.itemnumber | html %]" class="pickup_locations" style="width:100%;"
+                                                <select name="item_pickup_[% itemloo.itemnumber | html %]" class="pickup_locations requestspecific" style="width:100%;"
                                                         data-item-id="[% itemloo.itemnumber | html %]"
                                                         data-patron-id="[% patron.borrowernumber | html %]"
                                                         data-pickup-location-source="item">
@@ -753,11 +761,9 @@
                         </table> <!-- /#requestspecific -->
 
                         [% IF hiddencount %]
-                            <form>
-                                <p class="hiddencount">
-                                    <a href="request.pl?biblionumber=[% biblio.biblionumber | uri %]&amp;borrowernumber=[% borrowernumber | uri %]&amp;showallitems=1">Show all items ([% biblio.hiddencount | html %] hidden)</a>
-                                </p>
-                            </form>
+                            <p class="hiddencount">
+                                <a href="request.pl?biblionumber=[% biblio.biblionumber | uri %]&amp;borrowernumber=[% borrowernumber | uri %]&amp;showallitems=1">Show all items ([% biblio.hiddencount | html %] hidden)</a>
+                            </p>
                         [% END # /IF hiddencount %]
 
                             <fieldset class="action">
@@ -771,8 +777,8 @@
                                     [% END %]
                                 [% END # /IF patron %]
                             </fieldset> <!-- /.action -->
-                        </form> <!-- /#hold-request-form -->
-                    </fieldset> <!-- /.rows -->
+                        </fieldset> <!-- /.rows -->
+                    </form> <!-- /#hold-request-form -->
                 [% ELSE # Multi-hold %]
                     <fieldset class="rows">
                         <legend>Hold details</legend>
@@ -803,10 +809,6 @@
                                 [% END %]
                             </li>
 
-                            <li>
-                                <label for="holdnotes">Notes:</label>
-                                <textarea id="holdnotes" name="notes" cols="30" rows="1"></textarea>
-                            </li>
                             <li>
                                 <label for="pickup">Pickup at:</label>
                                 <select name="pickup" id="pickup_multi" data-patron-id="[% patron.borrowernumber | html %]">
@@ -817,7 +819,6 @@
                                 </select>
                             </li>
 
-
                             [% IF ( reserve_in_future ) %]
                                 <li>
                                     <label for="from">Hold starts on date:</label>
@@ -836,6 +837,11 @@
                                 <input name="non_priority" id="non_priority" type="checkbox" />
                                 <span class="hint">A non priority hold doesn't prevent a current checkout from renewing</span>
                             </li>
+
+                            <li>
+                                <label for="holdnotes">Notes:</label>
+                                <textarea id="holdnotes" name="notes" cols="30" rows="1"></textarea>
+                            </li>
                         </ol>
 
 
@@ -943,10 +949,10 @@
                                 [% END %]
                             [% END # /IF patron %]
                         </fieldset> <!-- /.action -->
-                    </form> <!-- /#hold-request-form -->
-                </fieldset> <!-- /.rows -->
-                [% END %]
-                [% END %]
+                    </fieldset> <!-- /.rows -->
+                </form> <!-- /#hold-request-form -->
+                [% END # /UNLESS ( multi_hold ) %]
+            [% END # /UNLESS club OR patron... %]
 
             [% UNLESS ( patron ) %]
                 [% UNLESS borrowers %]
@@ -1256,15 +1262,8 @@
             }
 
 
-            function ToggleHoldsToPlace() {
-                if ( $("#requestany").prop('checked') ) {
-                    $("#holds_to_place_count,#pickup,#itemtype").prop('disabled', false);
-                } else {
-                    $("#holds_to_place_count,#pickup,#itemtype").prop('disabled', true);
-                }
-            }
             ToggleHoldsToPlace();
-            $("#requestany").on('change', function(){
+            $(".requestany").on('change', function(){
                 ToggleHoldsToPlace();
             });
 
@@ -1283,13 +1282,6 @@
                 "sDom": '<"top pager"ilf>t',
             }));
 
-            //Override fieldset styling for dataTables search box
-            $("div.top.pager").css("margin-left","1em");
-            $(".dataTables_filter label").css({
-                "width":"auto",
-                "margin-right":"0em"
-            });
-
             $("#club-request-form").on("submit", function() {
                 let $t = $(this);
                 $('.clubalert, .holdalert').addClass('hide');
@@ -1388,6 +1380,16 @@
             });
         });
 
+        function ToggleHoldsToPlace() {
+            if ( $("#requestany").prop('checked') ) {
+                $("#holds_to_place_count, #pickup, #itemtype").prop('disabled', false);
+                $(".requestspecific").prop("disabled", true );
+            } else {
+                $("#holds_to_place_count, #pickup, #itemtype").prop('disabled', true);
+                $(".requestspecific").prop("disabled", false );
+            }
+        }
+
         function check( table ) {
 
             var msg = "";
-- 
2.20.1