Bug 33960 - Add ability to retrieve deleted biblios
Summary: Add ability to retrieve deleted biblios
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords: rel_24_05_candidate
Depends on:
Blocks:
 
Reported: 2023-06-08 13:11 UTC by Nick Clemens
Modified: 2024-03-26 17:58 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 33960: Add deleted option ot biblios endpoint (2.27 KB, patch)
2023-06-08 13:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.12 KB, patch)
2023-06-08 13:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Unit tests (2.53 KB, patch)
2023-06-08 13:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio (2.15 KB, patch)
2023-06-08 18:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33960: Add deleted option ot biblios endpoint (2.17 KB, patch)
2023-09-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.12 KB, patch)
2023-09-07 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Unit tests (2.29 KB, patch)
2023-09-07 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio (1.84 KB, patch)
2023-09-07 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio (4.94 KB, patch)
2023-09-07 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add deleted option ot biblios endpoint (2.17 KB, patch)
2023-09-08 13:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.12 KB, patch)
2023-09-08 13:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33960: Unit tests (2.29 KB, patch)
2023-09-08 13:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio (1.89 KB, patch)
2023-09-08 13:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio (5.00 KB, patch)
2023-09-08 13:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33960: Add deleted option ot biblios endpoint (2.29 KB, patch)
2023-10-26 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.24 KB, patch)
2023-10-26 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: Unit tests (2.41 KB, patch)
2023-10-26 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: (QA follow-up) ->delete doesn't send to deletedbiblio (1.96 KB, patch)
2023-10-26 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio (5.54 KB, patch)
2023-10-26 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: (QA follow-up) Enhance tests (1.83 KB, patch)
2023-10-26 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: (follow-up) Add to description to highlight that deleted limits query options (973 bytes, patch)
2023-10-30 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.24 KB, patch)
2023-12-19 13:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio (4.56 KB, patch)
2023-12-19 13:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add routes and controller (10.31 KB, patch)
2023-12-19 13:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Unit tests (8.91 KB, patch)
2023-12-19 13:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Tidy (16.62 KB, patch)
2023-12-19 13:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Tidy (16.39 KB, patch)
2024-01-04 19:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.42 KB, patch)
2024-03-25 18:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio (4.62 KB, patch)
2024-03-25 18:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33960: Add routes and controller (10.24 KB, patch)
2024-03-25 18:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33960: Unit tests (8.87 KB, patch)
2024-03-25 18:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33960: Add objects and update schema (8.48 KB, patch)
2024-03-26 17:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: Add api mapping and biblioitems to Old::Biblios to match Biblio (4.68 KB, patch)
2024-03-26 17:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: Add routes and controller (10.30 KB, patch)
2024-03-26 17:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33960: Unit tests (8.93 KB, patch)
2024-03-26 17:45 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-06-08 13:11:31 UTC
To add ability for discovery services to use the REST api to keep up to date, we need a way to retrieve information on deleted biblios.
Comment 1 Nick Clemens 2023-06-08 13:58:10 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
Comment 2 Nick Clemens 2023-06-08 13:58:12 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2023-06-08 13:58:14 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2023-06-08 18:28:33 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2023-09-07 16:27:58 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2023-09-07 16:28:01 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2023-09-07 16:28:03 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2023-09-07 16:28:05 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2023-09-07 16:28:07 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2023-09-08 12:02:42 UTC
Should this be signed off? I see lines from Tomas.
Comment 11 Nick Clemens 2023-09-08 12:31:22 UTC
He was just providing some feedback, still needs a signoff
Comment 12 Katrin Fischer 2023-09-08 13:49:18 UTC
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   |
+    -------------------------------
Comment 13 Katrin Fischer 2023-09-08 13:50:44 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2023-09-08 13:50:46 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2023-09-08 13:50:49 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2023-09-08 13:50:51 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2023-09-08 13:50:53 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2023-09-08 13:51:54 UTC
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.
Comment 19 Martin Renvoize 2023-10-26 10:33:51 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2023-10-26 10:33:55 UTC Comment hidden (obsolete)
Comment 21 Martin Renvoize 2023-10-26 10:33:58 UTC Comment hidden (obsolete)
Comment 22 Martin Renvoize 2023-10-26 10:34:00 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2023-10-26 10:34:03 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2023-10-26 10:34:05 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2023-10-26 10:37:12 UTC
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.
Comment 26 Martin Renvoize 2023-10-26 10:40:06 UTC
Marking blocked whilst I ask for feedback
Comment 27 Nick Clemens 2023-10-30 14:52:59 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2023-11-03 10:15:05 UTC
Looking here
Comment 29 Marcel de Rooy 2023-11-03 10:47:44 UTC
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.
Comment 30 Marcel de Rooy 2023-11-03 10:51:57 UTC
Cant finish here right now. Back to SO. Any one else feel free to take over.
Comment 31 Tomás Cohen Arazi 2023-12-11 19:46:44 UTC
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 :-/
Comment 32 Nick Clemens 2023-12-19 13:48:15 UTC
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>
Comment 33 Nick Clemens 2023-12-19 13:48:18 UTC
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>
Comment 34 Nick Clemens 2023-12-19 13:48:20 UTC
Created attachment 160000 [details] [review]
Bug 33960: Add routes and controller
Comment 35 Nick Clemens 2023-12-19 13:48:23 UTC
Created attachment 160001 [details] [review]
Bug 33960: Unit tests
Comment 36 Nick Clemens 2023-12-19 13:48:25 UTC
Created attachment 160002 [details] [review]
Bug 33960: Tidy
Comment 37 Owen Leonard 2024-01-04 16:15:43 UTC
I'm getting a couple warnings and a fail from the qa tool. Can you check?
Comment 38 Nick Clemens 2024-01-04 17:54:16 UTC
(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
Comment 39 Nick Clemens 2024-01-04 19:00:11 UTC
Created attachment 160552 [details] [review]
Bug 33960: Tidy
Comment 40 Brendan Lawlor 2024-01-25 20:46:16 UTC
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"]
}'
Comment 41 Tomás Cohen Arazi 2024-03-25 18:18:00 UTC
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>
Comment 42 Tomás Cohen Arazi 2024-03-25 18:18:03 UTC
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>
Comment 43 Tomás Cohen Arazi 2024-03-25 18:18:06 UTC
Created attachment 163854 [details] [review]
Bug 33960: Add routes and controller

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Tomás Cohen Arazi 2024-03-25 18:18:09 UTC
Created attachment 163855 [details] [review]
Bug 33960: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Martin Renvoize 2024-03-26 17:45:48 UTC
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>
Comment 46 Martin Renvoize 2024-03-26 17:45:51 UTC
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>
Comment 47 Martin Renvoize 2024-03-26 17:45:54 UTC
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>
Comment 48 Martin Renvoize 2024-03-26 17:45:57 UTC
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>
Comment 49 Martin Renvoize 2024-03-26 17:46:43 UTC
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.
Comment 50 Martin Renvoize 2024-03-26 17:47:02 UTC
Nick, was this one sponsored.. there's no sponsor lines ;)
Comment 51 Nick Clemens 2024-03-26 17:58:46 UTC
(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 ;-)