| Summary: | REST API: Add GET route for patronimage | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Shi Yao Wang <shi-yao.wang> |
| Component: | REST API | Assignee: | Shi Yao Wang <shi-yao.wang> |
| Status: | Signed Off --- | QA Contact: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Severity: | enhancement | ||
| Priority: | P5 - low | CC: | caroline.cyr-la-rose, david, dcook, jonathan.druart, lucas, martin.aubeut, philippe.blouin, tomascohen |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Comma delimited list of Sponsors: | |
| Crowdfunding goal: | 0 | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
This enhancement adds a new API route to request a patron image, if it exists. Errors are returned if a patron number doesn't exist or there is no patron image.
Example API request: http://127.0.0.1:8081/api/v1/patrons/49/default_image
Error messages:
- If there is no patron image: Patron image not found.
- If the patron doesn't exist: Patron not found.
|
Version(s) released in: | |
| Circulation function: | |||
| Attachments: |
Bug 35797: REST API Add GET route for patronimage
Bug 35797: REST API Add GET route for patronimage Bug 35797: return the image directly instead of base64 in json Bug 35797: REST API Add GET route for patronimage Bug 35797: REST API Add GET route for patronimage Bug 35797: REST API Add GET route for patronimage Bug 35797: REST API Add GET route for patronimage Bug 35797: Add tests Bug 35797: REST API Add GET route for patronimage Bug 35797: Add tests Bug 35797: REST API Add GET route for patronimage Bug 35797: Add tests |
||
|
Description
Shi Yao Wang
2024-01-12 17:19:07 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 First time adding a REST API route. Please feel free to give advice and correction. 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> 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
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 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) Created attachment 170137 [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- Run `npm run api:bundle` 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 an API testing tool (such as postman), 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- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage api keys) and generate a token 7- Send the request and notice the image of the patron is returned 8- Do step 5-7 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.) 9- Notice an appropriate error message is displayed I'm curious about the permissions. I haven't checked but should "list_borrowers" be able to access this too? It would make sense yes. Didn't have that privilege in the past I believe. Adding it Created attachment 170186 [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- Run `npm run api:bundle` 2- Go to administration -> sys.pref. -> set patronimages to allow 3- Log in koha using a user with "list_borrowers" or "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 an API testing tool (such as postman), 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- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage api keys) and generate a token 7- Send the request and notice the image of the patron is returned 8- Do step 5-7 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.) 9- Notice an appropriate error message is displayed Created attachment 172201 [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- Run `npm run api:bundle` 2- Go to administration -> sys.pref. -> set patronimages to allow 3- Log in koha using a user with "list_borrowers" or "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 an API testing tool (such as postman), 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- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage api keys) and generate a token 7- Send the request and notice the image of the patron is returned 8- Do step 5-7 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.) 9- Notice an appropriate error message is displayed Signed-off-by: David Nind <david@davidnind.com> Please provide tests. Created attachment 176695 [details] [review] Bug 35797: REST API Add GET route for patronimage Api changes adding GET route for patronimage To run the tests, run 'prove t/db_dependent/api/v1/image.t' Created attachment 176696 [details] [review] Bug 35797: Add tests To run the tests, run 'prove t/db_dependent/api/v1/image.t' (In reply to William Lavoie from comment #13) > Created attachment 176695 [details] [review] [review] > Bug 35797: REST API Add GET route for patronimage > > Api changes adding GET route for patronimage > To run the tests, run 'prove t/db_dependent/api/v1/image.t' We lost the original test plan, and the `signed-off by` line The test plan is the same as the one described above in comment 11, in addition to running 'prove t/db_dependent/api/v1/image.t'. Since the sign-off applied to the previous test plan, we need a new one. Test plan: 1- Apply the patch 2- Run `npm run api:bundle` 2- Go to administration -> sys.pref. -> set patronimages to allow 3- Log in koha using a user with "list_borrowers" or "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 an API testing tool (such as postman), 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- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage api keys) and generate a token 7- Send the request and notice the image of the patron is returned 8- Do step 5-7 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.) 9- Notice an appropriate error message is displayed 10- Run 'prove t/db_dependent/api/v1/image.t' Several problems from the QA tool here: [FAIL] t/db_dependent/api/v1/image.t FAIL file_permissions File must have the exec flag FAIL forbidden_patterns forbidden pattern: Use https:// instead of http:// for URLs (line 16) FAIL test_no_warnings 'use Test::NoWarnings' must be present in this test file. FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/api/v1/image.t` Created attachment 189432 [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- Run `npm run api:bundle` 2- Go to administration -> sys.pref. -> set patronimages to allow 3- Log in koha using a user with "list_borrowers" or "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 an API testing tool (such as postman), 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- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage api keys) and generate a token 7- Send the request and notice the image of the patron is returned 8- Do step 5-7 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.) 9- Notice an appropriate error message is displayed Signed-off-by: David Nind <david@davidnind.com> Created attachment 189433 [details] [review] Bug 35797: Add tests To run the tests, run 'prove t/db_dependent/api/v1/image.t' Fixed QA issues mentionned Created attachment 190727 [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- Run `npm run api:bundle` 2- Go to administration -> sys.pref. -> set patronimages to allow 3- Log in koha using a user with "list_borrowers" or "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 an API testing tool (such as postman), 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- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage api keys) and generate a token 7- Send the request and notice the image of the patron is returned 8- Do step 5-7 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.) 9- Notice an appropriate error message is displayed Signed-off-by: David Nind <david@davidnind.com> Created attachment 190728 [details] [review] Bug 35797: Add tests To run the tests, run 'prove t/db_dependent/api/v1/image.t' Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. Before applying the patch, check the API documentation at http://127.0.0.1:8080/api/v1/.html and search for getPatronImage - you shouldn't get any results. 2. Apply the patches 3. Rebuild the API and restart everything: . yarn build . restart_all 4. Repeat step 1, and now you should get some results. 5. I added an image to Mary Burton - borrowernumber = 49 6. Using Bruno API tool, with basic authentication (koha, koha), I made a GET request to GET http://127.0.0.1:8081/api/v1/patrons/49/default_image and an image was returned . 7. With an invalid patron - 999: GET http://127.0.0.1:8081/api/v1/patrons/999/default_image ==> Error message is: { "error": "Patron not found." } 8. With a patron without an image - 51: GET http://127.0.0.1:8081/api/v1/patrons/51/default_image ===> Error message is: { "error": "Patron image not found." } 9. I didn't test using OAuth 2.0 as I couldn't figure out how to use Brun for this, but I did work out part of it. 9.1 To generate a token to test OAuth 2.0 - Set RESTOAuth2ClientCredentials = enable - Generate the credentials for a patron: Patrons > [selected patron] > More > Manage API keys > Generate a new client id/secret pair 10. The tests pass: prove t/db_dependent/api/v1/image.t |