Bug 39600 - Paginate (and modernize) OPAC ILL requests table
Summary: Paginate (and modernize) OPAC ILL requests table
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: 24.11
Hardware: All All
: P5 - low enhancement
Assignee: HKS3 Tadeusz Sośnierz
QA Contact: Pedro Amorim
URL:
Keywords:
: 39450 (view as bug list)
Depends on:
Blocks: 38339 39446 39610
  Show dependency treegraph
 
Reported: 2025-04-10 09:49 UTC by HKS3 Tadeusz Sośnierz
Modified: 2025-04-15 09:37 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Switch OPAC ILL request listing to server-side rendering (13.85 KB, patch)
2025-04-10 09:51 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Implment public_read_lists for ILL::Request and ::Attribute (1.84 KB, patch)
2025-04-10 09:51 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 39460: Fix koha-upgrade-schema when no DB change needed (2.31 KB, patch)
2025-04-10 11:14 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Switch OPAC ILL request listing to server-side rendering (13.76 KB, patch)
2025-04-10 11:16 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Implment public_read_lists for ILL::Request and ::Attribute (1.84 KB, patch)
2025-04-10 11:16 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 39600: Switch OPAC ILL request listing to server-side rendering (13.83 KB, patch)
2025-04-10 11:20 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute (1.90 KB, patch)
2025-04-10 11:20 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 39600: Add tests for patron_list (3.66 KB, patch)
2025-04-10 13:36 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 39600: Switch OPAC ILL request listing to server-side rendering (14.41 KB, patch)
2025-04-10 13:45 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 39600: Switch OPAC ILL request listing to server-side rendering (14.48 KB, patch)
2025-04-10 16:29 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute (1.96 KB, patch)
2025-04-10 16:29 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 39600: Add tests for patron_list (3.72 KB, patch)
2025-04-10 16:29 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter (7.02 KB, patch)
2025-04-11 09:46 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description HKS3 Tadeusz Sośnierz 2025-04-10 09:49:24 UTC
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.
Comment 1 HKS3 Tadeusz Sośnierz 2025-04-10 09:51:38 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.
Comment 2 HKS3 Tadeusz Sośnierz 2025-04-10 09:51:41 UTC
Created attachment 180759 [details] [review]
Implment public_read_lists for ILL::Request and ::Attribute
Comment 3 HKS3 Tadeusz Sośnierz 2025-04-10 09:53:26 UTC
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/)
Comment 4 Katrin Fischer 2025-04-10 09:56:31 UTC
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.
Comment 5 Pedro Amorim 2025-04-10 11:12:40 UTC
*** Bug 38339 has been marked as a duplicate of this bug. ***
Comment 6 HKS3 Tadeusz Sośnierz 2025-04-10 11:14:05 UTC Comment hidden (obsolete)
Comment 7 HKS3 Tadeusz Sośnierz 2025-04-10 11:14:26 UTC
^ Added accidentally, ignore.
Comment 8 Pedro Amorim 2025-04-10 11:16:09 UTC
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
Comment 9 HKS3 Tadeusz Sośnierz 2025-04-10 11:16:26 UTC
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/)
Comment 10 HKS3 Tadeusz Sośnierz 2025-04-10 11:16:29 UTC
Created attachment 180762 [details] [review]
Implment public_read_lists for ILL::Request and ::Attribute
Comment 11 HKS3 Tadeusz Sośnierz 2025-04-10 11:20:47 UTC
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/)
Comment 12 HKS3 Tadeusz Sośnierz 2025-04-10 11:20:49 UTC
Created attachment 180764 [details] [review]
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute

Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Comment 13 Pedro Amorim 2025-04-10 11:51:02 UTC
*** Bug 39450 has been marked as a duplicate of this bug. ***
Comment 14 Pedro Amorim 2025-04-10 11:52:34 UTC
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.
Comment 15 Pedro Amorim 2025-04-10 12:01:48 UTC
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.
Comment 16 HKS3 Tadeusz Sośnierz 2025-04-10 13:36:19 UTC
Created attachment 180778 [details] [review]
Bug 39600: Add tests for patron_list

Sponsored-by: Wiko (https://www.wiko-berlin.de/)
Comment 17 HKS3 Tadeusz Sośnierz 2025-04-10 13:45:33 UTC
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/)
Comment 18 Pedro Amorim 2025-04-10 16:29:50 UTC
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>
Comment 19 Pedro Amorim 2025-04-10 16:29:54 UTC
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>
Comment 20 Pedro Amorim 2025-04-10 16:29:57 UTC
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>
Comment 21 Pedro Amorim 2025-04-10 16:33:37 UTC
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".
Comment 22 HKS3 Tadeusz Sośnierz 2025-04-11 09:46:56 UTC
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/)
Comment 23 Pedro Amorim 2025-04-11 15:33:04 UTC
Moving this to PQA to help move along as it's quite valuable work.
Thanks for following up with the feedback Tadeusz!