Description
HKS3 Tadeusz Sośnierz
2025-04-10 09:49:24 UTC
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! Created attachment 181132 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181133 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181134 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181135 [details] [review] Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter Sponsored-by: Wiko (https://www.wiko-berlin.de/) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> On trying to rebase, I noticed this is using `datatable()` and manually crafting everything. Some more work is needed here. Tadeusz, can you take care of it? Happy to assist. A few QA remarks. 1) It is great that you add a staff side endpoint for patron ILL requests 2) The OPAC should be using endpoints in the /public namespace (i.e. /public/patrons/:patron_id/ill/requests) 3) The /public namespace automatically stashes `is_public` (see V1/Auth.pm). That's why I noticed the problem with your patchset, as you manually stashed things. 4) You are using plain datatables. We use our wrapper instead. That's what should be used. If you cannot fix this, I'll give it a try. Created attachment 181437 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181438 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181439 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181440 [details] [review] Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter Sponsored-by: Wiko (https://www.wiko-berlin.de/) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181441 [details] [review] Bug 39600: Add Koha::Patron->ill_requests() accessor Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181442 [details] [review] Bug 39600: Rewrite API controllers This patch splits the controller in two: * A controller for staff users use * An unprivileged access controller A new endpoint is added for publi access: * `/public/patrons/:patron_id/ill/requests` The public one makes use of the state-of-the-art in Koha for public endpoints, using the `$c->auth->public` validation helper and leaving the (newly added) public path spec with no special permissions requirements. The staff-oriented endpoint gets the right permissions added (at least those used in the main `/ill/requests` endpoint. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/api/v1/ill_requests.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 181443 [details] [review] Bug 39600: Adapt template and use kohaTable This patch makes changes to the table introduced in the original patch: * It gets rendered using `kohaTable` instead of manually crafting the datatable * Embeds are parameters for `kohaTable` thus no need to build the headers manually * A default render method is introduced, which HTML-escapes the data for safety, and also deals with `null` values which could cause problems. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Tadeusz and all. I finally made some bigger changes than expected. My motivation was to make this through the finish line of course. And also show you how we introduce new controllers. Hope you like it and we can chat about this if you want to. You should re-test it too. Created attachment 181524 [details] [review] Bug 39600: Add Koha::Patron->ill_requests() accessor Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com> Created attachment 181525 [details] [review] Bug 39600: Rewrite API controllers This patch splits the controller in two: * A controller for staff users use * An unprivileged access controller A new endpoint is added for publi access: * `/public/patrons/:patron_id/ill/requests` The public one makes use of the state-of-the-art in Koha for public endpoints, using the `$c->auth->public` validation helper and leaving the (newly added) public path spec with no special permissions requirements. The staff-oriented endpoint gets the right permissions added (at least those used in the main `/ill/requests` endpoint. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/api/v1/ill_requests.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com> Created attachment 181526 [details] [review] Bug 39600: Adapt template and use kohaTable This patch makes changes to the table introduced in the original patch: * It gets rendered using `kohaTable` instead of manually crafting the datatable * Embeds are parameters for `kohaTable` thus no need to build the headers manually * A default render method is introduced, which HTML-escapes the data for safety, and also deals with `null` values which could cause problems. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com> Created attachment 181532 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181533 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181534 [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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181535 [details] [review] Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter Sponsored-by: Wiko <https://www.wiko-berlin.de/> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181536 [details] [review] Bug 39600: Add Koha::Patron->ill_requests() accessor Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181537 [details] [review] Bug 39600: Rewrite API controllers This patch splits the controller in two: * A controller for staff users use * An unprivileged access controller A new endpoint is added for publi access: * `/public/patrons/:patron_id/ill/requests` The public one makes use of the state-of-the-art in Koha for public endpoints, using the `$c->auth->public` validation helper and leaving the (newly added) public path spec with no special permissions requirements. The staff-oriented endpoint gets the right permissions added (at least those used in the main `/ill/requests` endpoint. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove t/db_dependent/api/v1/ill_requests.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181538 [details] [review] Bug 39600: Adapt template and use kohaTable This patch makes changes to the table introduced in the original patch: * It gets rendered using `kohaTable` instead of manually crafting the datatable * Embeds are parameters for `kohaTable` thus no need to build the headers manually * A default render method is introduced, which HTML-escapes the data for safety, and also deals with `null` values which could cause problems. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> This is great work and it's great to see such a team effort. Passing QA, thanks all. PS: I updated the Sponsored-by line.. the URL needs to be within <> rather than () got the scripts to properly pick it up Pushed for 25.05! Well done everyone, thank you! Created attachment 181608 [details] [review] Bug 39600: Add server-side rendering to OPAC datatables If commit 03dd9efb72d06dee6dc256b8d2e138ed7fc7b3ca gets reverted, we will still need to implement the server-side rendering feature from Staff. This patch does that. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> xt/api.t .. 3/6 # Failed test 'No errors in 400 definitions in the spec' # at xt/api.t line 168. # got: '2' # expected: '0' get /patrons/{patron_id}/ill/requests -> 'description' does not include '* `invalid_query`': (Bad request) get /public/patrons/{patron_id}/ill/requests -> 'description' does not include '* `invalid_query`': (Bad request) # Looks like you failed 1 test of 1. Created attachment 181652 [details] [review] Bug 39600: Fix spec to include invalid query handling Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi (tcohen) from comment #53) > Created attachment 181652 [details] [review] [review] > Bug 39600: Fix spec to include invalid query handling > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to main (very soon). |