Description
Tomás Cohen Arazi (tcohen)
2021-10-20 21:05:35 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> Created attachment 126618 [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> 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> 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> 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> 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> 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> I tested bug 29275 and checked the network requests to confirm it uses the new route. 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? 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> 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> 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> 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 Hold off.. what's our current requirements regards API route testing? I'm not seeing any tasts here. 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> (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! 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> Nice one.. tests added and passing.. PQA proper. Pushed to master for 21.11, thanks to everybody involved! This broke a few scripts, please review bug 29380 which contains fixes for those. (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. (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. |