Bug 22613 - Add /patrons/patron_id/checkouts endpoints
Summary: Add /patrons/patron_id/checkouts endpoints
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Martin Renvoize
URL: https://wiki.koha-community.org/wiki/...
Keywords:
: 26571 (view as bug list)
Depends on: 17005
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-01 08:34 UTC by Josef Moravec
Modified: 2024-04-12 07:02 UTC (History)
9 users (show)

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


Attachments
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.81 KB, patch)
2019-04-01 12:30 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.81 KB, patch)
2019-04-17 09:04 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.82 KB, patch)
2019-04-18 07:37 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.86 KB, patch)
2019-04-18 19:39 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (107.61 KB, patch)
2019-04-23 11:48 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.91 KB, patch)
2019-04-23 11:52 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.92 KB, patch)
2019-07-08 11:35 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (3.97 KB, patch)
2019-07-11 07:52 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22613: Add /patrons/{patron_id}/checkouts endpoint to REST API (4.02 KB, patch)
2019-10-22 08:55 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 22613: [REST API] Add GET endpoint /patrons/{patron_id}/checkouts (8.87 KB, patch)
2024-04-10 12:07 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 22613: Add GET endpoint /patrons/{patron_id}/checkouts (9.11 KB, patch)
2024-04-11 14:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22613: Add GET endpoint /patrons/{patron_id}/checkouts (9.23 KB, patch)
2024-04-11 15:20 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2019-04-01 08:34:36 UTC
As described in RFC, there should be endpoint in the form /patrons/{patron_id}/checkouts for getting patrons checkouts.
Comment 1 Josef Moravec 2019-04-01 12:30:02 UTC
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
Comment 2 Johanna Räisä 2019-04-17 08:24:31 UTC
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
Comment 3 Josef Moravec 2019-04-17 09:04:04 UTC
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
Comment 4 Josef Moravec 2019-04-17 09:04:54 UTC
(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
Comment 5 Johanna Räisä 2019-04-17 09:32:42 UTC
Thanks, the whitespace error disappeared. :)
Comment 6 Johanna Räisä 2019-04-18 06:30:24 UTC
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
Comment 7 Katrin Fischer 2019-04-18 06:32:46 UTC
I see the same problem using git bz apply 22613 and following all dependencies.
Comment 8 Josef Moravec 2019-04-18 07:28:06 UTC
(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 ;)
Comment 9 Josef Moravec 2019-04-18 07:37:47 UTC
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
Comment 10 Josef Moravec 2019-04-18 07:38:41 UTC
But yes, you both are right, there was some mess in my branch, should be OK now.
Comment 11 Michal Denar 2019-04-18 19:39:04 UTC
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>
Comment 12 Johanna Räisä 2019-04-23 11:03:05 UTC
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.
Comment 13 Josef Moravec 2019-04-23 11:21:10 UTC
(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
Comment 14 Johanna Räisä 2019-04-23 11:48:23 UTC
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>
Comment 15 Johanna Räisä 2019-04-23 11:52:36 UTC
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>
Comment 16 Bouzid Fergani 2019-07-05 19:06:19 UTC
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.
Comment 17 Michal Denar 2019-07-05 19:53:57 UTC
Hi Johanna,
can you verese this patch on top of master?

Thank you.
Comment 18 Johanna Räisä 2019-07-08 05:40:06 UTC
(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.
Comment 19 Josef Moravec 2019-07-08 11:35:22 UTC
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>
Comment 20 Josef Moravec 2019-07-08 11:35:50 UTC
Rebased on master. Thanks for testing
Comment 21 Michal Denar 2019-07-11 07:52:52 UTC
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>
Comment 22 Emmi Takkinen 2019-10-22 08:55:43 UTC
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>
Comment 23 Joonas Kylmälä 2019-12-16 14:28:44 UTC
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."
Comment 24 Tomás Cohen Arazi 2019-12-16 14:36:17 UTC
(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.
Comment 25 Joonas Kylmälä 2019-12-16 14:50:18 UTC
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}
Comment 26 Tomás Cohen Arazi 2019-12-16 15:35:19 UTC
(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'.
Comment 27 Martin Renvoize 2023-03-06 15:23:36 UTC
*** Bug 26571 has been marked as a duplicate of this bug. ***
Comment 28 Alex Arnaud 2024-04-10 12:07:53 UTC
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
Comment 29 Tomás Cohen Arazi 2024-04-11 14:48:05 UTC
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!
Comment 30 Tomás Cohen Arazi 2024-04-11 14:50:50 UTC
(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
Comment 31 Martin Renvoize 2024-04-11 15:20:07 UTC
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>
Comment 32 Martin Renvoize 2024-04-11 15:21:43 UTC
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
Comment 33 Tomás Cohen Arazi 2024-04-12 07:02:44 UTC
(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.