Bug 29049 - Holds page shows too many priority options in pulldown
Summary: Holds page shows too many priority options in pulldown
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-17 13:59 UTC by Nick Clemens
Modified: 2021-10-08 12:17 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05


Attachments
Bug 29049: Simply holds priorty dropdown logic on request.pl (8.79 KB, patch)
2021-09-17 14:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29049: (follow-up) Reduce number of line returns in page (16.51 KB, patch)
2021-09-17 16:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29049: Simply holds priorty dropdown logic on request.pl (8.86 KB, patch)
2021-09-20 14:21 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29049: (follow-up) Reduce number of line returns in page (16.58 KB, patch)
2021-09-20 14:21 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29049: Simplify holds priority dropdown logic on request.pl (9.04 KB, patch)
2021-09-25 11:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29049: (follow-up) Reduce number of line returns in page (16.72 KB, patch)
2021-09-25 11:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29049: (QA follow-up) Remove unused variable (874 bytes, patch)
2021-09-25 11:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29049: (QA follow-up) Remove excessive whitespace (1.34 KB, patch)
2021-09-25 11:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29049: (QA follow-up) Initialize uninitialized variable to prevent warning (1.01 KB, patch)
2021-09-25 11:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29049: Remove reference to optionloop (1.44 KB, patch)
2021-09-28 12:18 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-09-17 13:59:17 UTC
To recreate:
1 - Place 5 holds on a biblio
2 - 'Find' two holds i.e. check them in to set them as transit or waiting
3 - Load the holds page for the biblio
4 - Check the pull down for priority
5 - Note holds has priority 1-3 but dropdown is 1-5
6 - Click 4 or 5 and note nothing happens
Comment 1 Nick Clemens 2021-09-17 14:29:42 UTC
Created attachment 124992 [details] [review]
Bug 29049: Simply holds priorty 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
Comment 2 Nick Clemens 2021-09-17 16:36:53 UTC
Created attachment 125013 [details] [review]
Bug 29049: (follow-up) Reduce number of line returns in page

While moving the dropdown to a block, I noticed the page size was very large when there are many holds

This patch chomps a large amoutn of whitespace, from the block and elsewhere and drecreased the paeg from about 400k lines to 25k lines for a record with ~200 holds

To test:
1 - Apply previous patch
2 - Load holds tab for a biblio
3 - Save the page as html from your browser, note the size of the file
4 - Apply patch this
5 - Reload and repeat
6 - Note the size is much smaller
Comment 3 Andrew Fuerste-Henry 2021-09-20 14:21:44 UTC
Created attachment 125067 [details] [review]
Bug 29049: Simply holds priorty 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>
Comment 4 Andrew Fuerste-Henry 2021-09-20 14:21:47 UTC
Created attachment 125068 [details] [review]
Bug 29049: (follow-up) Reduce number of line returns in page

While moving the dropdown to a block, I noticed the page size was very large when there are many holds

This patch chomps a large amoutn of whitespace, from the block and elsewhere and drecreased the paeg from about 400k lines to 25k lines for a record with ~200 holds

To test:
1 - Apply previous patch
2 - Load holds tab for a biblio
3 - Save the page as html from your browser, note the size of the file
4 - Apply patch this
5 - Reload and repeat
6 - Note the size is much smaller

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 Joonas Kylmälä 2021-09-25 10:29:24 UTC
In case anybody else is testing this I was bit confused why i couldn't reproduce the problem at first: turns out we are missing HoldsSplitQueueNumbering from the installer/data/mysql/mandatory/sysprefs.sql file, so on fresh install the bug doesn't happen because we are using the virtual mechanism if none is specified (even though if you go to the systempreferences menu it shows actual).
Comment 6 Joonas Kylmälä 2021-09-25 11:01:11 UTC
This causes the following warning:

> Argument "" isn't numeric in subtraction (-) at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc line 47.

Let me see if I can come up with a fix.
Comment 7 Joonas Kylmälä 2021-09-25 11:17:27 UTC
Created attachment 125306 [details] [review]
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>
Comment 8 Joonas Kylmälä 2021-09-25 11:17:31 UTC
Created attachment 125307 [details] [review]
Bug 29049: (follow-up) Reduce number of line returns in page

While moving the dropdown to a block, I noticed the page size was very large when there are many holds

This patch chomps a large amoutn of whitespace, from the block and elsewhere and drecreased the paeg from about 400k lines to 25k lines for a record with ~200 holds

To test:
1 - Apply previous patch
2 - Load holds tab for a biblio
3 - Save the page as html from your browser, note the size of the file
4 - Apply patch this
5 - Reload and repeat
6 - Note the size is much smaller

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 9 Joonas Kylmälä 2021-09-25 11:17:35 UTC
Created attachment 125308 [details] [review]
Bug 29049: (QA follow-up) Remove unused variable

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 10 Joonas Kylmälä 2021-09-25 11:17:39 UTC
Created attachment 125309 [details] [review]
Bug 29049: (QA follow-up) Remove excessive whitespace

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 11 Joonas Kylmälä 2021-09-25 11:17:43 UTC
Created attachment 125310 [details] [review]
Bug 29049: (QA follow-up) Initialize uninitialized variable to prevent warning

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 12 Jonathan Druart 2021-09-28 11:57:54 UTC
(In reply to Joonas Kylmälä from comment #10)
> Created attachment 125309 [details] [review] [review]
> Bug 29049: (QA follow-up) Remove excessive whitespace
> 
> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Will be squashed.
Comment 13 Jonathan Druart 2021-09-28 11:58:18 UTC
Also will amend first patch to remove a trailing 'i':

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 daf50d69a76..c48987677eb 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
@@ -53,7 +53,7 @@
                 <input type="hidden" name="borrowernumber" value="[% hold.borrowernumber | html %]" />
                 <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 %]">i
+                    <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 %]
Comment 14 Jonathan Druart 2021-09-28 12:18:11 UTC
Created attachment 125397 [details] [review]
Bug 29049: Remove reference to optionloop

It's no longer used.
Comment 15 Jonathan Druart 2021-09-28 13:39:31 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Kyle M Hall 2021-10-08 12:17:07 UTC
 Pushed to 21.05.x for 21.05.05