|
Description
Agustín Moyano
2023-02-27 19:28:28 UTC
Created attachment 147469 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Created attachment 147662 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 147663 [details] [review] Bug 33080: (follow-up) Minor cleanups Whilst reviewing I spotted a few calls to the stash that were no longer required and errors in the POD for the new/updated methods This patch adds fixes for those minor issues. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Comment on attachment 147662 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api Review of attachment 147662 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/Plugin/Objects.pm @@ +243,3 @@ > > # Perform search > my $objects = $result_set->search( $filtered_params, $attributes ); I think we need to rework the pagination handling a little for this change. We can't really keep it inside this method now that it returns a resultset that we're expecting developers to then further filter and refine. I think we should update the add_pagination_headers helper to accept a resultset.. it can then build the total and base_total fields itself by executing the count and looking at the pager in the resultset passed. We could then call this automatically in the non-rs search header above and document that it should be called by the developer in their controller after they've applied their own additional filtering etc in the controller if they're using the _rs helper. For this.. I'm failing QA.. But I do really like where this is headed. I've reviewed the follow-up on the branch Tomas sent me to.. more than happy to signoff on the set now :) Great work (In reply to Martin Renvoize from comment #4) > Comment on attachment 147662 [details] [review] [review] > Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api > > Review of attachment 147662 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/REST/Plugin/Objects.pm > @@ +243,3 @@ > > > > # Perform search > > my $objects = $result_set->search( $filtered_params, $attributes ); > > I think we need to rework the pagination handling a little for this change. > We can't really keep it inside this method now that it returns a resultset > that we're expecting developers to then further filter and refine. > > I think we should update the add_pagination_headers helper to accept a > resultset.. it can then build the total and base_total fields itself by > executing the count and looking at the pager in the resultset passed. > > We could then call this automatically in the non-rs search header above and > document that it should be called by the developer in their controller after > they've applied their own additional filtering etc in the controller if > they're using the _rs helper. > > For this.. I'm failing QA.. But I do really like where this is headed. Hi Martin, thanks for pointing this out... I do not believe we should change the add_pagination_headers helper, it really wouldn't change much of how we should call it. And it's actually called by some controllers, so we should change those also. Now we are calling it with base_total, total, and params... but if we change it to resultsets we should call it with filtered_resultset (to get total), unfiltered_resultset (to get base_total), and params. What I'll do is remove the add_pagination_headers call from objects->search_rs as you mentioned, and put that in objects->search instead. Cheers Created attachment 147720 [details] [review] Bug 33080: (follow-up) Move pagination logic out of search_rs Global symbol "$args" requires explicit package name (did you forget to declare "my $args"?) at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 128. Sorry, made it in a hurry... And then Tomas told me he was actually working on this with a different approach. So I'll just leave it to him to delete my patch. (In reply to Agustín Moyano from comment #9) > Sorry, made it in a hurry... And then Tomas told me he was actually working > on this with a different approach. So I'll just leave it to him to delete my > patch. Yeah, I've worked on this on the referenced repo/branch. See URL. But right now I'm struggling with the ERM endpoints, which do funky use of the pagination headers and I'm not finding unit tests to make sure I don't introduce regressions. Hm... maybe you should leave add_pagination_headers as is, and create a new helper called something like 'calc_pagination_numbers' that creates total numbers and calls add_pagination_headers somehow (In reply to Agustín Moyano from comment #11) > Hm... maybe you should leave add_pagination_headers as is, and create a new > helper called something like 'calc_pagination_numbers' that creates total > numbers and calls add_pagination_headers somehow I'm pondering an intermediate solution in which parameters can be passed as now, but there's a fallback to using the stashed values. To put it clear, the use case that is requiring this is: we need the base total available in objects.search to be passed to add_pagination_headers later. Created attachment 147734 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 147735 [details] [review] Bug 33080: (follow-up) Minor cleanups Whilst reviewing I spotted a few calls to the stash that were no longer required and errors in the POD for the new/updated methods This patch adds fixes for those minor issues. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 147736 [details] [review] Bug 33080: Add small warpper helper to call add_pagination_headers This patch introduces a small helper that uses the stashed values in `search_rs` to properly build the pagination headers based on the resultset. To test: 1. Run all the API tests and make sure nothing breaks 2. Run: $ ktd --shell $ t/db_dependent/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! Created attachment 147788 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 147789 [details] [review] Bug 33080: (follow-up) Minor cleanups Whilst reviewing I spotted a few calls to the stash that were no longer required and errors in the POD for the new/updated methods This patch adds fixes for those minor issues. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 147790 [details] [review] Bug 33080: Add small warpper helper to call add_pagination_headers This patch introduces a small helper that uses the stashed values in `search_rs` to properly build the pagination headers based on the resultset. To test: 1. Run all the API tests and make sure nothing breaks 2. Run: $ ktd --shell $ t/db_dependent/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Hmm.. well I liked where the branch was going.. but I wasn't aware of the additional calls we're already making to add_pagination_headers as yet so wasn't aware it was a bit of a faff. However, now I've just reviewed this final patch it feels like a few bits have been missed from the commit. The POD calls a helper `objects.total_from_resultset` but then defines it as `objects.generate_pagination_headers` and inside that new helper we call the non-existing `total_from_resultset`.. that feels like it should just be $result_set->count anyway.. but I'm also not entirely clear on the naming of `add_pagination_headers` and `generate_pagination_headers`.. that sort of feel like synonyms.. I could see it getting confusing down the line. Would you like me to step in and lend a hand here Tomas? Created attachment 147823 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 147824 [details] [review] Bug 33080: Allow pagination to be built from stashed values The way the old `objects.search` was build implied several totals were calculated there, and passed to the `$c->add_pagination_headers` helper. With the introduction of `objects.search_rs` and the ability of doing things to the resultset afterwards, it felt like out of place to have the pagination headers be implicitly set inside `objects.search_rs`. This patch makes the search_rs stash some required values (from the original request) and makes `add_pagination_headers` accept those values. This way other callers can still build their own pagination values, while allowing this simplified implementation. Full-stack tests still pass, and the helper tests are moved to the `db_dependent` section and are now more meaningful as well To test: 1. Apply this patch 2. Run: $ ktd --shell $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \ t/db_dependent/api/v1/ => SUCCESS: Tests pass! Nothing broken! 3. Sign off :-D Created attachment 147825 [details] [review] Bug 33080: Adapt other use-cases This patch adapts the other controllers that rely on add_pagination_headers outside of `objects.search`. To test: 1. Apply this patchset 2. Run: $ ktd --shell $ prove t/db_dependent/api/v1/* \ t/db_dependent/Koha/REST/Plugin/Objects.t \ t/db_dependent/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! 3. Sign off :-D Created attachment 148020 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 148021 [details] [review] Bug 33080: Allow pagination to be built from stashed values The way the old `objects.search` was build implied several totals were calculated there, and passed to the `$c->add_pagination_headers` helper. With the introduction of `objects.search_rs` and the ability of doing things to the resultset afterwards, it felt like out of place to have the pagination headers be implicitly set inside `objects.search_rs`. This patch makes the search_rs stash some required values (from the original request) and makes `add_pagination_headers` accept those values. This way other callers can still build their own pagination values, while allowing this simplified implementation. Full-stack tests still pass, and the helper tests are moved to the `db_dependent` section and are now more meaningful as well To test: 1. Apply this patch 2. Run: $ ktd --shell $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \ t/db_dependent/api/v1/ => SUCCESS: Tests pass! Nothing broken! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 148022 [details] [review] Bug 33080: Adapt other use-cases This patch adapts the other controllers that rely on add_pagination_headers outside of `objects.search`. To test: 1. Apply this patchset 2. Run: $ ktd --shell $ prove t/db_dependent/api/v1/* \ t/db_dependent/Koha/REST/Plugin/Objects.t \ t/db_dependent/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All working fantastically and feels much cleaner. Signing off. Created attachment 148071 [details] [review] Bug 33080: Introduce objects.search_rs, objects.find_rs and objects.to_api This patch introduces the mentioned helpers, and reimplements objects.search and objects.find in terms of them. To test: 1. Apply patch 2. restart_all 3. Check that any API that uses objects.search helper is still running (GET /api/v1/items, GET /api/v1/holds, etc) 4. prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 148072 [details] [review] Bug 33080: Allow pagination to be built from stashed values The way the old `objects.search` was build implied several totals were calculated there, and passed to the `$c->add_pagination_headers` helper. With the introduction of `objects.search_rs` and the ability of doing things to the resultset afterwards, it felt like out of place to have the pagination headers be implicitly set inside `objects.search_rs`. This patch makes the search_rs stash some required values (from the original request) and makes `add_pagination_headers` accept those values. This way other callers can still build their own pagination values, while allowing this simplified implementation. Full-stack tests still pass, and the helper tests are moved to the `db_dependent` section and are now more meaningful as well To test: 1. Apply this patch 2. Run: $ ktd --shell $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \ t/db_dependent/api/v1/ => SUCCESS: Tests pass! Nothing broken! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 148073 [details] [review] Bug 33080: Adapt other use-cases This patch adapts the other controllers that rely on add_pagination_headers outside of `objects.search`. To test: 1. Apply this patchset 2. Run: $ ktd --shell $ prove t/db_dependent/api/v1/* \ t/db_dependent/Koha/REST/Plugin/Objects.t \ t/db_dependent/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to master for 23.05. Nice work everyone, thanks! Enhancement - not backporting to 22.11.x Nice work everyone! Required by bug 32734 which was backported already.. backporting for 22.11.x will be in 22.11.04 This seems like backend stuff only, nothing to document. Closing, but feel free to reopen with more details if I'm wrong. |