https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 127247 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127247&action=edit Bug 29403: Match the whole string This patch wraps the constructed regex such that we expect the entire regex to match and don't allow leading or trailing garbage. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29402 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127247|0 |1 is obsolete| | --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128077 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128077&action=edit Bug 29403: Match the whole string This patch wraps the constructed regex such that we expect the entire regex to match and don't allow leading or trailing garbage. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128077|0 |1 is obsolete| | --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128273&action=edit Bug 29403: Match the whole string This patch wraps the constructed regex such that we expect the entire regex to match and don't allow leading or trailing garbage. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|tomascohen@gmail.com |testopia@bugs.koha-communit | |y.org Status|Passed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128272|0 |1 is obsolete| | Attachment #128273|0 |1 is obsolete| | Attachment #128274|0 |1 is obsolete| | --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128450 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128450&action=edit Bug 29403: Unit tests Add a unit test for proper iso date handling Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128452 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128452&action=edit Bug 29403: Fix api/v1/patrons.t t::lib::Dates::compare expects either correct DateTime objects, or a system formatted datetime strings to be passed.. but as we're testing the API, we are consistently using RFC3339 strings for date-times and ISO8601 strings for dates. We need to therefore pre-process the strings into DateTime objects for the comparison. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |22.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29711 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29711 [Bug 29711] Release date not passed as ISO to about.tt -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Fridolin Somers from comment #16)

Pushed to master for 22.05, thanks to everybody involved 🦄

This is causing failures on t/db_dependent/Circulation.t not ok 17 - CanBookBeRenewed tests # Failed test 'CanBookBeRenewed tests' # at t/db_dependent/Circulation.t line 1450. The given date (2021-12-15T16:06:30) does not match the date format (us) at /kohadevbox/koha/Koha/DateUtils.pm line 197. # Looks like your test exited with 11 just after 17. Dubious, test returned 11 (wstat 2816, 0xb00) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |ASSIGNED Version(s)|22.05.00 | released in| | --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- I revert from master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Fridolin Somers from comment #19)

I revert from master

Ideally we prefer to not revert but fix. It will get messy if you revert every time jenkins is failing after a push :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I had a feeling this might throw a few issues.. I kinda hoped to have it pushed and help deal with the repercussions.... or that we'd team together to try and spot repercussions and fix them before the push. Honestly.. I thought I'd run the whole test suit though, so that's my bad.. I should have got the tests all passing first. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #24 from Fridolin Somers <fridolin.somers@biblibre.com> --- Ah good points. I've dig about an hour trying to fix but couldn't get it work.

This one is fun. We are storing a DT into a varchar (rule_value) and so the DT object is stringified using ISO 8601. And yes, this is closely related to bug 29718. Ah that is complex for me. We cant change how DT is stringified right ? So we better change how we store. Maybe a Koha::DateUtils dt_to_sql method ?

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #26 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Jonathan Druart from comment #25)

...

So we better change how we store. Maybe a Koha::DateUtils dt_to_sql method ?

Hum? I am not sure I understand that part. This is DBMS internals, we cannot modify how we store date/datetime/timestamp.

I mean a method to stringify a DT into a string that we store in database. Called "dt_to_sql" as oposite of "dt_from_string($str, 'sql')". We may call it "dt_to_string". -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #28 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Martin Renvoize from comment #27)

Does bug 29718 not basically resolve the test failure this introduces? I'll test.

