Do you plan to work on this? Support for deleted record is already there in master.

Hi! 

We got somebody from the national library to take a look at the OAI-PMH-implementation of Koha, and the Bugzilla entries didn't help a lot in deciphering what features have been implemented since 3.16.

I will provide the commit as soon as I receive the improved version of this patch.

> We got somebody from the national library to take a look at the
> OAI-PMH-implementation of Koha, and the Bugzilla entries didn't help a lot
> in deciphering what features have been implemented since 3.16.
> 11
> I will provide the commit as soon as I receive the improved version of this
> patch.

Please work on the top of bug 14939. Signing it would even been better. Having a modularized OAI server will help a lot in term of maintainability.

(In reply to Frédéric Demians from comment #3)
> > We got somebody from the national library to take a look at the
> > OAI-PMH-implementation of Koha, and the Bugzilla entries didn't help a lot
> > in deciphering what features have been implemented since 3.16.
> > 11
> > I will provide the commit as soon as I receive the improved version of this
> > patch.
> 
> Please work on the top of bug 14939. Signing it would even been better.
> Having a modularized OAI server will help a lot in term of maintainability.

I agree. Modularity is awesome.
Having complete test coverage is even better.

Failing QA because of missing tests

> I agree. Modularity is awesome.
> Having complete test coverage is even better.

Now, there isn't any test at all now. So modularizing is a first step. If you want tests, add them. And don't forget UT for you own patch which modify a lot of OAI server code without guaranty it won't something somewhere.

(In reply to Frédéric Demians from comment #8)
> > I agree. Modularity is awesome.
> > Having complete test coverage is even better.
> 
> Now, there isn't any test at all now. So modularizing is a first step. If
> you want tests, add them. And don't forget UT for you own patch which modify
> a lot of OAI server code without guaranty it won't something somewhere.

Just sharing the code here in hopes of it being helpful.

And you should make tests before you refactor.

(In reply to Olli-Antti Kivilahti from comment #10)
> And you should make tests before you refactor.

You're welcome.

It's modularizing not refactoring. The existing classes are split from oai.pl into individual .pm files, one per class. It doesn't change a line of code. So coding a full UT for OAI server would be awesome, I can't disagree, but it isn't a requirement for this patch.

Olli, Could you provide a rebased patch?
It seems that bug 15406 would need the second patch.

(In reply to Jonathan Druart from comment #12)
> Olli, Could you provide a rebased patch?
> It seems that bug 15406 would need the second patch.

And this part of the 1st patch:

diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
index e9d836c..c2eec84 100644
--- a/installer/data/mysql/kohastructure.sql
+++ b/installer/data/mysql/kohastructure.sql
@@ -209,6 +209,7 @@ CREATE TABLE `biblioitems` ( -- information related to bibliographic records in
   KEY `isbn` (`isbn`(255)),
   KEY `issn` (`issn`(255)),
   KEY `publishercode` (`publishercode`),
+  KEY `timestamp_bibno` (`timestamp`, `biblionumber`),
   CONSTRAINT `biblioitems_ibfk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
@@ -866,7 +867,8 @@ CREATE TABLE `deletedbiblioitems` ( -- information about bibliographic records t
   KEY `bibnoidx` (`biblionumber`),
   KEY `itemtype_idx` (`itemtype`),
   KEY `isbn` (`isbn`(255)),
-  KEY `publishercode` (`publishercode`)
+  KEY `publishercode` (`publishercode`),
+  KEY `timestamp_bibno` (`timestamp`, `biblionumber`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Hi!

We are upgrading our KohaSuomi branch (with 300 custom commits) built on top of Koha 3.16 to master this summer.
I heard that it is "impossible".

Looking forward to doing it with our soon-to-be continuous integration environment :)

When this is ready, this patch is also rebased.

Feel free to do it sooner.

It would be great if others could try this out and see if it works for them.

Hi Ere, status here is still Failed QA - so testers most likely won't see this. If you feel this is testable with current master, please switch to 'needs signoff'

As others have pointed out, we already have support for deleted records. Could you take another look and maybe update your commit message? It would also be good to have a test plan. Ideally all those changes should be splitted into separate bugs to make them easier to manage.

(In reply to Ere Maijala from comment #16)
> It would be great if others could try this out and see if it works for them.

It looks very good, Ere! How did you generate your patch? It seems you haven't do it with Git. You should...

It's definitely done with a simple git diff command. Let me know if you need it done with specific parameters or something.

Hi Ere, it's missing the information about you as the author and such - that's usually the first lines. Take a look at git bz for our usual workflow: 
https://wiki.koha-community.org/wiki/Git_bz_configuration
If you don't want to get that far yet, take a look at the user.name  and user.email settings in git and the git format-patch command (https://wiki.koha-community.org/wiki/Version_Control_Using_Git)

Thanks again for pointing that out. I hope I've now managed to attach a patch properly.

Hi Ere, looking good! One more thing: please attach a current test plan/description of the changes - best to include it in the commit message of your patch already. And one more important thing: First thing in the subject line should always be "Bug XXXXX". Thx!

Thx!

OKay, I've done my best. Sorry for the messy comments!

It seems that this patch depends on bug 14698.

Hi Frederic, that's ok - not really a dependency. The RM moves the sql file into updatedatabase on pushing the enh. This way we avoid constant rebasing and it's easier on the sanboxes.

(In reply to Katrin Fischer from comment #29)
> Hi Frederic, that's ok - not really a dependency. The RM moves the sql file
> into updatedatabase on pushing the enh. This way we avoid constant rebasing
> and it's easier on the sanboxes.

Ok. For testers (what I'm doing now), it requires tweaking the atomic .pl file in order to be able to update the DB. But I agree it's not that difficult...

> - Fixed date handling to use UTC as specs require.
> - Added support for second precision in time stamps.
> - Added support for marc21 metadata prefix as recommended in the guidelines
> (synonym for marcxml).
> - ListIdentifiers and ListRecords always return records in timestamp order
> (faster than biblionumber order with the new index).
> - Improved performance of database queries especially for large collections.

All good. I can see the SQL improvements, and immediate response time
improvement.

> - Unified functionality of ListRecords and ListIdentifiers to a common base
> class.

Good catch. It was stupid repeating convoluted code and queries in two
places.

> - If items are included in the records, their timestamps are taken into
> account everywhere so that whichever is the most recent (timestamp of
> biblioitem or any of its items) is considered the record's timestamp.

I approve this. It can be interesting to send back to the harvester items info
when items are modified in Koha. But it could also have a huge impact on the
quantity of records returned by incremental harvesting. Any circulation
transaction modify items.timestamp. So for libraries doing a lot of
circulation, harvester will have to harvest a lot of records just for the 'due
date' field or other circulation related fields. In some scenario, it may be
interesting to include items (include_items parameter) in order to have
homebranch and call number, but availability info is not required, so
resending the whole biblio+items when a transaction is done is not desirable.
I can't see any solution without adding a new 'datemodified' field to items
table, which would only by updated when non circulation-related info are
updated. 

Have you considered this point? Have you evaluated the impact of including
items with a Koha doing a lot of circulation?

Hm, I think in some cases it would be nice to include updates on available - I wouldn't rule that out. Maybe the decision to take the item change into account could be configurable?

(In reply to Frédéric Demians from comment #30)
> (In reply to Katrin Fischer from comment #29)
> > Hi Frederic, that's ok - not really a dependency. The RM moves the sql file
> > into updatedatabase on pushing the enh. This way we avoid constant rebasing
> > and it's easier on the sanboxes.
> 
> Ok. For testers (what I'm doing now), it requires tweaking the atomic .pl
> file in order to be able to update the DB. But I agree it's not that
> difficult...

Hm, if it's done right it should be enough to run updatedatabase.pl from commandline - it should run the atomic updates then automatically.

(In reply to Frédéric Demians from comment #31)
> > - If items are included in the records, their timestamps are taken into
> > account everywhere so that whichever is the most recent (timestamp of
> > biblioitem or any of its items) is considered the record's timestamp.
> 
> I approve this. It can be interesting to send back to the harvester items
> info
> when items are modified in Koha. But it could also have a huge impact on the
> quantity of records returned by incremental harvesting. Any circulation
> transaction modify items.timestamp. So for libraries doing a lot of
> circulation, harvester will have to harvest a lot of records just for the
> 'due
> date' field or other circulation related fields. In some scenario, it may be

I did consider it but I left it out for now since from our point of view any change in the data that's included in a harvested record should make the record be reharvested since you can't know what data the harvester finds important.

That said, if you think there should be an option to control whether item timestamps are taken into account, I can add that. But you're right in that if you'd like to only ignore status changes caused by circulation actions, that's not possible without changes deeper in the system.

Additionally we need to take into account floating collections that would cause item's location to change on checkin. I'm really an outsider to Koha (mainly a VuFind developer), so I'm not sure how everything works in practice, so I apologize if what I say doesn't make sense or use the correct terminology.

(In reply to Katrin Fischer from comment #32)

> Hm, I think in some cases it would be nice to include updates on available -
> I wouldn't rule that out. Maybe the decision to take the item change into
> account could be configurable?

You're correct. IMO there are three cases:

(1) I don't want to send item info to harvesters. The item info are not needed
or are retrieved later using Koha webservices. This could be achieved with
include_items=0.

(2) I want to send all item info to harvester, ie descriptive info (branch,
call number, etc.) and circulation info (due date). This could be achieved
with include_items=1. With Ere patch, those info are resent to harvester each
time an item record is modified, or if a new item is added to an existing
biblio record. It wasn't the case until now, which is a bug from my
perspective.

(3) I want to send to harvester item info, but just 'descriptive' info. I want
that because I don't need availability info for example, or because it will be
retrieved later via webservices. With the OAI-PMH protocol, the availability
can't be provided to third party tools (say VuFind) in real time: webservices
are needed. But items.homebranch could be very useful in a metacatalog
populated via harvesting. And in this case, real time is not a requirement. So
this mixed mode could make sense for some. Especially to avoid bringing a Koha
server to its knee.

(In reply to Ere Maijala from comment #34)

> That said, if you think there should be an option to control whether item
> timestamps are taken into account, I can add that. But you're right in that
> if you'd like to only ignore status changes caused by circulation actions,
> that's not possible without changes deeper in the system.

I suspect that such an option would just add confusion. For scenario (3)
described above, a new patch should be needed.

But I was just curious whether you was able to measure the number of OAI-PMH
requests to Koha on a large catalog with a lot of circulation.

> Additionally we need to take into account floating collections that would
> cause item's location to change on checkin. I'm really an outsider to Koha
> (mainly a VuFind developer), so I'm not sure how everything works in
> practice, so I apologize if what I say doesn't make sense or use the correct
> terminology.

Floating collections modify item records, and so timestamp field. So you UNION
request will work.

(In reply to Frédéric Demians from comment #35)
> > That said, if you think there should be an option to control whether item
> > timestamps are taken into account, I can add that. But you're right in that
> > if you'd like to only ignore status changes caused by circulation actions,
> > that's not possible without changes deeper in the system.
> 
> I suspect that such an option would just add confusion. For scenario (3)
> described above, a new patch should be needed.
> 
> But I was just curious whether you was able to measure the number of OAI-PMH
> requests to Koha on a large catalog with a lot of circulation.

I haven't had a chance to measure this in a live production system. It is estimated that for a large installation we may end up harvesting records in the scale of tens of thousand per day. However, we currently update the VuFind index every half an hour, so the amount of records that change per an update is not as large.

When considering scenario (3), please keep in mind that item addition and deletion are also important events. If you don't check item timestamps, you won't get e.g. the locations for any newly added items.

Hm, reading through the use cases Frederic wrote I wonder if it wouldn't work for a first step if we say that the item changes should only be taken into account if you have the add_items flag set. And then we could open a separate bug for an enhanced mode, where only some changes lead to 'republishing' the record via OAI.

include_items option ... is what I meant :)

(In reply to Katrin Fischer from comment #37)

That makes sense to me.

Created attachment 53853 [details] [review]
Remove Koha::AtomicUpdater dependency

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

Created attachment 53854 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

(In reply to Frédéric Demians from comment #8)
> > Having complete test coverage is even better.
> 
> Now, there isn't any test at all now. So modularizing is a first step. If
> you want tests, add them. And don't forget UT for you own patch which modify
> a lot of OAI server code without guaranty it won't something somewhere.

Frederic, the absence of unit tests in this patch set might be a reason for these patches getting stuck in the SO queue.

(In reply to Marcel de Rooy from comment #42)
> Frederic, the absence of unit tests in this patch set might be a reason for
> these patches getting stuck in the SO queue.

Jonathan just pointed me at this bug...

I've been mocking an OAI server very simply using Test::TCP and Net::Server::HTTP, so that I can test my OAI client.

The responses are all hard-coded at the moment, and it doesn't check the incoming requests besides a few verbs, but it's a start. It's 5:33pm on a Friday here, so I don't have time to flesh this out at the moment.

However, here's the bit of code I'm using:

my $server = Test::TCP->new(
    code => sub {
        my $port = shift;
        Net::Server::HTTP->run(
            log_level => 0,
            port => [$port],
            app => {
                '/' => sub {
                    my ($server) = @_;
                    my $request = $server->http_request_info;
                    my @uri_strings = ();
                    my @uri_string =  map { $_ if defined $_ } ($request->{request_protocol},"://",$request->{request_path},$request->{query_string});
                    my $uri_string = join("",@uri_string);
                    # my $uri_string = join($request->{request_protocol},"://",$request->{request_path},$request->{query_string});
                    my $uri = URI->new($uri_string);
                    my %query_parameters = $uri->query_form;
                    print "Content-type: text/xml\n\n";
                    if (%query_parameters){
                        if ($query_parameters{verb} eq "Identify"){
                            print "$identify_xml\n";
                        }
                        elsif ($query_parameters{verb} eq "GetRecord"){
                            print "$getrecord_xml\n";
                        }
                        elsif ($query_parameters{verb} eq "ListRecords"){
                            print "$listrecords_xml\n";
                        }
                    }
                    else {
                        print "$error_xml\n";
                    }
                },
            },
        );
    },
);

I imagine it should be fairly easy to test the Koha::OAI::Server modules by building off this pattern. 

Call $server->port to get the port to which the test server has bound, and you're able to connect to the server using localhost.

> I've been mocking an OAI server very simply using Test::TCP and
> Net::Server::HTTP, so that I can test my OAI client.

db_dependent/OAI/Server.t already mocks Koha OAI Server. But tests are to be extended...

Created attachment 56844 [details] [review]
Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.

QA: Looking here now. Rebasing first.

Created attachment 58058 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

Created attachment 58059 [details] [review]
Remove Koha::AtomicUpdater dependency

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

Created attachment 58060 [details] [review]
Bug 15108: Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.

Just a rebase to begin with. Note that we had merge markers in one of those patches too.

Created attachment 58061 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

If we order by biblionumber within the same timestamp, there is no
need to wait two seconds. Test is 20 seconds faster too. (Especially the
last 2 seconds were very annoying :-)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 58062 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

Created attachment 58063 [details] [review]
Remove Koha::AtomicUpdater dependency

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

EDIT:
Extension should be .perl [Otherwise it will not be executed at all.]
This file is just temporary, needs no license.
Followed the new skeleton file here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 58064 [details] [review]
Bug 15108: Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.

Created attachment 58065 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

If we order by biblionumber within the same timestamp, there is no
need to wait two seconds. Test is 20 seconds faster too. (Especially the
last 2 seconds were very annoying :-)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Only adjusted dbrev now.

Hm I have serious doubts about my last fourth patch. Will the adjusted order by undo the performance gain from the new indexes?

Comment on attachment 58065 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

Obsoleting it. We need to make the test more intelligent some other way.

Truncate operations cause an implicit commit, and so cannot be rolled back. 
I discovered that now too in my test database..
Will adjust the statement :)

Created attachment 58072 [details] [review]
Bug 15108: [QA Follow-up] Remove truncate

Truncate operations cause an implicit commit, and so cannot be rolled back.
So we need to replace it by a DELETE and adjust the code so that it
does not assume numbering biblionumbers from 1, etc.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 58080 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

Ordering by timestamp, biblionumber in ListBase.pm c/would be a
performance killer.
In order to speed up the test, we manipulate the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

QA: Unfortunately I cannot finish this QA session now. Will resume it later next week. Please wait :)

QA: Resuming today.

Created attachment 58245 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

Created attachment 58246 [details] [review]
Bug 15108: Remove Koha::AtomicUpdater dependency

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

EDIT:
Extension should be .perl [Otherwise it will not be executed at all.]
This file is just temporary, needs no license.
Followed the new skeleton file here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 58247 [details] [review]
Bug 15108: Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.

Created attachment 58248 [details] [review]
Bug 15108: [QA Follow-up] Remove truncate

Truncate operations cause an implicit commit, and so cannot be rolled back.
So we need to replace it by a DELETE and adjust the code so that it
does not assume numbering biblionumbers from 1, etc.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 58249 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

Ordering by timestamp, biblionumber in ListBase.pm c/would be a
performance killer.
In order to speed up the test, we manipulate the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 58250 [details] [review]
Bug 15108: [QA Follow-up] Changes from removing collection wrapper

See bug 17744.

Created attachment 58251 [details] [review]
Bug 15108: [QA Follow-up] Serious doubts on ResumptionToken::deleted

ListBase: Remove a comma construction by two statements.
ResumptionToken: Tidy sub deleted, added FIXME.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

The refactoring to ListBase looks good.
But although I spent hours on this patch set already, I am not yet convinced that we are ready to go.

Please respond to my doubts on the sub deleted (see commit message).
Please check this too:
Running aoi with
marcxml/50/1970-01-01T00:00:00Z/2016-12-16T23:59:59Z//1
starts with the first record (not expected !)
Running
marcxml/50/1970-01-01T00:00:00Z/2016-12-16T23:59:59Z//0
starts with record 51 (as expected)

If you propose further changes, please add a follow-up in order to keep sight of the changes.

Failed QA for now

One further note (from QA tools):
FAIL   Koha/OAI/Server/ListIdentifiers.pm
FAIL   Koha/OAI/Server/ListRecords.pm
   FAIL   valid
                Can't locate package HTTP::OAI::PartialList for @HTTP::OAI::ListIdentifiers::ISA

This seems to be a problem in HTTP::OAI::ListIdentifiers ?

Thanks for your work Marcel.

Needs rebase due to bug 17778

I'm finally able to get back to this. Thanks everyone for the work you've done meanwhile. I'll try to rebase and address any issues soon.

Created attachment 60260 [details] [review]
Bug 15108 - OAI-PMH provider improvements

This patch incorporates the extended test from bug 17493 and all other patches (if I didn't mess up anything!). It also fixes a record range display problem in OAI.xslt and reverts the order back to biblionumber to avoid a situation where a record update could have changed the order of records and cause records to be skipped.

This also fixes the QA problem with PartialList not found, removes the broken and unused deleted method from ResumptionToken, fixes tests and fixes resumptionToken cursor handling etc. compared to the previous patch. 

Here's the complete change list compared to master branch:

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

Created attachment 60331 [details] [review]
[SIGNED-OFF] Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

- Incorporated extended tests from Bug 17493 and their tweaks from Bug 15108.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Ere: Did you move all tests here now ?

Marcel: Yes, this patch should include all the tests from Bug 17493. If there were even more somewhere, then I've missed them.

(In reply to Ere Maijala from comment #80)
> Marcel: Yes, this patch should include all the tests from Bug 17493. If
> there were even more somewhere, then I've missed them.

OK I closed 17493

The tests fail for me:

1. If I have data in the issues table I get an error :

> DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key
> constraint fails (`koha`.`issues`, CONSTRAINT `issues_ib fk_2` FOREIGN KEY
> (`itemnumber`) REFERENCES `items` (`itemnumber`) ON UPDATE CASCADE) [for
> Statement "DELETE FROM biblio"] at t/db_dependent/OAI/Server.t line 62.
This can be easily fixed by deleting all issues before deleting all biblios

2. Biblio records are MARC21 (245$a for the title), so, as a UNIMARC user I get nothing in dc:title (tests "ListRecords oai_dc ...").

3. I don't know if this one is related to MARC21/UNIMARC but I also get nothing in dc:language (hashref expected) (tests "ListRecords oai_dc ...")

Result of prove here: http://paste.koha-community.org/325

Marcel: Was there a reason for removing the deletion of issues in your patch in Bug 17493?

I suppose the issue with UNIMARC is that the OAI-PMH provider does take it into account while the test doesn't.

I'll see if I can fix these issues with a follow-up patch to the test.

(In reply to Ere Maijala from comment #83)
> Marcel: Was there a reason for removing the deletion of issues in your patch
> in Bug 17493?
> 
> I suppose the issue with UNIMARC is that the OAI-PMH provider does take it
> into account while the test doesn't.
> 
> I'll see if I can fix these issues with a follow-up patch to the test.

Not sure anymore (seen too much code in the meantime).
Guess it was not needed..

I suppose that would have been the case with a clean database. I'll revise the tests a bit to assume it's not clean (delete issues and oai_sets at least).

Created attachment 60608 [details] [review]
Fixed tests to be more robust and also work with UNIMARC.

Added a patch to improve the server tests:
- delete issues before biblios
- delete oai_sets to avoid having them in the response
- create UNIMARC test records when UNIMARC is enabled
- include language code in the records to avoid failures caused by minor differences in how a missing language code is handled in OAI_DC transformation of different MARC flavors.

Created attachment 60835 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

- Incorporated extended tests from Bug 17493 and their tweaks from Bug 15108.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Created attachment 60836 [details] [review]
Bug 15108 - OAI-PMH provider improvements

Fixed OAI-PMH Server tests to delete any existing issues before deleting biblios, to delete oai_sets to avoid sets in the responses and to work with UNIMARC and NORMARC as well as MARC 21.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

*** Bug 15406 has been marked as a duplicate of this bug. ***

Created attachment 63172 [details] [review]
Bug 15108: OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

- Incorporated extended tests from Bug 17493 and their tweaks from Bug 15108.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 63173 [details] [review]
Bug 15108: (followup) OAI-PMH provider improvements

Fixed OAI-PMH Server tests to delete any existing issues before deleting biblios,
to delete oai_sets to avoid sets in the responses and to work with UNIMARC and
NORMARC as well as MARC21.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 63174 [details] [review]
Bug 15108: (QA followup) Fix atomic update file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

This patchset is a really cool improvement to our OAI server implementation. The patches themselves are not organized as we are pushing for, but the overall result (covered by more tests) and the speed improvement (plus the small fixes) are a good point, and the fact that Frederic has been looking after this one (I talked him about it personally) gives me the confidence that this is a good addition for the 17.05 release.

Good job, Ere :-D

Pushed to master for 17.05, thanks Ere, Tomas!

I think part of this would well qualify as bug fixes, but overall it's a big patch and I am not confident pushing it to the stable version. What do people think?

One of the OAI unit tests seems to have an issue with transactions.

(In reply to Marcel de Rooy from comment #96)
> One of the OAI unit tests seems to have an issue with transactions.

No, it is Sets.t and it is not in this patch set.

This won't get ported back to 16.11.x as it is an enhancement.