Bug 39600

Summary: Use the API to render the OPAC ILL requests table
Product: Koha Reporter: HKS3 Tadeusz Sośnierz <tadeusz>
Component: ILLAssignee: HKS3 Tadeusz Sośnierz <tadeusz>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: andrew.auld, jonathan.druart, katrin.fischer, magnus, martin.renvoize, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
This patch modernizes the ILL requests page in the OPAC by making it render using the API. New API endpoints are added for this purpose. This includes adding pagination and search features to the table previously not available.
Version(s) released in:
25.05.00
Circulation function:
Bug Depends on: 39739, 39762    
Bug Blocks: 39446, 39610    
Attachments: Switch OPAC ILL request listing to server-side rendering
Implment public_read_lists for ILL::Request and ::Attribute
Bug 39460: Fix koha-upgrade-schema when no DB change needed
Switch OPAC ILL request listing to server-side rendering
Implment public_read_lists for ILL::Request and ::Attribute
Bug 39600: Switch OPAC ILL request listing to server-side rendering
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute
Bug 39600: Add tests for patron_list
Bug 39600: Switch OPAC ILL request listing to server-side rendering
Bug 39600: Switch OPAC ILL request listing to server-side rendering
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute
Bug 39600: Add tests for patron_list
Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter
Bug 39600: Switch OPAC ILL request listing to server-side rendering
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute
Bug 39600: Add tests for patron_list
Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter
Bug 39600: Switch OPAC ILL request listing to server-side rendering
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute
Bug 39600: Add tests for patron_list
Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter
Bug 39600: Add Koha::Patron->ill_requests() accessor
Bug 39600: Rewrite API controllers
Bug 39600: Adapt template and use kohaTable
Bug 39600: Add Koha::Patron->ill_requests() accessor
Bug 39600: Rewrite API controllers
Bug 39600: Adapt template and use kohaTable
Bug 39600: Switch OPAC ILL request listing to server-side rendering
Bug 39600: Implement public_read_lists for ILL::Request and ::Attribute
Bug 39600: Add tests for patron_list
Bug 39600: Cleanup the changes -- tidy, document, add a missing TT filter
Bug 39600: Add Koha::Patron->ill_requests() accessor
Bug 39600: Rewrite API controllers
Bug 39600: Adapt template and use kohaTable
Bug 39600: Add server-side rendering to OPAC datatables
Bug 39600: Fix spec to include invalid query handling

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!
Comment 24 Tomás Cohen Arazi (tcohen) 2025-04-17 19:11:43 UTC
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>
Comment 25 Tomás Cohen Arazi (tcohen) 2025-04-17 19:11:46 UTC
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>
Comment 26 Tomás Cohen Arazi (tcohen) 2025-04-17 19:11:49 UTC
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>
Comment 27 Tomás Cohen Arazi (tcohen) 2025-04-17 19:11:51 UTC
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>
Comment 28 Tomás Cohen Arazi (tcohen) 2025-04-23 14:26:32 UTC
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.
Comment 29 Tomás Cohen Arazi (tcohen) 2025-04-23 14:34:21 UTC
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.
Comment 30 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:32 UTC
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>
Comment 31 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:35 UTC
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>
Comment 32 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:38 UTC
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>
Comment 33 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:40 UTC
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>
Comment 34 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:43 UTC
Created attachment 181441 [details] [review]
Bug 39600: Add Koha::Patron->ill_requests() accessor

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:46 UTC
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>
Comment 36 Tomás Cohen Arazi (tcohen) 2025-04-24 13:11:49 UTC
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>
Comment 37 Tomás Cohen Arazi (tcohen) 2025-04-24 13:24:11 UTC
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.
Comment 38 HKS3 Tadeusz Sośnierz 2025-04-25 12:49:54 UTC
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>
Comment 39 HKS3 Tadeusz Sośnierz 2025-04-25 12:49:57 UTC
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>
Comment 40 HKS3 Tadeusz Sośnierz 2025-04-25 12:50:00 UTC
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>
Comment 41 Martin Renvoize (ashimema) 2025-04-25 14:52:20 UTC
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>
Comment 42 Martin Renvoize (ashimema) 2025-04-25 14:52:23 UTC
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>
Comment 43 Martin Renvoize (ashimema) 2025-04-25 14:52:26 UTC
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>
Comment 44 Martin Renvoize (ashimema) 2025-04-25 14:52:29 UTC
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>
Comment 45 Martin Renvoize (ashimema) 2025-04-25 14:52:33 UTC
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>
Comment 46 Martin Renvoize (ashimema) 2025-04-25 14:52:36 UTC
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>
Comment 47 Martin Renvoize (ashimema) 2025-04-25 14:52:39 UTC
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>
Comment 48 Martin Renvoize (ashimema) 2025-04-25 14:54:20 UTC
This is great work and it's great to see such a team effort.

Passing QA, thanks all.
Comment 49 Martin Renvoize (ashimema) 2025-04-25 14:55:02 UTC
PS: I updated the Sponsored-by line.. the URL needs to be within <> rather than () got the scripts to properly pick it up
Comment 50 Katrin Fischer 2025-04-25 17:28:27 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 51 Tomás Cohen Arazi (tcohen) 2025-04-28 12:46:53 UTC
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>
Comment 52 Jonathan Druart 2025-04-29 12:56:13 UTC
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.
Comment 53 Tomás Cohen Arazi (tcohen) 2025-04-29 13:01:36 UTC
Created attachment 181652 [details] [review]
Bug 39600: Fix spec to include invalid query handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Katrin Fischer 2025-04-29 13:55:28 UTC
(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).