When including items in OAI we add the biblionumbers of deleted items into the deleted list This is from this line: 93 if ( $include_items ) { 94 $sql = "($sql) UNION (SELECT DISTINCT(biblionumber) FROM deleteditems $criteria)"; 95 push @bind_params, @part_bind_params; This is wrong because deletion of a single item does not indicate a record is deleted To recreate: 1 - Create /var/lib/koha/kohadev/OAI.yaml with: format: marcxml: metadataPrefix: marcxml metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd include_items: 1 2 - Set OAI-PMH:ConfFile to '/var/lib/koha/kohadev/OAI.yaml' 3 - Delete an item from a record 4 - View the page: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml 5 - Note the record is now listed as deleted
Good catch. I think there's some additional relevant lines lower down in Koha/OAI/Server/ListBase.pm as well when the max timestamp is fetched. $sql = " SELECT MAX(timestamp) FROM ( SELECT timestamp FROM deletedbiblio_metadata WHERE biblionumber = ? UNION SELECT timestamp FROM deleteditems WHERE biblionumber = ? ) bis "; I mean it should be impossible to deleted an item after the bib has been deleted. But this SQL would return a timestamp where there is a deleted item but not a deleted bib.
(In reply to David Cook from comment #1) > Good catch. > > I think there's some additional relevant lines lower down in > Koha/OAI/Server/ListBase.pm as well when the max timestamp is fetched. > Koha/OAI/Server/GetRecord.pm has that same weird logic, although it would just be a performance degradation rather than a wrong classification...
Oh no. It seems that I made a terrible mistake here. However, it's not that simple. The timestamp of a deleted item is still relevant for a live biblio. The latest timestamp of any change, be that change or deletion, must be used as the timestamp since everything affects what's in the returned record. So it's not as simple as removing the statements for non-deleted records. I'll take this with a high priority.
And yes, you're both right about the deleted records. I had missed the most logical shortcut that a deleted biblio can't have items deleted later. I think I have a fix which mostly just makes things more simple, but I'm still working on adding tests to cover all cases.
Created attachment 125570 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider Test plan: 1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0 2. Run prove -v t/db_dependent/OAI/
This should fix the issues. I haven't had a chance to manually test much yet, but at least the tests added failed miserably without the changes.
Thanks a lot, Ere, for your reactivity!
Created attachment 125576 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider Test plan: 1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0 2. Run prove -v t/db_dependent/OAI/
Jonathan, no problem, it was my bad anyway.. I've added a couple more tests. I'll run a harvesting job to verify.
Created attachment 125591 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider Test plan: 1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0 2. Run prove -v t/db_dependent/OAI/
Test harvesting completed successfully and record counts match.
For me the tests fail: not ok 33 - Tests for timestamp handling # Failed test 'Tests for timestamp handling' # at t/db_dependent/OAI/Server.t line 849. # Looks like you failed 1 test of 33. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/33 subtests Testing notes (koha-testing-docker started with ku): - Enable OAI-PMH - Before the patch is replied there is an incomplete item record listed at the end of the metadata section for the item deleted, after the patch nothing is shown for the deleted item, for example: <datafield ind2="" tag="952" ind1=""> <subfield code="0">0</subfield> <subfield code="1">0</subfield> <subfield code="4">0</subfield> <subfield code="6">_</subfield> <subfield code="7">0</subfield> <subfield code="8">REF</subfield> <subfield code="9">313</subfield> <subfield code="a">FPL</subfield> <subfield code="b">FPL</subfield> <subfield code="c">GEN</subfield> <subfield code="d">2014-09-04</subfield> <subfield code="p">39999000006117</subfield> <subfield code="r">2014-09-04</subfield> <subfield code="w">2014-09-04</subfield> <subfield code="y">BK</subfield>
Created attachment 125688 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider Test plan: 1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0 2. Run prove -v t/db_dependent/OAI/
David, Oops, silly me. I somehow thought it was clever that I made the timestamps start from an hour ago. This was fine until you created a dev box and ran the tests in an hour.. Should be fixed now.
8-)
Created attachment 125718 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider Test plan: 1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0 2. Run prove -v t/db_dependent/OAI/ Signed-off-by: David Nind <david@davidnind.com>
Created attachment 125955 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider Test plan: 1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0 2. Run prove -v t/db_dependent/OAI/ Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Can we get a proper commit message please?
Created attachment 126029 [details] [review] Bug 29135: Fix handling of deleted items in OAI-PMH provider When the OAI-PMH provider was configured to include item information in the returned records, deleting an item would cause the record to be reported as deleted. The provider also did some useless checks when determining the timestamp of a deleted record. These checks only had a performance impact. To test: 1) Create /var/lib/koha/kohadev/OAI.yaml with: format: marcxml: metadataPrefix: marcxml metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd include_items: 1 2) Set preference OAI-PMH to Enable 3) Set preference OAI-PMH:ConfFile to /var/lib/koha/kohadev/OAI.yaml 4) Delete an item from a record 5) View the page: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml 6) Note the record is now listed as deleted 7) Run prove -v t/db_dependent/OAI/ Sponsored-by: The National Library of Finland Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Commit message revised.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Depends on Bug 27584 not in 20.11.x