OPAC ILL table, as opposed to the Staff ILL table is using server-side rendering with no pagination. This proposes bringing it in line with its Staff equivalent, porting it to API+datatables and getting all the associated goodies along with it. Patch incoming.
Created attachment 180758 [details] [review] Switch OPAC ILL request listing to server-side rendering This brings it technologically closer to intranet's ILL table and also enables pagination.
Created attachment 180759 [details] [review] Implment public_read_lists for ILL::Request and ::Attribute
A drawback of this approach is that it's not longer possible to order on extended attributes (author, title etc). This is a flaw shared by the Staff ILL table, where it's also the case that only the "simple" (not x-koha-embedded) values can be used for ordering. Sponsored-by: Wiko (https://www.wiko-berlin.de/)
We currently have a lot of ILL work in the queue for 25.05 - I totally love the idea about a modernization as I think it's needed, but it might need some work to fit on top of the other changes once they are pushed.
*** Bug 38339 has been marked as a duplicate of this bug. ***
Created attachment 180760 [details] [review] Bug 39460: Fix koha-upgrade-schema when no DB change needed This patch fixes koha-upgrade-schema so that it returns an exit code of 0 whether or not a DB change is needed. This is important so that koha-common.postinst proceeds correctly even when there is no DB change needed (e.g. for 24.11.03-2) Test plan: 0. Apply the patch 1. cp debian/scripts/koha-upgrade-schema /usr/sbin/koha-upgrade-schema 2. dpkg-reconfigure koha-common 3. Note the process completes correctly 4. koha-upgrade-schema kohadev 5. Note following text appears very quickly: Upgrading database schema for kohadev No database change required 6. koha-mysql kohadev update systempreferences set value = '24.1102000' where variable = 'Version'; 7. Clear memcached 8. koha-upgrade-schema kohadev 9. Note that it applies the 24.11.03 database upgrade Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
^ Added accidentally, ignore.
I've closed bug 38339 as the work here implicitly fixes the issue described there. I understand this is an enhancement but could also be somewhat considered a bug fix as it's a big performance improvement for scenarios when a big a list of ILL requests are to be displayed. Please rebase on main when possible (if you haven't already) and let's try to move this quickly through QA! =D
Created attachment 180761 [details] [review] Switch OPAC ILL request listing to server-side rendering This brings it technologically closer to intranet's ILL table and also enables pagination. Rebased on top of main. Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Created attachment 180762 [details] [review] Implment public_read_lists for ILL::Request and ::Attribute
Created attachment 180763 [details] [review] Bug 39600: Switch OPAC ILL request listing to server-side rendering This brings it technologically closer to intranet's ILL table and also enables pagination. Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Created attachment 180764 [details] [review] Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute Sponsored-by: Wiko (https://www.wiko-berlin.de/)
*** Bug 39450 has been marked as a duplicate of this bug. ***
Marked bug 39450 as a duplicate of this for the same reason as bug 38339: The patches here implicitly fix the issue there (and do more). Putting this to "Needs Sign-off" as I believe the patches are ready to be looked at.
This is great work and greatly appreciated. I'm failing QA, to encourage quick follow-up, for the following reasons: 1) The new endpoint method patron_list needs a test in t/db_dependent/api/v1/ill_requests.t e.g. proving that requests cannot be retrieved for a different patron. 2) This needs a proper test plan (possibly on the main patch's commit message) e.g.: - Enable ILLModule - Create 50 requests for koha patron, run (if k-t-d): perl -Mt::lib::TestBuilder -e ' my $builder = t::lib::TestBuilder->new; do { $builder->build_sample_ill_request({borrowernumber => 51}); } for (1..50); ' - Access the OPAC ILL requests page: http://localhost:8080/cgi-bin/koha/opac-illrequests.pl - Notice the requests are shown, paginated.
Created attachment 180778 [details] [review] Bug 39600: Add tests for patron_list Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Created attachment 180781 [details] [review] Bug 39600: Switch OPAC ILL request listing to server-side rendering This brings it technologically closer to intranet's ILL table and also enables pagination. Test plan: 1. In KTD, go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=master+switch and enable the ILL module 2. Run the following in koha-shell to populate the request list for user `koha`: perl -Mt::lib::TestBuilder -e ' my $builder = t::lib::TestBuilder->new; do { $builder->build_sample_ill_request({borrowernumber => 51}); } for (1..50); ' 3. Navigate to http://localhost:8080/cgi-bin/koha/opac-illrequests.pl, log in as `koha` as needed 4. Observe the new request list being paginated and server-side-sortable Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Created attachment 180785 [details] [review] Bug 39600: Switch OPAC ILL request listing to server-side rendering This brings it technologically closer to intranet's ILL table and also enables pagination. Test plan: 1. In KTD, go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=master+switch and enable the ILL module 2. Run the following in koha-shell to populate the request list for user `koha`: perl -Mt::lib::TestBuilder -e ' my $builder = t::lib::TestBuilder->new; do { $builder->build_sample_ill_request({borrowernumber => 51}); } for (1..50); ' 3. Navigate to http://localhost:8080/cgi-bin/koha/opac-illrequests.pl, log in as `koha` as needed 4. Observe the new request list being paginated and server-side-sortable Sponsored-by: Wiko (https://www.wiko-berlin.de/) Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 180786 [details] [review] Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute Sponsored-by: Wiko (https://www.wiko-berlin.de/) Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Created attachment 180787 [details] [review] Bug 39600: Add tests for patron_list Sponsored-by: Wiko (https://www.wiko-berlin.de/) Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Putting this as "Signed-off". Thank you Tadeusz. I had to reorder the patches to have them apply nicely (please always submit the whole patchset when re-submiting a patch other than the last one, or they'll fall out of order). The QA script is complaining about the following: FAIL Koha/ILL/Request.pm FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl Koha/ILL/Request.pm` FAIL Koha/ILL/Request/Attribute.pm FAIL pod coverage POD is missing for 'public_read_list' FAIL Koha/REST/V1/ILL/Requests.pm FAIL pod coverage POD is missing for 'patron_list' FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl Koha/REST/V1/ILL/Requests.pm` OK api/v1/swagger/paths/patrons.yaml OK api/v1/swagger/swagger.yaml OK koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-close.inc FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt FAIL filters missing_filter at line 413 ( "url": '/api/v1/patrons/[% logged_in_user.borrowernumber %]/ill/requests',) FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt` FAIL opac/opac-illrequests.pl FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl opac/opac-illrequests.pl` FAIL t/db_dependent/api/v1/ill_requests.t FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/api/v1/ill_requests.t` Processing additional checks OK! Running tests (1) * Proving /kohadevbox/koha/t/db_dependent/api/v1/ill_requests.t OK! --------------- To have the code automatically tidied for you on commit, consider using the pre commit hook: https://gitlab.com/koha-community/koha-testing-docker/-/blob/main/files/git_hooks/pre-commit This comes built-in with k-t-d. These should be easy fixes, once we have these fixed I think we should be able to move this to "Passed QA".
Created attachment 180821 [details] [review] Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Moving this to PQA to help move along as it's quite valuable work. Thanks for following up with the feedback Tadeusz!