Add API route for checking renewability of an checkedout item.
Created attachment 54709 [details] [review] Bug 17003: Add API route to get checkout's renewability GET /checkouts/{checkout_id}/renewability (renewability status) Response: - 200: renewable (empty response object {}) - 403: not renewable (or not authorized to make renewals) To test: 1. Login to Koha 2. Checkout something for yourself 3. Make GET request to http://yourlibrary/api/v1/checkouts/YYY/renewability where YYY is checkout_id of your checkout. 4. Observe returned data and HTTP code (200 for renewable, 403 for not renewable) If not renewable, an error should also be presented. 5. Run t/db_dependent/api/v1/checkouts.t
Created attachment 54710 [details] [review] Bug 17003: Add API route to get checkout's renewability
Created attachment 56507 [details] [review] Bug 17003: Add API route to get checkout's renewability GET /checkouts/{checkout_id}/renewability (renewability status) Response: - 200: renewable (empty response object {}) - 403: not renewable (or not authorized to make renewals) To test: 1. Login to Koha 2. Checkout something for yourself 3. Make GET request to http://yourlibrary/api/v1/checkouts/YYY/renewability where YYY is checkout_id of your checkout. 4. Observe returned data and HTTP code (200 for renewable, 403 for not renewable) If not renewable, an error should also be presented. 5. Run t/db_dependent/api/v1/checkouts.t
Rebased on top of master.
Created attachment 57489 [details] [review] Bug 17003: Add API route to get checkout's renewability GET /checkouts/{checkout_id}/renewability (renewability status) Response: - 200: renewability { renewable: true/false, error: "error message" } To test: 1. Login to Koha 2. Checkout something for yourself 3. Make GET request to http://yourlibrary/api/v1/checkouts/YYY/renewability where YYY is checkout_id of your checkout. 4. Observe returned data. If not renewable, an error should also be presented. 5. Run t/db_dependent/api/v1/checkouts.t
I changed the return JSON. It was confusing in previous patch. Now we return HTTP 200 with: { "renewable": true, "error": null } ...or... { "renewable": false, "error": "too_many" }
Created attachment 57493 [details] [review] Bug 17003: Add API route to get checkout's renewability GET /checkouts/{checkout_id}/renewability (renewability status) Response: - 200: renewability { renewable: true/false, error: "error message" } To test: 1. Login to Koha 2. Checkout something for yourself 3. Make GET request to http://yourlibrary/api/v1/checkouts/YYY/renewability where YYY is checkout_id of your checkout. 4. Observe returned data. If not renewable, an error should also be presented. 5. Run t/db_dependent/api/v1/checkouts.t
Comment on attachment 57493 [details] [review] Bug 17003: Add API route to get checkout's renewability Review of attachment 57493 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/V1/Checkout.pm @@ +109,5 @@ > + $OpacRenewalAllowed = C4::Context->preference('OpacRenewalAllowed'); > + } > + > + unless ($user && ($OpacRenewalAllowed > + || haspermission($user->userid, { circulate => "circulate_remaining_permissions" }))) { This "haspermission" check should be moved to the path definition using "x-koha-authorization". Also when OpacRenewalAllowed syspref is disabled, the error should be returned sooner then possible "Checkout doesn't exist" error (don't give users hope to renew it when not found, but tell them renewal is not allowed at all instead). ::: api/v1/swagger/paths/checkouts.json @@ +98,5 @@ > + "get": { > + "operationId": "renewabilityCheckout", > + "tags": ["patrons", "checkouts"], > + "parameters": [{ > + "name": "checkout_id", I think this parameter should be exported in dependent bug 13895 and only referenced here instead of defined again.
Created attachment 58099 [details] [review] Bug 17003: Add API route to get checkout's renewability GET /checkouts/{checkout_id}/renewability (renewability status) Response: - 200: renewability { renewable: true/false, error: "error message" } To test: 1. Login to Koha 2. Checkout something for yourself 3. Make GET request to http://yourlibrary/api/v1/checkouts/YYY/renewability where YYY is checkout_id of your checkout. 4. Observe returned data. If not renewable, an error should also be presented. 5. Run t/db_dependent/api/v1/checkouts.t Signed-off-by: Jiri Kozlovsky <mail@jkozlovsky.cz> I've signed-off this because I've made no major change and the funcionality remains the same. Changes made: 1. Added x-koha-authorization to the paths definition 2. Changed checkout_id parameter definition to the reference 3. Moved error raised when syspref OpacRenewalAllowed disabled in front of possible error "Checkout doesn't exist" (don't give users hope to renew it when not found, but tell them renewal is not allowed at all instead)
(In reply to Jiri Kozlovsky from comment #8) > Comment on attachment 57493 [details] [review] [review] > Bug 17003: Add API route to get checkout's renewability > > Review of attachment 57493 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/REST/V1/Checkout.pm > @@ +109,5 @@ > > + $OpacRenewalAllowed = C4::Context->preference('OpacRenewalAllowed'); > > + } > > + > > + unless ($user && ($OpacRenewalAllowed > > + || haspermission($user->userid, { circulate => "circulate_remaining_permissions" }))) { > > This "haspermission" check should be moved to the path definition using > "x-koha-authorization". > Sorry about that inconvenience - I haven't realized that ownership check is required. It would be nice to have bug 17479 implmemented soon so that we avoid such misunderstandings :) .. btw how about setting that bug as a dependency and rewriting this code?
Depends on a FQA
Created attachment 85475 [details] [review] Bug 17003: Add API route to get checkout's renewability GET /checkouts/{checkout_id}/renewability (renewability status) Response: - 200: renewability { renewable: true/false, error: "error message" } To test: 1. Login to Koha 2. Checkout something for yourself 3. Make GET request to http://yourlibrary/api/v1/checkouts/YYY/renewability where YYY is checkout_id of your checkout. 4. Observe returned data. If not renewable, an error should also be presented. 5. Run t/db_dependent/api/v1/checkouts.t Signed-off-by: Jiri Kozlovsky <mail@jkozlovsky.cz> I've signed-off this because I've made no major change and the funcionality remains the same. Changes made: 1. Added x-koha-authorization to the paths definition 2. Changed checkout_id parameter definition to the reference 3. Moved error raised when syspref OpacRenewalAllowed disabled in front of possible error "Checkout doesn't exist" (don't give users hope to renew it when not found, but tell them renewal is not allowed at all instead)
Created attachment 85476 [details] [review] Bug 17003: Adapt to OpenAPI Test plan: prove t/db_dependent/api/v1/checkouts.t
Created attachment 87155 [details] [review] Bug 17003: Add API route to get checkout's renewability
Created attachment 87156 [details] [review] Bug 17003: Adapt to OpenAPI Test plan: prove t/db_dependent/api/v1/checkouts.t
Created attachment 87157 [details] [review] Bug 17003: (follow-up) Update definitions according to voted RFC Test plan: 1) Have some patrons with checkouts, some renewable and some not renewable 2) Use your favorite API tester and access GET http://koha.url/api/v1/checkouts/{checkout_id}/allows_renewal 3) Check the response is OK according to voted RFC: https://wiki.koha-community.org/wiki/Checkouts_endpoint_RFC#Checkout_renewability_2 4) prove t/db_dependent/api/v1/checkouts.t
Created attachment 87445 [details] [review] Bug 17003: Adapt to OpenAPI Test plan: prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 87446 [details] [review] Bug 17003: (follow-up) Update definitions according to voted RFC Test plan: 1) Have some patrons with checkouts, some renewable and some not renewable 2) Use your favorite API tester and access GET http://koha.url/api/v1/checkouts/{checkout_id}/allows_renewal 3) Check the response is OK according to voted RFC: https://wiki.koha-community.org/wiki/Checkouts_endpoint_RFC#Checkout_renewability_2 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 88260 [details] [review] Bug 17003: Add API route to get checkout's renewability
Created attachment 88261 [details] [review] Bug 17003: Adapt to OpenAPI Test plan: prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 88262 [details] [review] Bug 17003: (follow-up) Update definitions according to voted RFC Test plan: 1) Have some patrons with checkouts, some renewable and some not renewable 2) Use your favorite API tester and access GET http://koha.url/api/v1/checkouts/{checkout_id}/allows_renewal 3) Check the response is OK according to voted RFC: https://wiki.koha-community.org/wiki/Checkouts_endpoint_RFC#Checkout_renewability_2 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Just rebased
Created attachment 88589 [details] [review] Bug 17003: Add API route to get checkout's renewability
Created attachment 88590 [details] [review] Bug 17003: Adapt to OpenAPI Test plan: prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com>
Created attachment 88591 [details] [review] Bug 17003: (follow-up) Update definitions according to voted RFC Test plan: 1) Have some patrons with checkouts, some renewable and some not renewable 2) Use your favorite API tester and access GET http://koha.url/api/v1/checkouts/{checkout_id}/allows_renewal 3) Check the response is OK according to voted RFC: https://wiki.koha-community.org/wiki/Checkouts_endpoint_RFC#Checkout_renewability_2 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com>
Created attachment 91102 [details] [review] Bug 17003: Add API route to get checkout's renewability Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91103 [details] [review] Bug 17003: Adapt to OpenAPI Test plan: prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91104 [details] [review] Bug 17003: (follow-up) Update definitions according to voted RFC Test plan: 1) Have some patrons with checkouts, some renewable and some not renewable 2) Use your favorite API tester and access GET http://koha.url/api/v1/checkouts/{checkout_id}/allows_renewal 3) Check the response is OK according to voted RFC: https://wiki.koha-community.org/wiki/Checkouts_endpoint_RFC#Checkout_renewability_2 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91105 [details] [review] Bug 17003: (QA follow-up) Fix the number of tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work! Pushed to master for 19.11.00
Fridolin, are you going to backport this to 19.05?
Pushed to 19.05.x for 19.05.03 API is good ;)
not backporting this enhancement to 18.11.x
Just working through bugs for the release notes.. Who sponsored this one?