|
Description
Pablo AB
2018-07-23 19:42:32 UTC
This makes a HUGE difference for anyone not in a UTC timezone. I think this should be higher priority... Currently, the code to get the earliestDatestamp looks like this:
earliestDatestamp => _get_earliest_datestamp() || '0001-01-01T00:00:00Z',
and
sub _get_earliest_datestamp {
my $dbh = C4::Context->dbh;
my ( $earliest ) = $dbh->selectrow_array("SELECT MIN(timestamp) AS earliest FROM biblio" );
return $earliest
}
There are two problems here:
1. From the MySQL docs: "MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back from UTC to the current time zone for retrieval." Similarly for MariaDB: "If a column uses the TIMESTAMP data type, then any inserted values are converted from the session's time zone to Coordinated Universal Time (UTC) when stored, and converted back to the session's time zone when retrieved." But we MUST get this as a UTC value according to the OAI-PMH spec (see http://www.openarchives.org/OAI/openarchivesprotocol.html#Identify and http://www.openarchives.org/OAI/openarchivesprotocol.html#Dates )
2. It's formatted in the default MySQL way, which is YYYY-MM-DD hh:mm:ss (local time) rather than YYYY-MM-DDThh:mm:ssZ (UTC).
I don't really know Perl, but I can suggest a SQL query that should fix the problem:
SELECT DATE_FORMAT(CONVERT_TZ(MIN(timestamp), 'SYSTEM', '+00:00'), '%Y-%m-%dT%H:%i:%SZ') AS earliest FROM biblio;
Example results:
Before:
+---------------------+
| earliest |
+---------------------+
| 2012-11-03 13:41:32 |
+---------------------+
After:
+----------------------+
| earliest |
+----------------------+
| 2012-11-03T11:41:32Z |
+----------------------+
Rudolf: It's just a formatting issue. If you examine your data, you'll see that the timestamp reported by oai.pl is in UTC. This is thanks to some cleverness in "Koha/OAI/Server/Repository.pm" that sets the the time zone to UTC in the MySQL client session. (Thanks Ere :D) So the following should be sufficient: SELECT DATE_FORMAT(MIN(timestamp),'%Y-%m-%dT%H:%i:%SZ') AS earliest FROM biblio; Created attachment 119604 [details] [review] Proposed patch Thanks for writing the patch, Rudolf. Have you tested it out? I haven't tried it myself yet, but at a glance it contains a syntax error. You've used single quotes around the SQL, but the SQL contains single quotes, so it will bust. Take a look at https://perldoc.perl.org/perlop#Quote-and-Quote-like-Operators. You could replace the outer single quotes with something like q{}. Created attachment 119692 [details] [review] New patch. Fixed quote problem and tested. Sorry, I did not test the previous patch properly. Here is a new one. I tested it this time. It fixes the problem in the XML response, but it stills seems to be formatted as YYYY-MM-DD hh:mm:ss instead of YYYY-MM-DDThh:mm:ssZ in the browser. Not sure if that's a (separate) bug or a feature. Setting back to Needs Sign-off as the comment from David has been taken care of in the new patch. (In reply to Rudolf Byker from comment #6) > Created attachment 119692 [details] [review] [review] > New patch. Fixed quote problem and tested. > > Sorry, I did not test the previous patch properly. Here is a new one. I > tested it this time. It fixes the problem in the XML response, but it stills > seems to be formatted as YYYY-MM-DD hh:mm:ss instead of YYYY-MM-DDThh:mm:ssZ > in the browser. Not sure if that's a (separate) bug or a feature. I think that it would be reasonable to handle it here. Take a look at ./koha-tmpl/opac-tmpl/xslt/OAI.xslt. You'll see that the characters T and Z are being stripped out of the datestamps. Do you mean that they are only stripped out when displayed in the browser? In that case, all is well. (In reply to Rudolf Byker from comment #9) > Do you mean that they are only stripped out when displayed in the browser? > In that case, all is well. Yes, the XSLT is sent in the browser response, and the browser applies the XSLT to the XML to generate a HTML document. That XSLT strips out the 'T' and the 'Z'. Hi there. Who needs to sign off on this issue? Is there anything else I can do to help speed up the process? (In reply to Rudolf Byker from comment #11) > Hi there. Who needs to sign off on this issue? Is there anything else I can > do to help speed up the process? Hi Rudolf, at this stage, anyone can sign off - there is not much you can do, but maybe talk to people who might be interested in this patch got gain a sign off. Sometimes an email to the mailing list might help. Hi Rudolf. Could you write up a test plan so that I could test? This should be a step by step list of things to do, for example: Test plan: 1. [Steps to replicate the issue] 2. Apply the patch 3. [Steps that indicate the issue is fixed, such as repeating steps in 1 and the result expected] Here is a good example: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28408#c3 And also: https://wiki.koha-community.org/wiki/Commit_messages#Test_plan Bearing in mind that those testing (like me) may not be developers or familiar with this area of Koha. David Nind Sorry for the late reply. The test plan is in the commit message. Here it is again: 1) Get a Koha instance with OAI-PMH enabled. 2) Visit /cgi-bin/koha/oai.pl?verb=Identify and observe earliestDate in wrong format "YYYY-MM-DD hh:mm:ss" 3) Apply patch 4) Visit /cgi-bin/koha/oai.pl?verb=Identify and observe earliestDate in correct format "YYYY-MM-DDThh:mm:ssZ" 5) Sign off Created attachment 126236 [details] Bug 21105 - Screenshots before and after patch applied Thanks for the test plan! I noticed no difference in the display before and after the patch was applied. Created attachment 126240 [details] Screenshot of difference in raw XML That's because of the CSS. I should have been more specific: Disable CSS, or click on "View Page Source". Or use this validator, which also shows the raw XML, but pretty-printed: https://validator.oaipmh.com/ I attached my screenshot showing the difference in the raw XML. Created attachment 126241 [details] [review] Bug 21105: oai.pl returns invalid earliestDatestamp For OAI-PMH, all date and time values must be in the format "YYYY-MM-DDThh:mm:ssZ" and in UTC. Currently, Identify.earliestDatestamp uses the SQL format "YYYY-MM-DD hh:mm:ss". This patch fixes that. To test: 1) Get a Koha instance with OAI-PMH enabled. 2) Visit /cgi-bin/koha/oai.pl?verb=Identify, view source for the page and observe that earliestDate is in the wrong format "YYYY-MM-DD hh:mm:ss" 3) Apply patch 4) Visit /cgi-bin/koha/oai.pl?verb=Identify, view the source for the page and observe that earliestDate is now in the correct format "YYYY-MM-DDThh:mm:ssZ" 5) Sign off Sponsored-by: Reformational Study Centre Signed-off-by: David Nind <david@davidnind.com> (In reply to Rudolf Byker from comment #16) > That's because of the CSS. I should have been more specific: Disable CSS, or > click on "View Page Source". Or use this validator, which also shows the raw > XML, but pretty-printed: https://validator.oaipmh.com/ I attached my > screenshot showing the difference in the raw XML. Thanks Rudolf! Have signed off (have updated the patch to incorporate that). Looking here. This requires regression tests, I'm writing them and will come back shortly. Created attachment 127586 [details] [review] Bug 21105: oai.pl returns invalid earliestDatestamp For OAI-PMH, all date and time values must be in the format "YYYY-MM-DDThh:mm:ssZ" and in UTC. Currently, Identify.earliestDatestamp uses the SQL format "YYYY-MM-DD hh:mm:ss". This patch fixes that. To test: 1) Get a Koha instance with OAI-PMH enabled. 2) Visit /cgi-bin/koha/oai.pl?verb=Identify, view source for the page and observe that earliestDate is in the wrong format "YYYY-MM-DD hh:mm:ss" 3) Apply patch 4) Visit /cgi-bin/koha/oai.pl?verb=Identify, view the source for the page and observe that earliestDate is now in the correct format "YYYY-MM-DDThh:mm:ssZ" 5) Sign off Sponsored-by: Reformational Study Centre Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 127587 [details] [review] Bug 21105: Regression tests This patch introduces a regression test for the fixed behavior. I also changed a oneliner regex we had for stripping query parameters because it was causing a warning with the tests base URL (http://localhost) because it was generating an empty baseURL. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/OAI/Server.t => FAIL: Tests fail, the response is incorrect! 3. Apply the bug's patch 4. Repeat 2 => SUCCESS: Tests pass! Good response! Created attachment 127588 [details] [review] Bug 21105: (QA follow-up) Silence warning because of tests setup The tests are using 'http://localhost' and the regex is setting $baseURL to empty in this case. It works on the oai.pl front-end. This patch changes the regex. To test: 1. Run: $ kshell k$ t/db_dependent/OAI/Server.t => FAIL: Boo! Warning! 2. Apply this patch 3. Repeat 2 => SUCCESS: Oh, cool. No warning :-D 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 127589 [details] [review] Bug 21105: (QA follow-up) Avoid MySQL-ism Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 127628 [details] [review] Bug 21105: (follow-up) Regression tests Mock KohaAdminEmailAddress in case it differs from the default value Pushed to master for 21.11, thanks to everybody involved! Created attachment 127644 [details] [review] Bug 21105: Fix test if sample data missing # Failed test 'Identify - earliestDatestamp in the right format' # at t/db_dependent/OAI/Server.t line 182. # Compared $data->{"Identify"}{"earliestDatestamp"} # got : '2021-11-15T13:16:09Z' # expect : '2014-05-07T13:36:23Z' (In reply to Jonathan Druart from comment #26) > Created attachment 127644 [details] [review] [review] > Bug 21105: Fix test if sample data missing > > # Failed test 'Identify - earliestDatestamp in the right format' > # at t/db_dependent/OAI/Server.t line 182. > # Compared $data->{"Identify"}{"earliestDatestamp"} > # got : '2021-11-15T13:16:09Z' > # expect : '2014-05-07T13:36:23Z' Aren't we then missing the point of the test with this patch? Created attachment 127650 [details] [review] Bug 21105: Fix test if sample data missing # Failed test 'Identify - earliestDatestamp in the right format' # at t/db_dependent/OAI/Server.t line 182. # Compared $data->{"Identify"}{"earliestDatestamp"} # got : '2021-11-15T13:16:09Z' # expect : '2014-05-07T13:36:23Z' Created attachment 127653 [details] [review] Bug 21105: Fix test if sample data missing # Failed test 'Identify - earliestDatestamp in the right format' # at t/db_dependent/OAI/Server.t line 182. # Compared $data->{"Identify"}{"earliestDatestamp"} # got : '2021-11-15T13:16:09Z' # expect : '2014-05-07T13:36:23Z' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #29) > Created attachment 127653 [details] [review] [review] > Bug 21105: Fix test if sample data missing > > # Failed test 'Identify - earliestDatestamp in the right format' > # at t/db_dependent/OAI/Server.t line 182. > # Compared $data->{"Identify"}{"earliestDatestamp"} > # got : '2021-11-15T13:16:09Z' > # expect : '2014-05-07T13:36:23Z' > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master. Pushed to 21.05.x for 21.05.05 Does not apply on 20.11.x, fails on t/db_dependent/OAI/Server.t because of missing Bug 29135 |