Bug 29290

Summary: Add routes to fetch checkouts for a given biblio
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, joonas.kylmala, kyle, martin.renvoize, nick, victor
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
21.11.00
Circulation function:
Bug Depends on: 29288    
Bug Blocks: 29275, 29380, 32801    
Attachments: Bug 29290: Add spec changes
Bug 29290: Add GET /biblios/:biblio_id/checkouts
Bug 29290: Rename relationships borrower => patron
Bug 29290: Add GET /biblios/:biblio_id/checkouts
Bug 29290: Add spec changes
Bug 29290: Rename relationships borrower => patron
Bug 29290: Add GET /biblios/:biblio_id/checkouts
Bug 29290: Add spec changes
Bug 29290: Rename relationships borrower => patron
Bug 29290: Add GET /biblios/:biblio_id/checkouts
Bug 29290: Unit tests
Bug 29290: Unit tests

Description Tomás Cohen Arazi (tcohen) 2021-10-20 21:05:35 UTC
I propose we add

GET /biblios/:biblio_id/checkouts

it will have the same behavior as /checkouts, but filtering by the specified biblio.
Comment 1 Tomás Cohen Arazi (tcohen) 2021-10-20 21:13:38 UTC
Created attachment 126617 [details] [review]
Bug 29290: Add spec changes

This patch adds the new route. It also tweaks the checkout object
definition to allow embedding the required related objects (for bug 29275).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2021-10-20 21:13:44 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi (tcohen) 2021-10-20 21:29:03 UTC
Created attachment 126622 [details] [review]
Bug 29290: Rename relationships borrower => patron

This is a trivial change that is required to be able to embed patron
objects in the (old) checkout object.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi (tcohen) 2021-10-20 21:33:19 UTC
Created attachment 126625 [details] [review]
Bug 29290: Add GET /biblios/:biblio_id/checkouts

This patch adds the required controller method.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Victor Grousset/tuxayo 2021-10-21 02:50:32 UTC
Created attachment 126637 [details] [review]
Bug 29290: Add spec changes

This patch adds the new route. It also tweaks the checkout object
definition to allow embedding the required related objects (for bug 29275).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2021-10-21 02:50:36 UTC
Created attachment 126638 [details] [review]
Bug 29290: Rename relationships borrower => patron

This is a trivial change that is required to be able to embed patron
objects in the (old) checkout object.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2021-10-21 02:50:40 UTC
Created attachment 126639 [details] [review]
Bug 29290: Add GET /biblios/:biblio_id/checkouts

This patch adds the required controller method.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2021-10-21 02:52:21 UTC
I tested bug 29275 and checked the network requests to confirm it uses the new route.
Comment 9 Martin Renvoize (ashimema) 2021-10-27 08:31:35 UTC
The only thing I wonder about is the 'checked_in' parameter.  I'm wondering if there's a use case for 'all' checkouts.. i.e both historic and current?

Perhaps a 'state' param instead with 'out, in, both' ?

Just a thought, let me know what you think?
Comment 10 Martin Renvoize (ashimema) 2021-10-27 08:37:52 UTC
Created attachment 126957 [details] [review]
Bug 29290: Add spec changes

This patch adds the new route. It also tweaks the checkout object
definition to allow embedding the required related objects (for bug 29275).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2021-10-27 08:37:56 UTC
Created attachment 126958 [details] [review]
Bug 29290: Rename relationships borrower => patron

This is a trivial change that is required to be able to embed patron
objects in the (old) checkout object.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2021-10-27 08:38:00 UTC
Created attachment 126959 [details] [review]
Bug 29290: Add GET /biblios/:biblio_id/checkouts

This patch adds the required controller method.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize (ashimema) 2021-10-27 08:38:52 UTC
Of course.. the normal checkouts route already uses a 'checked_in' parameter so this is consistent.

All works as expected then, QA scripts happy. Passing QA
Comment 14 Martin Renvoize (ashimema) 2021-10-27 08:49:15 UTC
Hold off.. what's our current requirements regards API route testing?  I'm not seeing any tasts here.
Comment 15 Tomás Cohen Arazi (tcohen) 2021-10-27 12:08:08 UTC
Created attachment 126980 [details] [review]
Bug 29290: Unit tests

This patch adds unit tests for the new route. All behaviors are tested:

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi (tcohen) 2021-10-27 12:08:57 UTC
(In reply to Martin Renvoize from comment #14)
> Hold off.. what's our current requirements regards API route testing?  I'm
> not seeing any tasts here.

Good catch, I forgot!

Done!
Comment 17 Martin Renvoize (ashimema) 2021-10-27 12:13:54 UTC
Created attachment 126982 [details] [review]
Bug 29290: Unit tests

This patch adds unit tests for the new route. All behaviors are tested:

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize (ashimema) 2021-10-27 12:14:15 UTC
Nice one.. tests added and passing.. PQA proper.
Comment 19 Jonathan Druart 2021-10-28 15:45:25 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 20 Joonas Kylmälä 2021-10-30 19:17:32 UTC
This broke a few scripts, please review bug 29380 which contains fixes for those.
Comment 21 Jonathan Druart 2021-11-02 12:12:48 UTC
(In reply to Joonas Kylmälä from comment #20)
> This broke a few scripts, please review bug 29380 which contains fixes for
> those.

I completely overlooked that patch.

It was definitely a candidate for its own bug, it needs to be advertised in the release notes as well (may break plugins, notice templates, custom scripts, etc.). Also it would have been nice to rename other ->borrower methods then.
Comment 22 Tomás Cohen Arazi (tcohen) 2021-11-02 12:43:10 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Joonas Kylmälä from comment #20)
> > This broke a few scripts, please review bug 29380 which contains fixes for
> > those.
> 
> I completely overlooked that patch.
> 
> It was definitely a candidate for its own bug, it needs to be advertised in
> the release notes as well (may break plugins, notice templates, custom
> scripts, etc.). Also it would have been nice to rename other ->borrower
> methods then.

I usually add new relationships, I have overlooked it myself.