Bug 35105 - ILL - Saving 'Edit request' form with invalid Patron ID causes ILL table to not render
Summary: ILL - Saving 'Edit request' form with invalid Patron ID causes ILL table to n...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Pedro Amorim
QA Contact: Martin Renvoize
URL:
Keywords: rel_22_11_candidate, rel_23_05_candidate
Depends on: 35094
Blocks:
  Show dependency treegraph
 
Reported: 2023-10-19 10:28 UTC by Pedro Amorim
Modified: 2023-11-29 10:57 UTC (History)
8 users (show)

See Also:
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


Attachments
Bug 35105: Fix patron accessor in Illrequest.pm (1.95 KB, patch)
2023-10-19 10:48 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35105: Fix patron accessor in Illrequest.pm (2.01 KB, patch)
2023-10-19 10:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35105: Regression tests (2.12 KB, patch)
2023-10-19 18:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35105: Fix patron accessor in Illrequest.pm (2.01 KB, patch)
2023-10-19 18:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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