| Summary: | Poor performance when clicking 'Update hold(s)' on request.pl for records with many holds | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
| Component: | Hold requests | Assignee: | Nick Clemens (kidclamp) <nick> |
| Status: | Pushed to main --- | QA Contact: | Testopia <testopia> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | andrew, bibliothek, david, dcook, gmcharlt, kebliss, lisette, mspinney, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23269 | ||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Comma delimited list of Sponsors: | |
| Crowdfunding goal: | 0 | Patch complexity: | Small patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
26.05.00
|
|
| Circulation function: | |||
| Attachments: |
Bug 41416: Add skip_fix_priority option to ModReserve
Bug 41416: Add skip_fix_priority option to ModReserve Bug 41416: Add skip_fix_priority option to ModReserve |
||
|
Description
Nick Clemens (kidclamp)
2025-12-09 18:58:50 UTC
Created attachment 190383 [details] [review] Bug 41416: Add skip_fix_priority option to ModReserve This patch adds a new parameter to ModReserve: skip_fix_priority The page for altering priority ensures each hold has a unique rank, so doesn't allow for setting two holds to the same rank, so we don't need to reorder everything until the end (in case of some cancellations etc) This patch also makes FixPriority exported and updates the name as it is already called in a few places outside of C4:Reserve To test: 1 - Place many holds on a record, 50 - 150 https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl 2 - View the record in the staff interface: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312 3 - Click on the 'Holds' tab 4 - Adjust the priority of several holds using the dropdowns 5 - Open the network tab in the browser console (F12) to view the timing 6 - Click 'Update hold(s)' and note the time 7 - Apply patch, restart all 8 - Repeat 9 - Confirm timing is better and numbering updates correctly 10 - Sign off! Note: The script adds holds that may be marked as 'lowestPriority' these will always all group at the bottom of the record Created attachment 190868 [details] [review] Bug 41416: Add skip_fix_priority option to ModReserve This patch adds a new parameter to ModReserve: skip_fix_priority The page for altering priority ensures each hold has a unique rank, so doesn't allow for setting two holds to the same rank, so we don't need to reorder everything until the end (in case of some cancellations etc) This patch also makes FixPriority exported and updates the name as it is already called in a few places outside of C4:Reserve To test: 1 - Place many holds on a record, 50 - 150 https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl 2 - View the record in the staff interface: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312 3 - Click on the 'Holds' tab 4 - Adjust the priority of several holds using the dropdowns 5 - Open the network tab in the browser console (F12) to view the timing 6 - Click 'Update hold(s)' and note the time 7 - Apply patch, restart all 8 - Repeat 9 - Confirm timing is better and numbering updates correctly 10 - Sign off! Note: The script adds holds that may be marked as 'lowestPriority' these will always all group at the bottom of the record Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. Placing lots of holds: perl randhold.pl -b 312 -n 150 2. Before the patch, even just changing the priority of one hold (with the tick box selected) timed out after 5 mins with a server error: Proxy Error The proxy server received an invalid response from an upstream server. The proxy server could not handle the request 3. After the patch, the update worked and didn't timeout - 5 to 10 seconds for around 10 priority changes. I wanted to take a look at this one and followed the test plan. In my case the changing of the priority did not work as expected neither before nor after applying the patch so it should not be related to this patch. I used a slightly modified version of your randhold.pl (https://gitlab.com/-/snippets/4923530) in order to have only readable patron names and only used a short amount of holds: - perl randhold.pl -b 262 -n 5 - the behavior is a bit obscure but take f.e. one patron and move its prio step by step down and check if the prio changes as expected and then move it up. I alway get stuck with one patron that would not get a higher prio. Maybe someone can confirm this. Again, this behavior does also occur before applying the patch and therefore should not interfere with it. (In reply to Jan Kissig from comment #4) > I wanted to take a look at this one and followed the test plan. In my case > the changing of the priority did not work as expected neither before nor > after applying the patch so it should not be related to this patch. > > I used a slightly modified version of your randhold.pl > (https://gitlab.com/-/snippets/4923530) in order to have only readable > patron names and only used a short amount of holds: > > - perl randhold.pl -b 262 -n 5 > - the behavior is a bit obscure but take f.e. one patron and move its prio > step by step down and check if the prio changes as expected and then move it > up. I alway get stuck with one patron that would not get a higher prio. > > Maybe someone can confirm this. > > Again, this behavior does also occur before applying the patch and therefore > should not interfere with it. Can you confirm the 'stuck' holds aren't marked as 'lowestPriority'? These will always get sorted to the bottom before and after (In reply to Nick Clemens (kidclamp) from comment #5) > (In reply to Jan Kissig from comment #4) > > I wanted to take a look at this one and followed the test plan. In my case > > the changing of the priority did not work as expected neither before nor > > after applying the patch so it should not be related to this patch. > > > > I used a slightly modified version of your randhold.pl > > (https://gitlab.com/-/snippets/4923530) in order to have only readable > > patron names and only used a short amount of holds: > > > > - perl randhold.pl -b 262 -n 5 > > - the behavior is a bit obscure but take f.e. one patron and move its prio > > step by step down and check if the prio changes as expected and then move it > > up. I alway get stuck with one patron that would not get a higher prio. > > > > Maybe someone can confirm this. > > > > Again, this behavior does also occur before applying the patch and therefore > > should not interfere with it. > > Can you confirm the 'stuck' holds aren't marked as 'lowestPriority'? These > will always get sorted to the bottom before and after You were absolutely right. Some of the holds were randomly marked with the underlined double arrow (lowestPriority). Without any lowestPriority item the changing of priorities worked perfectly. So I learned something new from Koha today and maybe I shouldn't test features I seldom use ;) Created attachment 191481 [details] [review] Bug 41416: Add skip_fix_priority option to ModReserve This patch adds a new parameter to ModReserve: skip_fix_priority The page for altering priority ensures each hold has a unique rank, so doesn't allow for setting two holds to the same rank, so we don't need to reorder everything until the end (in case of some cancellations etc) This patch also makes FixPriority exported and updates the name as it is already called in a few places outside of C4:Reserve To test: 1 - Place many holds on a record, 50 - 150 https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl 2 - View the record in the staff interface: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312 3 - Click on the 'Holds' tab 4 - Adjust the priority of several holds using the dropdowns 5 - Open the network tab in the browser console (F12) to view the timing 6 - Click 'Update hold(s)' and note the time 7 - Apply patch, restart all 8 - Repeat 9 - Confirm timing is better and numbering updates correctly 10 - Sign off! Note: The script adds holds that may be marked as 'lowestPriority' these will always all group at the bottom of the record Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Nice work everyone! Pushed to main for 26.05 |