In error we see : The given date (2021-12-15T16:06:30) does not match the date format (us) Realy strange to see US format there, looks like its a call on "dt_from_string" without explicit format. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #30 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- OK.. I took a deeper look and my assumptions were correct.. so I applied the two patchsets and ran the tests again.. and for me.. they pass as expected?! Are you sure you had both this patchset and bug 29718 applied? It is a bit of a funky test... in smart-rules we actually grab the passed date, convert to a datetime, then convert to an iso string and drop the time component. In the test, we're just passing a DateTime object, so it'll include a time element and it'll contextually stringify using the datetime defaults (to ISO including Time - Hense the 'T'). What is happening in our DateUtils is that we try to match on preference format ('us' in this case which will fail) and then fallback to 'sql/iso'. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 128825 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128825&action=edit Bug 29403: Fix unit test for bug 29718 Including a 'T' is actually valid for ISO date format, I was being naughty and writing the test to follow our code rather than writing the test to conform to the ISO spec first time around. Now we are properly supporting 'T' in ISO dates with bug 29718, we need to update the test here to properly reflect an invalid ISO date string. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128451|0 |1 is obsolete| | --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 128827 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128827&action=edit Bug 29403: Match the whole string This patch wraps the constructed regex such that we expect the entire regex to match and don't allow leading or trailing garbage. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128825|0 |1 is obsolete| | --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 128829 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128829&action=edit Bug 29403: Fix unit test for bug 29718 Including a 'T' is actually valid for ISO date format, I was being naughty and writing the test to follow our code rather than writing the test to conform to the ISO spec first time around. Now we are properly supporting 'T' in ISO dates with bug 29718, we need to update the test here to properly reflect an invalid ISO date string. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I reversed the dependencies and fixed a badly written test I included here in the first place. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #38 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129374 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129374&action=edit Bug 29403: Match the whole string This patch wraps the constructed regex such that we expect the entire regex to match and don't allow leading or trailing garbage. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #40 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129376 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129376&action=edit Bug 29403: Fix unit test for bug 29718 Including a 'T' is actually valid for ISO date format, I was being naughty and writing the test to follow our code rather than writing the test to conform to the ISO spec first time around. Now we are properly supporting 'T' in ISO dates with bug 29718, we need to update the test here to properly reflect an invalid ISO date string. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129374|0 |1 is obsolete| | --- Comment #42 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 129392 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129392&action=edit Bug 29403: Match the whole string This patch wraps the constructed regex such that we expect the entire regex to match and don't allow leading or trailing garbage. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129376|0 |1 is obsolete| | --- Comment #44 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 129394 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129394&action=edit Bug 29403: Fix unit test for bug 29718 Including a 'T' is actually valid for ISO date format, I was being naughty and writing the test to follow our code rather than writing the test to conform to the ISO spec first time around. Now we are properly supporting 'T' in ISO dates with bug 29718, we need to update the test here to properly reflect an invalid ISO date string. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #46 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Having a look here now -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #48 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- - t::lib::Dates::compare( $updated_on_got, $updated_on_expected, 'updated_on values matched' ); + t::lib::Dates::compare( + dt_from_string( $updated_on_got, 'rfc3339' ), + dt_from_string( $updated_on_expected, 'rfc3339' ), + 'updated_on values matched' + ); But what does t::lib::Dates? sub compare { my ( $got, $expected ) = @_; my $dt_got = dt_from_string($got); my $dt_expected = dt_from_string($expected); my $diff = $dt_got->epoch - $dt_expected->epoch; if ( abs($diff) <= 5 ) { return 0 } return $diff > 0 ? 1 : -1; } So $got en $expected should be strings, right? Does this actually show that we miss a parameter in sub compare, and we are solving it by twisting the test instead? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |enhancement --- Comment #50 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #15)

In my opinion we should not backport those patches.

Seems to be wise. Will mark this as enh. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #52 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- commit dfcb2b8071582a55e35c47926d4b5c1b8b2a9a59 Author: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Date: Fri Aug 25 17:56:52 2017 -0300 Bug 19176: Compare the number of seconds when comparing dates in tests Five years go fast. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #54 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 129473 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129473&action=edit Bug 29403: (QA follow-up) Add comments why test should fail Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|jonathan.druart+koha@gmail. |m.de.rooy@rijksmuseum.nl |com | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #56 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- * I've adapted the bug title as requested. * Before the last submission, I ran the test suite for db_dependant/api/* and everything is passing. I also ran a few other tests that included dt_from_string.. but not all.. nothing failed so far * The compare function is indeed a bit of a fudge.. but one from quite a while ago.. I'm not sure if we should be attempting to fix that here or not.. I'm also not entirely happy with the shared ISO/SQL/Fallback behaviour.. we're still not being explicit in our iso support (We're allowing for am/pm which is not allowed in ISO). Not sure what more to do here.. happy to discus for clarifications -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |29884 --- Comment #58 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #48)

- t::lib::Dates::compare( $updated_on_got, $updated_on_expected, 'updated_on values matched' ); + t::lib::Dates::compare( + dt_from_string( $updated_on_got, 'rfc3339' ), + dt_from_string( $updated_on_expected, 'rfc3339' ), + 'updated_on values matched' + );

But what does t::lib::Dates?

sub compare { my ( $got, $expected ) = @_; my $dt_got = dt_from_string($got); my $dt_expected = dt_from_string($expected); my $diff = $dt_got->epoch - $dt_expected->epoch; if ( abs($diff) <= 5 ) { return 0 } return $diff > 0 ? 1 : -1; }

So $got en $expected should be strings, right? Does this actually show that we miss a parameter in sub compare, and we are solving it by twisting the test instead?

Will fix on bug 29884. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29884 [Bug 29884] Missing test in api/v1/patrons.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #60 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129487 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129487&action=edit Bug 29403: Fix unit test for bug 29718 Including a 'T' is actually valid for ISO date format, I was being naughty and writing the test to follow our code rather than writing the test to conform to the ISO spec first time around. Now we are properly supporting 'T' in ISO dates with bug 29718, we need to update the test here to properly reflect an invalid ISO date string. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #62 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #58)

