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.
Maybe this would be another candidate to be moved to the REST API?
(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.
*** Bug 37705 has been marked as a duplicate of this bug. ***
(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.
(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.
I added a branch URL to this bug. Still needs testing that everything works correctly with permissions, multiple records etc.
Hi Johanna, thank you. If this is ready for testing please set the bug as "Needs Signoff".
(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.
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
Finally got the patch here! Splitting the results with HoldsSplitQueue system preference took more time than I expected.
Looking here
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.
(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.
Created attachment 176705 [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
Now all those that Pedro pointed out should be fixed. API endpoints changed a bit so running build-resources.PL is needed.
Patch no longer applies I believe because of bug 38921 recently pushed. Please rebase.
Created attachment 177151 [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
Created attachment 181244 [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 Pedro rebased and tidied after bug 38664
Created attachment 181247 [details] [review] Bug 23269: Remove 'filtered from x total entries'
Hi Johanna, thank you for your patience here. I've rebased and tidied the patch on top of bug 38664. I believe the integrity of the patch was kept but apologies in advance if some regression was introduced here, my intention is to help move this forward. Here are my notes: Previously mentioned regressions that are still occurring (but not critical): 1) Setting an expiration date still triggers 2 pairs of API requests (the update + table rerender) 2) The button label for a suspended hold is "Resume", previously was "Unsuspend". New regressions (worth investigating): 1) Before, if a hold is "in transit" (found = T), the priority column shows a disabled dropdown with value 'In transit'. Now, the dropdown contains 'In transit' but it's not disabled. Changing the value here for a hold triggers a table re-render but seemingly doesn't update any value. 2) Table columns were sortable before, they are no longer. This may be tricky but if at least some of them are easy to make sortable we should consider this. Observation (not critical) The table now shows: Showing 1 to 4 of 4 entries (filtered from 5 total entries) This is the standard behavior of API based tables. But the '5' from the above example is referring to the total holds number of the database.I wonder if it will ever be relevant to show the total number of holds in the database in this screen? Provided patch if you agree.
Created attachment 182728 [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 Pedro rebased and tidied after bug 38664
Created attachment 182729 [details] [review] Bug 23269: Remove 'filtered from x total entries' Signed-off-by: Johanna Räisä <johanna.raisa@gmail.com>
Created attachment 182730 [details] [review] Bug 23269: Add datatable fixes
(In reply to Pedro Amorim from comment #20) > Hi Johanna, thank you for your patience here. > I've rebased and tidied the patch on top of bug 38664. I believe the > integrity of the patch was kept but apologies in advance if some regression > was introduced here, my intention is to help move this forward. > Here are my notes: > > Previously mentioned regressions that are still occurring (but not critical): > 1) Setting an expiration date still triggers 2 pairs of API requests (the > update + table rerender) > 2) The button label for a suspended hold is "Resume", previously was > "Unsuspend". I made fixes to these. Those should be ok now. > > New regressions (worth investigating): > 1) Before, if a hold is "in transit" (found = T), the priority column shows > a disabled dropdown with value 'In transit'. Now, the dropdown contains 'In > transit' but it's not disabled. Changing the value here for a hold triggers > a table re-render but seemingly doesn't update any value. For some reason I didn't notice this. I compared to current main and both were disabled. > 2) Table columns were sortable before, they are no longer. This may be > tricky but if at least some of them are easy to make sortable we should > consider this. I added sortable to columns were it would make sense. > > Observation (not critical) > The table now shows: > Showing 1 to 4 of 4 entries (filtered from 5 total entries) > > This is the standard behavior of API based tables. But the '5' from the > above example is referring to the total holds number of the database.I > wonder if it will ever be relevant to show the total number of holds in the > database in this screen? Provided patch if you agree. This was a good addition. It looked a bit silly there.
Created attachment 182754 [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 Pedro rebased and tidied after bug 38664 Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 182755 [details] [review] Bug 23269: Remove 'filtered from x total entries' Signed-off-by: Johanna Räisä <johanna.raisa@gmail.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 182756 [details] [review] Bug 23269: Add datatable fixes Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 182757 [details] [review] Bug 23269: Address qa script complaints Use __("") instead of _("") File is not tidy holds.js Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Thank you, Johanna. Adding Nick as QA contact here as I've signed-off.
I really like this, and I really want it for Koha, however, in testing with many holds I am finding that the performance is not great. The page and table do load faster, but in using the API to update the holds I think this dev reveals that our holds APIs themselves are too slow. I used the 'randhold.pl' script from here: https://github.com/kidclamp/handy-koha-script I tried with about 2800 holds, then 800, and found that adjusting the priority of a hold gave no indication anything was happening. It would, sometimes, eventually complete, but without any indication it makes it hard to know why a reload is delayed. I saw similar issues when adding an expiration date to a hold. Additionally, if I selected several checkboxes on a single page, they would be remembered if I navigated to a different page and back, however, if I selected anything on a different page, then anything on the other page was forgotten. i.e. select the third hold on page one, go to page three, go back to page one, the selection stays got to page three, select the first hold, go back to page one, initial selection is lost, go back to page three, the first hold remains selected