Bug 26271 - Call to /api/v1/patrons/{patron_id}/account returns 500 error if manager_id is NULL
Summary: Call to /api/v1/patrons/{patron_id}/account returns 500 error if manager_id i...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Joonas Kylmälä
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-21 13:21 UTC by Joonas Kylmälä
Modified: 2020-11-27 14:15 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04, 19.11.10


Attachments
Bug 26271: Add failing test to reveal issue with patrons API endpoint (2.33 KB, patch)
2020-08-21 13:31 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26271: Add null to the list of accepted account_line data types (1.27 KB, patch)
2020-08-21 13:31 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26271: Add null to the list of accepted account_line data types (1.33 KB, patch)
2020-09-03 08:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26271: Add failing test to reveal issue with patrons API endpoint (2.38 KB, patch)
2020-09-03 08:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26271: Add null to the list of accepted account_line data types (1.33 KB, patch)
2020-09-03 08:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26271: Add failing test to reveal issue with patrons API endpoint (2.44 KB, patch)
2020-09-03 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26271: Add null to the list of accepted account_line data types (1.40 KB, patch)
2020-09-03 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2020-08-21 13:21:33 UTC
Making a call to /api/v1/patrons/<patron_id>/account where the patron has an accountline with manager_id = NULL causes the request to fail with error code 500 because the swagger spec defines only integer as user_id/manager_id type. It should also accept null types because fines.pl cron seems to create accountlines with manager_id=NULL.
Comment 1 Joonas Kylmälä 2020-08-21 13:31:06 UTC
Created attachment 108820 [details] [review]
Bug 26271: Add failing test to reveal issue with patrons API endpoint

When manager_id is null/undef the API returns error code 500.

To test:
 1) Notice failure when running
    prove t/db_dependent/api/v1/patrons_accounts.t
Comment 2 Joonas Kylmälä 2020-08-21 13:31:08 UTC
Created attachment 108821 [details] [review]
Bug 26271: Add null to the list of accepted account_line data types

The database schema for accountlines table allows the
manager_id/user_id column to be NULL. If request to
/api/v1/patrons/<patron_id>/account returns such an accountline where
it is NULL we get 500 error as response. Adding NULL to allowed data
types fixes this issue.

To test:
 1) Run prove t/db_dependent/api/v1/patrons_accounts.t and notice it
    doesn't fail
Comment 3 Fridolin Somers 2020-09-03 08:22:22 UTC
Created attachment 109583 [details] [review]
Bug 26271: Add null to the list of accepted account_line data types

The database schema for accountlines table allows the
manager_id/user_id column to be NULL. If request to
/api/v1/patrons/<patron_id>/account returns such an accountline where
it is NULL we get 500 error as response. Adding NULL to allowed data
types fixes this issue.

To test:
 1) Run prove t/db_dependent/api/v1/patrons_accounts.t and notice it
    doesn't fail

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 4 Fridolin Somers 2020-09-03 08:22:51 UTC
Created attachment 109584 [details] [review]
Bug 26271: Add failing test to reveal issue with patrons API endpoint

When manager_id is null/undef the API returns error code 500.

To test:
 1) Notice failure when running
    prove t/db_dependent/api/v1/patrons_accounts.t

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 5 Fridolin Somers 2020-09-03 08:23:02 UTC
Created attachment 109585 [details] [review]
Bug 26271: Add null to the list of accepted account_line data types

The database schema for accountlines table allows the
manager_id/user_id column to be NULL. If request to
/api/v1/patrons/<patron_id>/account returns such an accountline where
it is NULL we get 500 error as response. Adding NULL to allowed data
types fixes this issue.

To test:
 1) Run prove t/db_dependent/api/v1/patrons_accounts.t and notice it
    doesn't fail

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 6 Fridolin Somers 2020-09-03 08:23:25 UTC
Looks perfect ;)
Comment 7 Martin Renvoize 2020-09-03 09:00:34 UTC
Created attachment 109591 [details] [review]
Bug 26271: Add failing test to reveal issue with patrons API endpoint

When manager_id is null/undef the API returns error code 500.

To test:
 1) Notice failure when running
    prove t/db_dependent/api/v1/patrons_accounts.t

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-09-03 09:00:40 UTC
Created attachment 109592 [details] [review]
Bug 26271: Add null to the list of accepted account_line data types

The database schema for accountlines table allows the
manager_id/user_id column to be NULL. If request to
/api/v1/patrons/<patron_id>/account returns such an accountline where
it is NULL we get 500 error as response. Adding NULL to allowed data
types fixes this issue.

To test:
 1) Run prove t/db_dependent/api/v1/patrons_accounts.t and notice it
    doesn't fail

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-09-03 09:01:02 UTC
Works as expected, well spotted.

Passing QA
Comment 10 Jonathan Druart 2020-09-03 11:05:37 UTC
That is something I was expected for a while. We are going to face this problem again and again.
We need a test to compare the API definition and the DB schema (datatype and nullable).
Comment 11 Jonathan Druart 2020-09-03 13:04:54 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 12 Lucas Gass 2020-09-14 21:39:02 UTC
backported to 20.05.x for 20.05.04
Comment 13 Aleisha Amohia 2020-09-15 11:43:22 UTC
backported to 19.11.x for 19.11.10
Comment 14 Victor Grousset/tuxayo 2020-09-20 19:20:55 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.