Description
Nick Clemens (kidclamp)
2023-06-08 13:11:31 UTC
Created attachment 152199 [details] [review] Bug 33960: Add deleted option ot biblios endpoint This patch adds a 'deleted' parameter to the biblios endpoint. If this parameter is passed the API will return deleted biblios rather than current ones To test: 1 - Apply patch 2 - GET http://localhost:8081/api/v1/biblios 3 - Confirm you can search and page 4 - add 'deleted' param to query 5 - Confirm you get dleted biblios information 6 - Confirm searching and paging work as expected Created attachment 152200 [details] [review] Bug 33960: Add objects and update schema Created attachment 152201 [details] [review] Bug 33960: Unit tests Created attachment 152215 [details] [review] Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 155331 [details] [review] Bug 33960: Add deleted option ot biblios endpoint This patch adds a 'deleted' parameter to the biblios endpoint. If this parameter is passed the API will return deleted biblios rather than current ones To test: 1 - Apply patch 2 - GET http://localhost:8081/api/v1/biblios 3 - Confirm you can search and page 4 - add 'deleted' param to query 5 - Confirm you get dleted biblios information 6 - Confirm searching and paging work as expected Created attachment 155332 [details] [review] Bug 33960: Add objects and update schema Created attachment 155333 [details] [review] Bug 33960: Unit tests Created attachment 155334 [details] [review] Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 155335 [details] [review] Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio Should this be signed off? I see lines from Tomas. He was just providing some feedback, still needs a signoff I suggest to tidy for QA ;) WARN Koha/Old/Biblio.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 13) WARN Koha/Old/Biblio/Metadata.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 3) FAIL Koha/Old/Biblio/Metadatas.pm FAIL pod coverage POD is missing for 'object_class' OK Koha/Old/Biblios.pm WARN Koha/REST/V1/Biblios.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 138, now: 139) WARN Koha/Schema/Result/Deletedbiblio.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 41, now: 49) OK api/v1/swagger/paths/biblios.yaml WARN t/db_dependent/api/v1/biblios.t WARN tidiness The file is less tidy than before (bad/messy lines before: 370, now: 387) This table seems a little overkill: +Returns an object representing the metadata record. The expected record type +corresponds to this table: + + ------------------------------- + | format | object type | + ------------------------------- + | marcxml | MARC::Record | + ------------------------------- Created attachment 155416 [details] [review] Bug 33960: Add deleted option ot biblios endpoint This patch adds a 'deleted' parameter to the biblios endpoint. If this parameter is passed the API will return deleted biblios rather than current ones To test: 1 - Apply patch 2 - GET http://localhost:8081/api/v1/biblios 3 - Confirm you can search and page 4 - add 'deleted' param to query 5 - Confirm you get dleted biblios information 6 - Confirm searching and paging work as expected Created attachment 155417 [details] [review] Bug 33960: Add objects and update schema Created attachment 155418 [details] [review] Bug 33960: Unit tests Created attachment 155419 [details] [review] Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 155420 [details] [review] Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> I am not sure if anything more is needed to make the API docs look pretty, I was not entirely sure how to test the local API documentation. Created attachment 157899 [details] [review] Bug 33960: Add deleted option ot biblios endpoint This patch adds a 'deleted' parameter to the biblios endpoint. If this parameter is passed the API will return deleted biblios rather than current ones To test: 1 - Apply patch 2 - GET http://localhost:8081/api/v1/biblios 3 - Confirm you can search and page 4 - add 'deleted' param to query 5 - Confirm you get dleted biblios information 6 - Confirm searching and paging work as expected Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157900 [details] [review] Bug 33960: Add objects and update schema Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157901 [details] [review] Bug 33960: Unit tests Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157902 [details] [review] Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157903 [details] [review] Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157904 [details] [review] Bug 33960: (QA follow-up) Enhance tests I'm not 100% there on this yet.. I enhanced the test to actually check that the 'deleted' option filters on only those deleted and doesn't add both resultsets. I note there's also no tests for the more involved queries and without passing through the api_query_fixer I would imagine some queries would result in a 500 right now. The question is.. should passing the deleted flag prevent such searches anyway.. and if so should we somehow note that in the specifications? I not also, we don't validate the output.. so the addition of a deleted_on field isn't flagged either way in our specification.. I'm not sure I like the inconsistency here... but I'm not really sure on a resolution either. Open to thoughts. Marking blocked whilst I ask for feedback Created attachment 158071 [details] [review] Bug 33960: (follow-up) Add to description to highlight that deleted limits query options Looking here Got struck here by another bug: [2023/11/03 10:33:00] [ERROR] GET /api/v1/biblios: unhandled exception (Mojo::Exception)<<Can't call method "to_api" on an undefined value at /usr/share/koha/Koha/Biblio.pm line 1433.>> => my $json_biblioitem = $self->biblioitem->to_api( $args ); Just testing /api/v1/biblios with Accept=application/json. Note that Accept=application/marc works fine. Cant finish here right now. Back to SO. Any one else feel free to take over. I know I suggested Nick to stick to the 'deleted' query parameter... But right now I feel like this should be a separate endpoint, with deleted_biblio_id as the identifier. My reasoning is it would be ugly that you can GET /biblios but then cannot do things like POST /biblios/:biblio_id/items on the retrieved biblio_ids, for example. Or worse, need to code something to deal with such scenarios polluting the controllers. Sorry :-/ Created attachment 159998 [details] [review] Bug 33960: Add objects and update schema Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 159999 [details] [review] Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 160000 [details] [review] Bug 33960: Add routes and controller Created attachment 160001 [details] [review] Bug 33960: Unit tests Created attachment 160002 [details] [review] Bug 33960: Tidy I'm getting a couple warnings and a fail from the qa tool. Can you check? (In reply to Owen Leonard from comment #37) > I'm getting a couple warnings and a fail from the qa tool. Can you check? You can ignore the fail, the warnings should be cleared by tidy patch Created attachment 160552 [details] [review] Bug 33960: Tidy I tried to test this, but I can't figure out how to format the query for the biblios endpoint. I can only make queries with the path parameters. Is there an example of what to put in the body of the request? I tried like this but get 500 error: curl --location --request GET 'http://localhost:8081/api/v1/biblios' \ --header 'Content-Type: application/json' \ --header 'Authorization: Bearer ...' \ --data '{ "_page": 1, "_per_page": 10, "_match": "contains", "_order_by": [], "q": ["array", "of", "strings"] }' Created attachment 163852 [details] [review] Bug 33960: Add objects and update schema Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 163853 [details] [review] Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 163854 [details] [review] Bug 33960: Add routes and controller Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 163855 [details] [review] Bug 33960: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 163948 [details] [review] Bug 33960: Add objects and update schema Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 163949 [details] [review] Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 163950 [details] [review] Bug 33960: Add routes and controller Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 163951 [details] [review] Bug 33960: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> My apologies, this one slipped the net for me. Looks great now with the work that's happened since I last assessed it. I'm happy to PQA on it, tests all passing, QA script is happy and it all makes clear sense to me. Nick, was this one sponsored.. there's no sponsor lines ;) (In reply to Martin Renvoize from comment #50) > Nick, was this one sponsored.. there's no sponsor lines ;) This one does not need credit beyond ByWater, and all the community folks that helped ;-) Pushed for 24.05! Well done everyone, thank you! We have some test fails to be fixed, that were probably caused by the work here: t_db_dependent_TestBuilder_t not ok 697 - Module Koha::Old::Biblio::Metadatas should have koha_object[s]_class method if needed not ok 698 - Module Koha::DeletedbiblioMetadata should be defined not ok 699 - Module Koha::DeletedbiblioMetadatas should be defined I've added the requisite pieces to the schema class now.. will push shortly.. just removing the additiona_work_needed tag to prevent duplicate work. Not backported to 23.11.x API route change, nothing to add/edit in the Koha manual. I assume API documentation is done automatically? |