To reproduce: 1) Setup all the system preferences in ERM: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=erm 2) Enable EBSCO in ERMProviders. Set the ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID prefs. 3) Do a search that returns more than 20 records. Click on a page other than 1. Notice the records do not change (Page is the same).
Created attachment 172782 [details] [review] Bug 38177: Fix HoldingsIQ pagination Test plan: 1) Setup all the system preferences in ERM: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=erm 2) Enable EBSCO in ERMProviders. Set the ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID prefs. 3) Do a search that returns more than 20 records. Click on a page other than 1. Notice the records do not change (Page is the same). 4) Apply patch. Repeat. Notice pagination now works.
Created attachment 172783 [details] [review] Bug 38177: Hide protected ERM prefs on UI Hide ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID on the sys prefs UI
Providing the second patch in case anyone wants to test this on sandboxes (hide the credentials). I think it's a good enhancement either way. Happy to push to a different bug if asked to.
I am not sure what you are doing here. "All" results in 500, and the problem still exists for packages and resources.
May be related to bug 36483.
Created attachment 172803 [details] [review] [ALTERNATIVE PATCH] Bug 38177: Do not use stash to retrieve parameters The pagination for EBSCO's titles, packages and resources is broken. We shouldn't rely on $c->stash but use params from the request instead. Test plan: 1) Setup all the system preferences in ERM: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=erm 2) Enable EBSCO in ERMProviders. Set the ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID prefs. 3) Do a search that returns more than 20 records. Click on a page other than 1. Notice the records do not change (Page is the same). 4) Apply patch. Repeat. Notice pagination now works. Search titles, packages and resources Use the pagination and modify the number of results per page
(In reply to Jonathan Druart from comment #4) > I am not sure what you are doing here. $c->stash('koha.pagination.per_page') and $c->stash('koha.pagination.page') are undefined. I'm not sure why, or if they should ever be defined. But I'm changing that to use the params coming directly from the URL instead. This fixes the problem. I'm obv open to other options. (In reply to Jonathan Druart from comment #4) > "All" results in 500 This problem exists before the patch, i.e. I think it's another problem. (In reply to Jonathan Druart from comment #4) > the problem still exists for packages and resources. I missed this but it makes sense since it's the same code duplicated. Whatever fix is pushed to Titles should also be done for Resources+Packages I reckon.
Created attachment 172806 [details] [review] Bug 38177: Do not use stash to retrieve parameters The pagination for EBSCO's titles, packages and resources is broken. We shouldn't rely on $c->stash but use params from the request instead. Test plan: 1) Setup all the system preferences in ERM: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=erm 2) Enable EBSCO in ERMProviders. Set the ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID prefs. 3) Do a search that returns more than 20 records. Click on a page other than 1. Notice the records do not change (Page is the same). 4) Apply patch. Repeat. Notice pagination now works. Search titles, packages and resources Use the pagination and modify the number of results per page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Thank you for the patch Joubu. It looks great and works great! I'll try to get a third-party SO and change my SO to PQA if I can do that.
(In reply to Pedro Amorim from comment #7) > (In reply to Jonathan Druart from comment #4) > > "All" results in 500 > > This problem exists before the patch, i.e. I think it's another problem. I don't think so. It's one of the responsibility of build_query_pagination 215 if ( $per_page == -1 || $per_page > 100 ) { $per_page = 100; }
You're(In reply to Jonathan Druart from comment #10) > (In reply to Pedro Amorim from comment #7) > > (In reply to Jonathan Druart from comment #4) > > > "All" results in 500 > > > > This problem exists before the patch, i.e. I think it's another problem. > > I don't think so. It's one of the responsibility of build_query_pagination > > 215 if ( $per_page == -1 || $per_page > 100 ) { $per_page = 100; } I must've tested it wrong, tested it again, you're right. Apologies. Thanks a lot for your help and input here @Joubu
Created attachment 173068 [details] [review] Bug 38177: Fix lengthMenu for components using KohaTable EBSCO HoldingsIQ doesnt consider a "All" option so neither should we. At the moment, if we pick 'All', only 100 are listed due to the logic in Providers/EBSCO/build_query_pagination. The original code hints at trying to do this but its not working. I think this got lost sometime during KohaTable.vue This patch should also have the same entry for EHoldingsEBSCOTitlePackagesList but that isnt using KohaTable and for some reason its not picking up on either aLengthMenu or lengthMeny, always shows 'All'.
@Joubu can you look at my latest patch please. We need to fix the fact that 'All' is only returning the first 100 results from HoldingsIQ. I can't get this to work for EHoldingsEBSCOTitlePackagesList and having that adopt KohaTable is out of scope here I think.
The patch is working for me (ie. 'All' is not displayed).
Created attachment 173119 [details] [review] Bug 38177: (follow-up) Fix lengthMenu for components using KohaTable
Created attachment 173121 [details] [review] Bug 38177: Fix lengthMenu by removing extend $.extend is doing a deep copy of the objects. ``` var dataTablesDefaults = { "lengthMenu": [[10, 20, 50, 100, -1], [10, 20, 50, 100, __('All')]] }; var mine = {lengthMenu: [[1, 2, 5, 10], [1, 2, 5, 10]]}; $.extend(true, {}, dataTablesDefaults, mine); ``` Returns: ``` [ 1, 2, 5, 10, -1 ] ``` Which is ugly and not what we want. We only need to merge the first level. Using the spread operator (...) will do the trick here. Note that we certainly want to adjust all the other occurrences of extend for DT's options.
Created attachment 173127 [details] [review] Bug 38177: Do not use stash to retrieve parameters The pagination for EBSCO's titles, packages and resources is broken. We shouldn't rely on $c->stash but use params from the request instead. Test plan: 1) Setup all the system preferences in ERM: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=erm 2) Enable EBSCO in ERMProviders. Set the ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID prefs. 3) Do a search that returns more than 20 records. Click on a page other than 1. Notice the records do not change (Page is the same). 4) Apply patch. Repeat. Notice pagination now works. Search titles, packages and resources Use the pagination and modify the number of results per page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 173128 [details] [review] Bug 38177: Fix lengthMenu for components using KohaTable EBSCO HoldingsIQ doesnt consider a "All" option so neither should we. At the moment, if we pick 'All', only 100 are listed due to the logic in Providers/EBSCO/build_query_pagination. The original code hints at trying to do this but its not working. I think this got lost sometime during KohaTable.vue This patch should also have the same entry for EHoldingsEBSCOTitlePackagesList but that isnt using KohaTable and for some reason its not picking up on either aLengthMenu or lengthMeny, always shows 'All'. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 173129 [details] [review] Bug 38177: Fix lengthMenu by removing extend $.extend is doing a deep copy of the objects. ``` var dataTablesDefaults = { "lengthMenu": [[10, 20, 50, 100, -1], [10, 20, 50, 100, __('All')]] }; var mine = {lengthMenu: [[1, 2, 5, 10], [1, 2, 5, 10]]}; $.extend(true, {}, dataTablesDefaults, mine); ``` Returns: ``` [ 1, 2, 5, 10, -1 ] ``` Which is ugly and not what we want. We only need to merge the first level. Using the spread operator (...) will do the trick here. Note that we certainly want to adjust all the other occurrences of extend for DT's options. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 173190 [details] [review] Bug 38177: Do not use stash to retrieve parameters The pagination for EBSCO's titles, packages and resources is broken. We shouldn't rely on $c->stash but use params from the request instead. Test plan: 1) Setup all the system preferences in ERM: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=erm 2) Enable EBSCO in ERMProviders. Set the ERMProviderEbscoApiKey and ERMProviderEbscoCustomerID prefs. 3) Do a search that returns more than 20 records. Click on a page other than 1. Notice the records do not change (Page is the same). 4) Apply patch. Repeat. Notice pagination now works. Search titles, packages and resources Use the pagination and modify the number of results per page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 173191 [details] [review] Bug 38177: Fix lengthMenu for components using KohaTable EBSCO HoldingsIQ doesnt consider a "All" option so neither should we. At the moment, if we pick 'All', only 100 are listed due to the logic in Providers/EBSCO/build_query_pagination. The original code hints at trying to do this but its not working. I think this got lost sometime during KohaTable.vue This patch should also have the same entry for EHoldingsEBSCOTitlePackagesList but that isnt using KohaTable and for some reason its not picking up on either aLengthMenu or lengthMeny, always shows 'All'. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 173192 [details] [review] Bug 38177: Fix lengthMenu by removing extend $.extend is doing a deep copy of the objects. ``` var dataTablesDefaults = { "lengthMenu": [[10, 20, 50, 100, -1], [10, 20, 50, 100, __('All')]] }; var mine = {lengthMenu: [[1, 2, 5, 10], [1, 2, 5, 10]]}; $.extend(true, {}, dataTablesDefaults, mine); ``` Returns: ``` [ 1, 2, 5, 10, -1 ] ``` Which is ugly and not what we want. We only need to merge the first level. Using the spread operator (...) will do the trick here. Note that we certainly want to adjust all the other occurrences of extend for DT's options. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Skipping QA here, I don't think we can rely on somebody else to QA this one.
Missing the EBSCO login to test this, but trusting Pedro and Jonathan.
Pushed for 24.11! Well done everyone, thank you!
Backported to 24.05.x for upcoming 24.05.06
Pushed to 23.11.x for 23.11.11
Not pushed to LTS. Marked Resolved. If you feel this should be in LTS please reply with your reason.
This should definitely be considered for backport but it's a pain to test and there also be a datatables version mismatch by 23.05.x Please reach out if you need help testing this on 23.05.x.