Bug 40654

Summary: Sorting holds table can cause priority issues
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Hold requestsAssignee: Lucas Gass (lukeg) <lucas>
Status: Passed QA --- QA Contact: Emily Lamancusa (emlam) <emily.lamancusa>
Severity: major    
Priority: P5 - low CC: dbarden, emily.lamancusa, flaterdavid, gmcharlt, kkrueger, lisette, lucas, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 33575    
Bug Blocks:    
Attachments: Bug 40654: Fix how rank/priority is adjusted when changing from the UI
Bug 40654: Fix how rank/priority is adjusted when changing from the UI
Bug 40654: Fix how rank/priority is adjusted when changing from the UI
Bug 40654: Fix how rank/priority is adjusted when changing from the UI
Bug 40654: Adjust display to show current priority
Bug 40654: Adjust display to show current priority
Bug 40654: Fix how rank/priority is adjusted when changing from the UI
Bug 40654: Adjust display to show current priority

Description Nick Clemens (kidclamp) 2025-08-14 16:59:42 UTC
The table of holds has oddities about how holds are updated, and the datatables sorting has made this worse.

To recreate:
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - See all the other priorities change
6 - Click 'Update holds'
7 - Many unintended changes are saved
8 - Sort alphabetically the other direction and click the arrow to move another hold to the top  - weirdness occurs

I also saw that only 19-20 holds would be affect sometimes, I think sorting on this table may need to rethought
Comment 1 Lucas Gass (lukeg) 2025-08-14 17:51:20 UTC
This is because when rank-request is changed there is JS to move the entire row with insertBefore/insertAfter, I don't think we should be doing that with a dataTable. 

Instead we should only be adjusting the other priorities when a change is made to one.
Comment 2 Lucas Gass (lukeg) 2025-08-14 18:53:38 UTC
Created attachment 185419 [details] [review]
Bug 40654: Fix how rank/priority is adjusted when changing from the UI

To test:
0 - APPLY PATCH
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - All the priorities should now ajust accordingly but they will NOT be resorted on the fly
6 - Play with adjusting some priorities, making sure everything correctly adjusts.
7 - Click 'Update holds' to make sure the priorities adjust right
8 - Make sure you test with the 'del' option and the adjustments still work.
Comment 3 Lucas Gass (lukeg) 2025-08-14 19:06:36 UTC
This patch makes it so we no longer move the entire row around, we shouldn't do that in a dataTable. 

Instead, it recalculates priority for all holds on the record after one priority is changed.
Comment 4 David Flater 2025-08-15 20:02:29 UTC
Created attachment 185477 [details] [review]
Bug 40654: Fix how rank/priority is adjusted when changing from the UI

To test:
0 - APPLY PATCH
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - All the priorities should now ajust accordingly but they will NOT be resorted on the fly
6 - Play with adjusting some priorities, making sure everything correctly adjusts.
7 - Click 'Update holds' to make sure the priorities adjust right
8 - Make sure you test with the 'del' option and the adjustments still work.

Signed-off-by: David Flater <flaterdavid@gmail.com>
Comment 5 Emily Lamancusa (emlam) 2025-08-20 14:41:05 UTC
Sorting worked correctly for every combination I tested, but there still seems to be some weirdness with the 'del' option. For example:

I placed 10 holds on a bib for the following patrons, in the following order:
1. Marie Mcknight
2. Floyd Delgado
3. Joyce Gaines
4. Edna Acosta
5. Henry Acevedo
6. Nicole Schneider
7. Mary Burton
8. Terrence Mcconnell
9. Marilyn Mcfarland
10. Marcus Welch

Then sorted the holds table alphabetically:
4. Edna
2. Floyd
5. Henry
3. Joyce
10. Marcus
1. Marie
9. Marilyn
7. Mary
6. Nicole
8. Terrence

Next, I used the 'del' option to cancel Floyd's hold, and noticed the following errors in the priority calculation:
Name    Original Rank    Expected    Actual
Edna     4                3           2        X
Henry    5                4           4
Joyce    3                2           3        X
Marcus   10               9           7        X
Marie    1                1           1
Marilyn  9                8           9        X
Mary     7                6           6
Nicole   6                5           5
Terrence 8                7           8        X

In other words, Edna skipped ahead to rank 2 instead of 3, pushing Joyce down one. Marcus skipped ahead to rank 7 instead of 9, pushing Marilyn and Terrence down one.
Comment 6 Lucas Gass (lukeg) 2025-08-20 14:57:59 UTC
Ah! Thanks, Emily. I think I see the problem with the 'del', a fix is incoming!
Comment 7 Lucas Gass (lukeg) 2025-08-20 14:59:26 UTC
Created attachment 185589 [details] [review]
Bug 40654: Fix how rank/priority is adjusted when changing from the UI

To test:
0 - APPLY PATCH
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - All the priorities should now ajust accordingly but they will NOT be resorted on the fly
6 - Play with adjusting some priorities, making sure everything correctly adjusts.
7 - Click 'Update holds' to make sure the priorities adjust right
8 - Make sure you test with the 'del' option and the adjustments still work.

Signed-off-by: David Flater <flaterdavid@gmail.com>
Comment 8 Lucas Gass (lukeg) 2025-08-20 15:50:10 UTC
Created attachment 185590 [details] [review]
Bug 40654: Fix how rank/priority is adjusted when changing from the UI

To test:
0 - APPLY PATCH
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - All the priorities should now ajust accordingly but they will NOT be resorted on the fly
6 - Play with adjusting some priorities, making sure everything correctly adjusts.
7 - Click 'Update holds' to make sure the priorities adjust right
8 - Make sure you test with the 'del' option and the adjustments still work.

Signed-off-by: David Flater <flaterdavid@gmail.com>
Comment 9 Lucas Gass (lukeg) 2025-08-20 16:35:23 UTC
Created attachment 185597 [details] [review]
Bug 40654: Adjust display to show current priority
Comment 10 Lucas Gass (lukeg) 2025-08-20 17:14:59 UTC
Created attachment 185600 [details] [review]
Bug 40654: Adjust display to show current priority
Comment 11 Emily Lamancusa (emlam) 2025-08-20 20:46:50 UTC
Created attachment 185605 [details] [review]
Bug 40654: Fix how rank/priority is adjusted when changing from the UI

To test:
0 - APPLY PATCH
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - All the priorities should now ajust accordingly but they will NOT be resorted on the fly
6 - Play with adjusting some priorities, making sure everything correctly adjusts.
7 - Click 'Update holds' to make sure the priorities adjust right
8 - Make sure you test with the 'del' option and the adjustments still work.

Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 12 Emily Lamancusa (emlam) 2025-08-20 20:46:53 UTC
Created attachment 185606 [details] [review]
Bug 40654: Adjust display to show current priority

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>