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
(In reply to Nick Clemens (kidclamp) from comment #30) > 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 I will try to improve the features, so this would pass. At the moment we are facing the issue that if there is over 1000 holds then the whole page won't open, there is a limit in Template toolkit that it can handle less than 1000 item arrays.
Created attachment 191638 [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 I hopefully have fixed the issues and this works with new hold features. Was bit of a nightmare to check the related features again since there were lot of refactoring made.
Created attachment 191669 [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 191670 [details] [review] Bug 23269: use REST API for patron_holds_table Added a minor fix for showing the cancel alert box.
Created attachment 192821 [details] [review] Bug 23269: (follow-up) Show lowest priority toggle unless hold is 'found'
This works great, I tested in combination with 41801 and was able to quickly move holds and adjust priorities. I added a follow-up as the condition for showing the lowest priority toggle was incorrect - we can still toggle on item level holds, but not on found holds. The only issue I found was that clicking the 'move to top' or 'move to bottom' arrow on a record with hundreds of holds was slow and there was no spinner - if you can add that I will sign off. Excellent work!
Created attachment 192849 [details] [review] Bug 23269: (follow-up) Show lowest priority toggle unless hold is 'found' Signed-off-by: Johanna Räisä <johanna.raisa@gmail.com>
Created attachment 192850 [details] [review] Bug 23269: (follow-up) Fix suspend date change This patch fixes suspend date change, so changing the suspend will save the new suspend date.
Created attachment 192851 [details] [review] Bug 23269: (follow-up) Add spinner and fix details column This patch adds a spinner to moving priorities and fixes the details column to show the correct information.
Created attachment 192939 [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 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 192940 [details] [review] Bug 23269: (follow-up) Show lowest priority toggle unless hold is 'found' Signed-off-by: Johanna Räisä <johanna.raisa@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 192941 [details] [review] Bug 23269: (follow-up) Fix suspend date change This patch fixes suspend date change, so changing the suspend will save the new suspend date. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 192942 [details] [review] Bug 23269: (follow-up) Add spinner and fix details column This patch adds a spinner to moving priorities and fixes the details column to show the correct information. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 192943 [details] [review] Bug 23269: (follow-up) Check if hold item_level before grabbing barcode Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 193100 [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 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 193101 [details] [review] Bug 23269: (follow-up) Show lowest priority toggle unless hold is 'found' Signed-off-by: Johanna Räisä <johanna.raisa@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 193102 [details] [review] Bug 23269: (follow-up) Fix suspend date change This patch fixes suspend date change, so changing the suspend will save the new suspend date. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 193103 [details] [review] Bug 23269: (follow-up) Add spinner and fix details column This patch adds a spinner to moving priorities and fixes the details column to show the correct information. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 193104 [details] [review] Bug 23269: (follow-up) Check if hold item_level before grabbing barcode Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Passing QA as everything appears to be working as intended and this adds great value to Koha. I have some minor comments which I don't think are blockers and up to you Johanna if you feel they're worth following up on or not! 1) FIXME: in holds_table.inc I think we can remove this about form-submit.js as is included in js_includes.inc so should be always available everywhere. 2) request.tt and request.pl It appears 'total_holds' is replacing 'reserveloop'. If this is the case, we should cleanup the remaining instances of reserveloop in both files. 3) Could use new API tests under 'edit' in t/db_dependent/api/v1/holds.t to test new params (item_level, item_id) 4) Hold::item_level_holds missing tests, possibly better renamed to item_level_holds_count
Created attachment 193198 [details] [review] Bug 23269: (follow-up) Cleanup code
Created attachment 193199 [details] [review] Bug 23269: (follow-up) Add tests for item_level and item_id in holds API This patch adds tests for the item_level and item_id parameters in the holds API. It also updates the C4/Reserves.pm module to handle these parameters correctly. Test plan: 1. prove t/db_dependent/api/v1/holds.t Sponsored-by: Koha-Suomi Oy
Created attachment 193200 [details] [review] Bug 23269: (follow-up) Rename to item_level_holds_count and add tests Test plan: 1. prove t/db_dependent/Koha/Holds.t Sponsored-by: Koha-Suomi Oy
I implemented the changes that Pedro suggested and added more tests. The QA tool is complaining about tidiness. I fixed the issues in the files I modified, but it wants to fix more lines especially in t/db_dependent/api/v1/holds.t"
These look great, thank you Johanna, last patch appears to not be applying nicely for me: Applying: Bug 23269: (follow-up) Rename to item_level_holds_count and add tests .git/rebase-apply/patch:39: trailing whitespace. type: error: sha1 information is lacking or useless (api/v1/swagger/definitions/hold.yaml). error: could not build fake ancestor Patch failed at 0001 Bug 23269: (follow-up) Rename to item_level_holds_count and add tests
Created attachment 193201 [details] [review] Bug 23269: (follow-up) Rename to item_level_holds_count and add tests I fixed the trailing whitespace, hope it will work now.
Still not applying for me, when trying: git apply --reject <patch_file_name> I see error on holds.js not hold.yaml despite the sha1 error. Do you have Nick's patch applied? Bug 23269: (follow-up) Check if hold item_level before grabbing barcode
(In reply to Pedro Amorim from comment #58) > Still not applying for me, when trying: > git apply --reject <patch_file_name> I see error on holds.js not hold.yaml > despite the sha1 error. > > Do you have Nick's patch applied? > Bug 23269: (follow-up) Check if hold item_level before grabbing barcode Oh, I missed that. Thank you!
Created attachment 193219 [details] [review] Bug 23269: (follow-up) Rename to item_level_holds_count and add tests Now the patch should apply. Thanks for help. :)
Thank you so much for following up with my suggestions. I think this is ready to go.
Created attachment 193336 [details] [review] Bug 23269: (follow-up) Fix priority modification for lesser permissions This is a minor patch to hide priority modification from users who doesn't have permission to do it. For some reason visibility of the columns didn't work any more.
*** Bug 29124 has been marked as a duplicate of this bug. ***
*** Bug 28964 has been marked as a duplicate of this bug. ***