Created attachment 87276 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t I have difficulties to apply this. It fails to trailing whitespace error. Applying: Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API .git/rebase-apply/patch:18: trailing whitespace. error: sha1 information is lacking or useless (api/v1/swagger/paths/checkouts.json). error: could not build fake ancestor Patch failed at 0001 Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Created attachment 88148 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t (In reply to Johanna Räisä from comment #2) > I have difficulties to apply this. It fails to trailing whitespace error. > > Applying: Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API > .git/rebase-apply/patch:18: trailing whitespace. > > error: sha1 information is lacking or useless > (api/v1/swagger/paths/checkouts.json). > error: could not build fake ancestor > Patch failed at 0001 Bug 22613: Add /patrons/{patron_id}/checkouts endpoint > to REST API Have you applied dependencies? I just rebased it anyway Thanks for testing Thanks, the whitespace error disappeared. :) Even with all dependencies I can't apply this. Seems that there are more rows in checkouts.json and it fails to that. Probably some follow-ups have added them? Applying: Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API error: sha1 information is lacking or useless (api/v1/swagger/paths/checkouts.json). error: could not build fake ancestor Patch failed at 0001 Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API I see the same problem using git bz apply 22613 and following all dependencies. (In reply to Katrin Fischer from comment #7) > I see the same problem using git bz apply 22613 and following all > dependencies. I was able to apply them all yesterday, but master is changing quickly now, I'll do rebase of them all ;) Created attachment 88266 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t But yes, you both are right, there was some mess in my branch, should be OK now. Created attachment 88332 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Have to ask what version of Mojolicious and OpenAPI you are using?
I'm getting errors with browser from api/v1/checkouts?patron_id={patron_id}
message "Malformed query string"
path "/query/patron_id"
Also with running test the same 400 errors come up with patrons-endpoints. With cURL and d-parameter the request is ok.
Otherwise the patch seemed to work fine, so I think I have some different module version somewhere.
(In reply to Johanna Räisä from comment #12) > Have to ask what version of Mojolicious and OpenAPI you are using? > > I'm getting errors with browser from api/v1/checkouts?patron_id={patron_id} > > message "Malformed query string" > path "/query/patron_id" > > Also with running test the same 400 errors come up with patrons-endpoints. > With cURL and d-parameter the request is ok. > > Otherwise the patch seemed to work fine, so I think I have some different > module version somewhere. I am using Mojolicious 7.21 and openapi plugin 1.15 Created attachment 88448 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Created attachment 88450 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Hi Josef, When applying the patch, I have sha1. Bug 22613 - Add /patrons/patron_id/checkouts endpoints 88450 - Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API fatal: sha1 information is lacking or useless (api/v1/swagger/paths.json). error: could not build fake ancestor Patch failed at 0001 Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-22613-Add-patronspatronidcheckouts-endpoint-to-SKP48A.patch Thanks. Hi Johanna, can you verese this patch on top of master? Thank you. (In reply to Michal Denar from comment #17) > Hi Johanna, > can you verese this patch on top of master? > > Thank you. I think Josef will do better with this one. ;) I just tested it. Created attachment 91406 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Rebased on master. Thanks for testing Created attachment 91457 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94517 [details] [review] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Subject: [PATCH] Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API Test plan: 1) Apply the patch 2) Restart plack 3) Access GET to /patrons/{patron_id}/checkouts It should have the same abilities as /checkouts?patron_id={patron_id} 4) prove t/db_dependent/api/v1/checkouts.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com> The description "By default, only current checkout are returned, when this is true, all checkouts (current and checked in) are returned as result." is not true, only checked in checkouts are returned. The description should be the same as for /checkouts endpoint: "By default, current checkouts are returned, when this is true then checked in checkouts are returned as result." (In reply to Joonas Kylmälä from comment #23) > The description > > "By default, only current checkout are returned, when this is true, all > checkouts (current and checked in) are returned as result." > > is not true, only checked in checkouts are returned. The description should > be the same as for /checkouts endpoint: > > "By default, current checkouts are returned, when this is true then checked > in checkouts are returned as result." We should really merge the f* tables. Also, would we be able to do this without having to duplicate the endpoint definition by using the existing definition for /checkouts endpoint by replacing > "$ref": "paths/checkouts.json#/~1patrons~1{patron_id}~1checkouts" with > $ref": "paths/checkouts.json#/~1checkouts~1{patron_id} (In reply to Joonas Kylmälä from comment #25) > Also, would we be able to do this without having to duplicate the endpoint > definition by using the existing definition for /checkouts endpoint by > replacing > > > "$ref": "paths/checkouts.json#/~1patrons~1{patron_id}~1checkouts" > > with > > > $ref": "paths/checkouts.json#/~1checkouts~1{patron_id} As-is, the patch looks correct and what Martin and I have been saying. I haven't tried or run the tests to verify it is functional, but it looks exactly as what we would've expected when saying 'alias'. *** Bug 26571 has been marked as a duplicate of this bug. *** Created attachment 164611 [details] [review] Bug 22613: [REST API] Add GET endpoint /patrons/{patron_id}/checkouts Test plan: curl -v -s -u koha:koha --request GET http://kohadev.local/api/v1/patrons/{id}/checkouts test with query parameters (they are the same as for /patrons/{id}/holds Created attachment 164759 [details] [review] Bug 22613: Add GET endpoint /patrons/{patron_id}/checkouts This patch adds a new endpoint, for fetching checkouts from a specific patron. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/patrons_checkouts.t => SUCCESS: Tests pass! 3. Run: $ curl -v -s -u koha:koha --request GET \ http://kohadev.local/api/v1/patrons/{id}/checkouts test with query parameters (they are the same as for /patrons/{id}/holds => SUCCESS: The API works! (In reply to Tomás Cohen Arazi from comment #26) > (In reply to Joonas Kylmälä from comment #25) > > Also, would we be able to do this without having to duplicate the endpoint > > definition by using the existing definition for /checkouts endpoint by > > replacing > > > > > "$ref": "paths/checkouts.json#/~1patrons~1{patron_id}~1checkouts" > > > > with > > > > > $ref": "paths/checkouts.json#/~1checkouts~1{patron_id} > > As-is, the patch looks correct and what Martin and I have been saying. I > haven't tried or run the tests to verify it is functional, but it looks > exactly as what we would've expected when saying 'alias'. I don't remember what 2019-Tomas meant by this XD Created attachment 164770 [details] [review] Bug 22613: Add GET endpoint /patrons/{patron_id}/checkouts This patch adds a new endpoint, for fetching checkouts from a specific patron. Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/patrons_checkouts.t => SUCCESS: Tests pass! 3. Run: $ curl -v -s -u koha:koha --request GET \ http://kohadev.local/api/v1/patrons/{id}/checkouts test with query parameters (they are the same as for /patrons/{id}/holds => SUCCESS: The API works! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All working as expected.. I could have swarn we had examples of just passing the query to an existing controller.. i.e. not duplicating the code as we do here. But I don't see a reason to block this.. we can tidy up later. Passing QA (In reply to Tomás Cohen Arazi from comment #30) > > I don't remember what 2019-Tomas meant by this XD Ah, that thing Martin wrote (In reply to Martin Renvoize from comment #32) > All working as expected.. I could have swarn we had examples of just passing > the query to an existing controller.. i.e. not duplicating the code as we do > here. Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.06 Enhancement will not be backported to 23.05.x |
As described in RFC, there should be endpoint in the form /patrons/{patron_id}/checkouts for getting patrons checkouts.