Bug 35105

Summary: ILL - Saving 'Edit request' form with invalid Patron ID causes ILL table to not render
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: ILLAssignee: Pedro Amorim <pjamorim91>
Status: Pushed to oldstable --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: major    
Priority: P5 - low CC: fridolin.somers, katrin.fischer, lucas, magnus, martin.renvoize, matt.blenkinsop, pedro.amorim, tomascohen
Version: MainKeywords: rel_22_11_candidate, rel_23_05_candidate
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35106
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.05,22.11.12
Bug Depends on: 35094    
Bug Blocks:    
Attachments: Bug 35105: Fix patron accessor in Illrequest.pm
Bug 35105: Fix patron accessor in Illrequest.pm
Bug 35105: Regression tests
Bug 35105: Fix patron accessor in Illrequest.pm

Description Pedro Amorim 2023-10-19 10:28:01 UTC
To reproduce, k-t-d:
1) Install FreeForm and enable ILLmodule, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Visit /cgi-bin/koha/ill/ill-requests.pl
3) Create 'New ILL request'
4) Select the request type, input cardnumber '42' and select a library
5) Hit 'Create'
6) Manage the request created just now: click the request id or 'manage request' button from the table
7) Click "Edit request" from the top actions toolbar
8a) Input text in Patron ID e.g. 'asdasdasf'. Notice you get redirected to the table - that doesn't render - with an error.
8b) Input a patron id that doesn't exist e.g. '987654'. Notice you get an error upon saving "FK Constraint error"
Comment 1 Pedro Amorim 2023-10-19 10:48:56 UTC
Created attachment 157417 [details] [review]
Bug 35105: Fix patron accessor in Illrequest.pm

Patron is nullable, so we need to consider undef return

Test plan, k-t-d:
1) Install FreeForm and enable ILLmodule, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Visit /cgi-bin/koha/ill/ill-requests.pl
3) Create 'New ILL request'
4) Select the request type, input cardnumber '42' and select a library
5) Hit 'Create'
6) Manage the request created just now: click the request id or 'manage request' button from the table
7) Click "Edit request" from the top actions toolbar
8a) Input text in Patron ID e.g. 'asdasdasf'. Notice you get redirected to the table - now renders correctly.
8b) Input a patron id that doesn't exist e.g. '987654'. Notice you get an error upon saving "FK Constraint error",
go back to the table and verify it renders correctly.

In both instances, the request is saved with borrowernumber as NULL,
this was preventing the table from rendering because the 'patron' API
embed was trying to retrieve a patron - getting an empty resultset instead
of undef.
Comment 2 Martin Renvoize 2023-10-19 10:56:43 UTC
Created attachment 157418 [details] [review]
Bug 35105: Fix patron accessor in Illrequest.pm

Patron is nullable, so we need to consider undef return

Test plan, k-t-d:
1) Install FreeForm and enable ILLmodule, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Visit /cgi-bin/koha/ill/ill-requests.pl
3) Create 'New ILL request'
4) Select the request type, input cardnumber '42' and select a library
5) Hit 'Create'
6) Manage the request created just now: click the request id or 'manage request' button from the table
7) Click "Edit request" from the top actions toolbar
8a) Input text in Patron ID e.g. 'asdasdasf'. Notice you get redirected to the table - now renders correctly.
8b) Input a patron id that doesn't exist e.g. '987654'. Notice you get an error upon saving "FK Constraint error",
go back to the table and verify it renders correctly.

In both instances, the request is saved with borrowernumber as NULL,
this was preventing the table from rendering because the 'patron' API
embed was trying to retrieve a patron - getting an empty resultset instead
of undef.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2023-10-19 10:58:07 UTC
Fast QA of this trivial patch.. all working perfectly and following prior art.

Passing QA
Comment 4 Pedro Amorim 2023-10-19 11:36:46 UTC
I setting this as rel 23.05 and 22.11 candidates as this bug is actually a follow-up of bug 22440. 

Still requires bug 35094 (not backportable to those releases) to test on current master.
Comment 5 Pedro Amorim 2023-10-19 11:45:31 UTC
Updated bug title to better reflect the issue being fixed here: we're fixing the table not rendering, not the explosion that happens upon inserting invalid data, that is fixed in bug 35106.
Comment 6 Tomás Cohen Arazi 2023-10-19 18:07:18 UTC
Created attachment 157474 [details] [review]
Bug 35105: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2023-10-19 18:07:21 UTC
Created attachment 157475 [details] [review]
Bug 35105: Fix patron accessor in Illrequest.pm

Patron is nullable, so we need to consider undef return

Test plan, k-t-d:
1) Install FreeForm and enable ILLmodule, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Visit /cgi-bin/koha/ill/ill-requests.pl
3) Create 'New ILL request'
4) Select the request type, input cardnumber '42' and select a library
5) Hit 'Create'
6) Manage the request created just now: click the request id or 'manage request' button from the table
7) Click "Edit request" from the top actions toolbar
8a) Input text in Patron ID e.g. 'asdasdasf'. Notice you get redirected to the table - now renders correctly.
8b) Input a patron id that doesn't exist e.g. '987654'. Notice you get an error upon saving "FK Constraint error",
go back to the table and verify it renders correctly.

In both instances, the request is saved with borrowernumber as NULL,
this was preventing the table from rendering because the 'patron' API
embed was trying to retrieve a patron - getting an empty resultset instead
of undef.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Tomás Cohen Arazi 2023-10-19 19:02:41 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 9 Fridolin Somers 2023-10-25 08:18:15 UTC
Pushed to 23.05.x for 23.05.05
Comment 10 Matt Blenkinsop 2023-11-13 13:59:35 UTC
Dependency missing for 22.11.x - not backporting
Comment 11 Matt Blenkinsop 2023-11-13 14:30:25 UTC
Ignore previous comment - backported to 22.11.x