Bug 26271

Summary: Call to /api/v1/patrons/{patron_id}/account returns 500 error if manager_id is NULL
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: REST APIAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: aleisha, fridolin.somers, jonathan.druart, lucas, martin.renvoize, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26332
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
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
Bug 26271: Add null to the list of accepted account_line data types
Bug 26271: Add null to the list of accepted account_line data types
Bug 26271: Add failing test to reveal issue with patrons API endpoint
Bug 26271: Add null to the list of accepted account_line data types
Bug 26271: Add failing test to reveal issue with patrons API endpoint
Bug 26271: Add null to the list of accepted account_line data types

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.