Bug 35797 - REST API: Add GET route for patronimage
Summary: REST API: Add GET route for patronimage
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Shi Yao Wang
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-12 17:19 UTC by Shi Yao Wang
Modified: 2024-01-30 00:30 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35797: REST API Add GET route for patronimage (6.77 KB, patch)
2024-01-12 17:44 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 35797: REST API Add GET route for patronimage (6.83 KB, patch)
2024-01-23 10:51 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 35797: return the image directly instead of base64 in json (4.91 KB, patch)
2024-01-29 16:48 UTC, Shi Yao Wang
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Shi Yao Wang 2024-01-12 17:19:07 UTC
Add GET route to get patronimage database table data
Comment 1 Shi Yao Wang 2024-01-12 17:44:26 UTC
Created attachment 160993 [details] [review]
Bug 35797: REST API Add GET route for patronimage

Api changes adding GET route for patronimage

Test plan:
1- Apply the patch
2- Go to administration -> sys.pref. -> set patronimages to allow
3- Log in koha using a user with edit_borrowers privileges
4- Have a patron with a profile image (add an image if necessary by
searching for a patron and then clicking on the default profile image on
the top left)
5- Open a new browser tab, write
{intranet_url}/api/v1/patrons/{borrowernumber}/image
where {intranet_url} is the base intranet url of koha and {borrowernumber}
is the borrowernumber of a borrower that has an image
6- Notice the response is a json with 3 keys: imagefile, mimetype and
patron_id which corresponds to the fields of patronimage table in the
database. Note that imagefile in db and in json response are different
because it has been base64 encoded in order to be sent as a string in json
7- Do step 5 with a borrowernumber of a patron without an image and a
borrowernumber of a patron that doesn't exist (and other edge cases
where there should be an error message displayed such as "Access
forbidden", "Authentication required", etc.)
8- Notice an appropriate error message is displayed
Comment 2 Shi Yao Wang 2024-01-12 17:47:34 UTC
First time adding a REST API route. Please feel free to give advice and correction.
Comment 3 Biblibre Sandboxes 2024-01-23 10:51:32 UTC
Created attachment 161273 [details] [review]
Bug 35797: REST API Add GET route for patronimage

Api changes adding GET route for patronimage

Test plan:
1- Apply the patch
2- Go to administration -> sys.pref. -> set patronimages to allow
3- Log in koha using a user with edit_borrowers privileges
4- Have a patron with a profile image (add an image if necessary by
searching for a patron and then clicking on the default profile image on
the top left)
5- Open a new browser tab, write
{intranet_url}/api/v1/patrons/{borrowernumber}/image
where {intranet_url} is the base intranet url of koha and {borrowernumber}
is the borrowernumber of a borrower that has an image
6- Notice the response is a json with 3 keys: imagefile, mimetype and
patron_id which corresponds to the fields of patronimage table in the
database. Note that imagefile in db and in json response are different
because it has been base64 encoded in order to be sent as a string in json
7- Do step 5 with a borrowernumber of a patron without an image and a
borrowernumber of a patron that doesn't exist (and other edge cases
where there should be an error message displayed such as "Access
forbidden", "Authentication required", etc.)
8- Notice an appropriate error message is displayed

Signed-off-by: Martin AUBEUT <martin.aubeut@gmail.com>
Comment 4 Tomás Cohen Arazi 2024-01-26 18:59:46 UTC
Some important remarks:

- For retrieving the image, you need to know the `patron_id` on the consumer code (i.e. the path is /patrons/{patron_id}/image), so why returning the patron_id attribute at all?
- Why not return the raw/binary image so this is usable on a browser without extra processing on the client side? Look at the biblios endpoint for examples [1]
- The MIME type should be returned as a header [2]
- The code is not tidy [3]

Not a real blocker:
- Most of the endpoints we have follow a pattern:
  GET    /objects
  POST   /objects
  GET    /objects/:object_id
  PUT    /objects/:object_id
  DELETE /objects/:object_id
In this particular case, there's no object_id because the patronimage table doesn't even have an auto-increment value. I would like you to think if there could be a use case for which having more than one image per patron could be desirable (in the future). If such was the case, we could do something like
GET /patrons/:patron_id/default_image
instead of the current route, envisioning a future addition of a patron_image_id and being able to add many images per patron.

I'm not asking you to go implement such thing, but to think of a future-proof RESTful API design.


[1] Here you can see we return raw binary MARC data https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/REST/V1/Biblios.pm#L94
[2] https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/REST/V1.pm#L71
[3] There's a coding guideline for this, but basically follow this instructions to set your dev environment: https://wiki.koha-community.org/wiki/Perltidy
Comment 5 Shi Yao Wang 2024-01-29 16:48:19 UTC
Created attachment 161610 [details] [review]
Bug 35797: return the image directly instead of base64 in json

Test plan:
1- Apply the patch
2- Go to administration -> sys.pref. -> set patronimages to allow
3- Log in koha using a user with edit_borrowers privileges
4- Have a patron with a profile image (add an image if necessary by
searching for a patron and then clicking on the default profile image on
the top left)
5- Open a new browser tab, write
{intranet_url}/api/v1/patrons/{borrowernumber}/default_image
where {intranet_url} is the base intranet url of koha and {borrowernumber}
is the borrowernumber of a borrower that has an image
6- Notice the image displays in the browser
7- Do step 5 with a borrowernumber of a patron without an image and a
borrowernumber of a patron that doesn't exist (and other edge cases
where there should be an error message displayed such as "Access
forbidden", "Authentication required", etc.)
8- Notice an appropriate error message is displayed
Comment 6 Shi Yao Wang 2024-01-29 17:04:24 UTC
All remarks have been (hopefully well) implemented.

I am not sure about a patron having multiple images since I currently cannot see any practical reason for such a thing, but I renamed the route just in case since it's trivial to do.

One thing I thought I should comment on is that currently all images uploaded as patron image seem to be converted to png. So I hardcoded the return format for simplicity. Else (if say we preserve the image format), I think we would need some sort of mapping of koha db patronimage mimetype to mojolicious accepted format option (https://docs.mojolicious.org/Mojolicious/Types#DESCRIPTION)