(In reply to Marcel de Rooy from comment #48)

...

- t::lib::Dates::compare( $updated_on_got, $updated_on_expected, 'updated_on values matched' ); + t::lib::Dates::compare( + dt_from_string( $updated_on_got, 'rfc3339' ), + dt_from_string( $updated_on_expected, 'rfc3339' ), + 'updated_on values matched' + );

But what does t::lib::Dates?

sub compare { my ( $got, $expected ) = @_; my $dt_got = dt_from_string($got); my $dt_expected = dt_from_string($expected); my $diff = $dt_got->epoch - $dt_expected->epoch; if ( abs($diff) <= 5 ) { return 0 } return $diff > 0 ? 1 : -1; }

So $got en $expected should be strings, right? Does this actually show that we miss a parameter in sub compare, and we are solving it by twisting the test instead?

Will fix on bug 29884.

That seems to be a good fix, but does not fully address my comment. This compare sub runs dt_from_string on its input. So normally I would expect it to have string parameters. The patrons.t test here is doing a dt conversion already with rfc3339. (Compare calls it again.) It would be more consistent if we would pass strings to compare and possibly a type like RFC3339, ISO8601, etc. I am seeing a hardcoded <=5, could be a parameter too or at least a constant? So yes, we are leaving scope here. But compare needs attention, and this patch set triggers it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #64 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #62)

(In reply to Jonathan Druart from comment #58)

...

(In reply to Marcel de Rooy from comment #48)

...

- t::lib::Dates::compare( $updated_on_got, $updated_on_expected, 'updated_on values matched' ); + t::lib::Dates::compare( + dt_from_string( $updated_on_got, 'rfc3339' ), + dt_from_string( $updated_on_expected, 'rfc3339' ), + 'updated_on values matched' + );

But what does t::lib::Dates?

sub compare { my ( $got, $expected ) = @_; my $dt_got = dt_from_string($got); my $dt_expected = dt_from_string($expected); my $diff = $dt_got->epoch - $dt_expected->epoch; if ( abs($diff) <= 5 ) { return 0 } return $diff > 0 ? 1 : -1; }

So $got en $expected should be strings, right? Does this actually show that we miss a parameter in sub compare, and we are solving it by twisting the test instead?

Will fix on bug 29884.

That seems to be a good fix, but does not fully address my comment. This compare sub runs dt_from_string on its input. So normally I would expect it to have string parameters. The patrons.t test here is doing a dt conversion already with rfc3339. (Compare calls it again.) It would be more consistent if we would pass strings to compare and possibly a type like RFC3339, ISO8601, etc. I am seeing a hardcoded <=5, could be a parameter too or at least a constant? So yes, we are leaving scope here. But compare needs attention, and this patch set triggers it.

It's not very nice I agree, but dt_from_string accepts DTs :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #66 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Marcel de Rooy from comment #63)

(In reply to Martin Renvoize from comment #57)

...

OK, Running the whole test suite against this now.. will report back

Thanks. Any results?

All passed, with the exception of a couple of conflict markers that were in a uncommited local file here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129391|0 |1 is obsolete| | --- Comment #67 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 129552 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129552&action=edit Bug 29403: Unit tests Add a unit test for proper iso date handling Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129486|0 |1 is obsolete| | --- Comment #69 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 129554 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129554&action=edit Bug 29403: Fix api/v1/patrons.t t::lib::Dates::compare expects either correct DateTime objects, or a system formatted datetime strings to be passed.. but as we're testing the API, we are consistently using RFC3339 strings for date-times and ISO8601 strings for dates. We need to therefore pre-process the strings into DateTime objects for the comparison. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129488|0 |1 is obsolete| | --- Comment #71 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 129556 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129556&action=edit Bug 29403: (QA follow-up) Add comments why test should fail Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #73 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Martin Renvoize from comment #66)

(In reply to Marcel de Rooy from comment #63)

...

(In reply to Martin Renvoize from comment #57)

...

OK, Running the whole test suite against this now.. will report back

Thanks. Any results?

All passed, with the exception of a couple of conflict markers that were in a uncommited local file here.

Great. Confident enough to pass qa in view of responses etc. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 --- Comment #74 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED CC| |kyle@bywatersolutions.com Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |30030 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30030 [Bug 30030] Test t/db_dependent/Koha/Patrons.t is_going_to_expire fails on U18 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Bug 29403 depends on bug 29718, which changed state. Bug 29718 Summary: DateTime - our 'iso' is not ISO 8601 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29718 What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |--- -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29403 Bug 29403 depends on bug 29718, which changed state. Bug 29718 Summary: DateTime - our 'iso' is not ISO 8601 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29718 What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.