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

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   |
+    -------------------------------

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.

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

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?