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
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
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
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>
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>
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).
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.
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>
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>
Created attachment 125308 [details] [review] Bug 29049: (QA follow-up) Remove unused variable Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125309 [details] [review] Bug 29049: (QA follow-up) Remove excessive whitespace Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
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>
(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.
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 %]
Created attachment 125397 [details] [review] Bug 29049: Remove reference to optionloop It's no longer used.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05