Bug 23269 - Long hold queues are slowing the service
Summary: Long hold queues are slowing the service
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Johanna Räisä
QA Contact: Pedro Amorim
URL: https://github.com/KohaSuomi/Koha/tre...
Keywords:
: 37705 (view as bug list)
Depends on: 30660 38253 30661
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-05 09:50 UTC by Emmi Takkinen
Modified: 2024-11-07 13:22 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 23269: use REST API for patron_holds_table (84.09 KB, patch)
2024-10-31 11:02 UTC, Johanna Räisä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2019-07-05 09:50:46 UTC
In Finlands public libraries hold queues can be huge due popular records having a lot of holds at the same time. Most popular records may even have over 1000 holds. This leads the service slowing significantly when browsing their holds page. There should be pagination in the holds table for efficient page loading.
Comment 1 Katrin Fischer 2023-04-16 01:33:26 UTC
Maybe this would be another candidate to be moved to the REST API?
Comment 2 Emmi Takkinen 2023-06-29 05:22:32 UTC
(In reply to Katrin Fischer from comment #1)
> Maybe this would be another candidate to be moved to the REST API?

Johanna has actually worked on this (but is currently absent) and we've been using it for awhile now https://github.com/KohaSuomi/Koha-22x/commit/c26f53eeac4c12e13e1e595931b4ec279f054da7

However, this depends on bug 30660 and bug 30661.
Comment 3 Johanna Räisä 2024-10-21 05:40:47 UTC
*** Bug 37705 has been marked as a duplicate of this bug. ***
Comment 4 Pedro Amorim 2024-10-23 12:09:19 UTC
(In reply to Emmi Takkinen from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > Maybe this would be another candidate to be moved to the REST API?
> 
> Johanna has actually worked on this (but is currently absent) and we've been
> using it for awhile now
> https://github.com/KohaSuomi/Koha-22x/commit/
> c26f53eeac4c12e13e1e595931b4ec279f054da7
> 
> However, this depends on bug 30660 and bug 30661.

Hi all, is this still the most up to date work in progress?
Would you please add the relevant git repo branch URL to this bug's URL?
I'm available to help out here.
Comment 5 Johanna Räisä 2024-10-23 12:16:42 UTC
(In reply to Pedro Amorim from comment #4)
> (In reply to Emmi Takkinen from comment #2)
> > (In reply to Katrin Fischer from comment #1)
> > > Maybe this would be another candidate to be moved to the REST API?
> > 
> > Johanna has actually worked on this (but is currently absent) and we've been
> > using it for awhile now
> > https://github.com/KohaSuomi/Koha-22x/commit/
> > c26f53eeac4c12e13e1e595931b4ec279f054da7
> > 
> > However, this depends on bug 30660 and bug 30661.
> 
> Hi all, is this still the most up to date work in progress?
> Would you please add the relevant git repo branch URL to this bug's URL?
> I'm available to help out here.

Yes, I working on it at the moment. We have our tweaked version and I'm trying to improve it to work in main. I will add a URL tomorrow, after I have cleaned some code.
Comment 6 Johanna Räisä 2024-10-24 08:32:59 UTC
I added a branch URL to this bug. Still needs testing that everything works correctly with permissions, multiple records etc.
Comment 7 Pedro Amorim 2024-10-24 08:36:53 UTC
Hi Johanna, thank you. If this is ready for testing please set the bug as "Needs Signoff".
Comment 8 Johanna Räisä 2024-10-24 08:42:34 UTC
(In reply to Pedro Amorim from comment #7)
> Hi Johanna, thank you. If this is ready for testing please set the bug as
> "Needs Signoff".

I will improve it a bit and add a patch for testing after.
Comment 9 Johanna Räisä 2024-10-31 11:02:16 UTC
Created attachment 173770 [details] [review]
Bug 23269: use REST API for patron_holds_table

This patch uses the REST API to populate the holds table

Test plan:
1) Apply the patch
2) Go to a record with holds
3) Check that the holds table is populated correctly
4) Check that hold tables functions work correctly
5) Change HoldsSplitQueue and HoldsSplitQueueNumbering system preference, try all variations
6) Repeat steps 3 and 4
7) Search for records and place holds to multiple records
8) Repeat steps 3 and 4
9) Try with lesser permissions

Sponsored-by: Koha-Suomi Oy
Comment 10 Johanna Räisä 2024-10-31 11:07:23 UTC
Finally got the patch here! Splitting the results with HoldsSplitQueue system preference took more time than I expected.
Comment 11 Pedro Amorim 2024-11-05 11:28:51 UTC
Looking here
Comment 12 Pedro Amorim 2024-11-05 12:19:58 UTC
This is really impressive work. It works great from what I can tell.
I have some observations, though I don't think any one in particular is a blocker, and some may not need a follow-up and are simply observations:

- Entering an expiration date triggers the same API call twice
- Entering "suspend on" date triggers the same API call twice
- Date inputs are missing the "x" to clear the input
- Entering "suspend on" date immediately suspends the hold. On the previous table you'd have to click the 'suspend' button after inserting the date to effectively suspend it.
- The label for a suspended hold is "Resume", previously was "Unsuspend".
- Trash icon for "delete" icon missing hover pointer cursor.
- If all actions now update holds in real-time, do we still need the "Update hold(s)" button at the top? Previously this was needed to save the changes, but it's not anymore.
- "Print slip" button now always shows, previously it only showed if hold.intransit || hold.atdestination.
- Deleting a hold is triggering table redraw API call multiple times, to reproduce:
-- Place 3 or 4 holds on a patron.
-- Move the priority around multiple times for all holds.
-- Delete one of the holds.
-- Notice the network tab calling the same API endpoint multiple times (sometimes over 10 times)

I think at least the "Update holds" button question and the multiple API calls issues should be addressed. What do you think Johanna? Do you agree? Up to you how best we should follow-up here. I think this is really good as is already.
Comment 13 Johanna Räisä 2024-11-06 09:26:54 UTC
(In reply to Pedro Amorim from comment #12)
> This is really impressive work. It works great from what I can tell.
> I have some observations, though I don't think any one in particular is a
> blocker, and some may not need a follow-up and are simply observations:
> 
> - Entering an expiration date triggers the same API call twice
> - Entering "suspend on" date triggers the same API call twice
> - Date inputs are missing the "x" to clear the input
> - Entering "suspend on" date immediately suspends the hold. On the previous
> table you'd have to click the 'suspend' button after inserting the date to
> effectively suspend it.
> - The label for a suspended hold is "Resume", previously was "Unsuspend".
> - Trash icon for "delete" icon missing hover pointer cursor.
> - If all actions now update holds in real-time, do we still need the "Update
> hold(s)" button at the top? Previously this was needed to save the changes,
> but it's not anymore.
> - "Print slip" button now always shows, previously it only showed if
> hold.intransit || hold.atdestination.
> - Deleting a hold is triggering table redraw API call multiple times, to
> reproduce:
> -- Place 3 or 4 holds on a patron.
> -- Move the priority around multiple times for all holds.
> -- Delete one of the holds.
> -- Notice the network tab calling the same API endpoint multiple times
> (sometimes over 10 times)
> 
> I think at least the "Update holds" button question and the multiple API
> calls issues should be addressed. What do you think Johanna? Do you agree?
> Up to you how best we should follow-up here. I think this is really good as
> is already.

Thank you for reviewing, this was a big one!

I totally agree with you. We don't actually need the "Update hold(s)" button anymore, I doubted it while I was doing it. Good to hear that you think the same. 

I will optimize those API calls and improve other features as well. Good to get some other to look at this. I was getting blind to all these features.