Created attachment 151720 [details] [review] Bug 33837: Add datetime comparison to filter_by_last_update Test plan: Verify that filter_by_last_update still works as expected by running cleanup_database.pl with --messages DAYS parameter. Look at the oldest messages.message_date (or insert one). Run with -days X where today - X is before that date. Run with -days Y where today - Y is just after it. Note: misc/cronjobs/cleanup_database.pl -confirm -messages -1 -v will remove all messages including one you inserted just now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 151721 [details] [review] Bug 33837: Add unit test Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 151722 [details] [review] Bug 33837: filter_by_last_update: Add older_than and younger_than Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 151723 [details] [review] Bug 33837: Replace days_inclusive by min_days Instead of passing days and days_inclusive, this patch adds min_days as replacement. Since days_inclusive is not widely used, this can be done easily. It removes the confusion whether days_inclusive impacted other parameters or not. Test plan: Run t/db_dependent/Koha/Objects.t Run t/db_dependent/Koha/Old/Checkouts.t Run t/db_dependent/Koha/Patrons.t (verifying the change in the Koha::Patrons module for filtering by expiration date). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The datetime switch is weird. I am wondering if we should not switch to always datetime, and adjust the existing occurrence, I've caught only one. Setting HH:MM to 00:00. Would that work?
Created attachment 153778 [details] [review] Bug 33837: Remove datetime parameter
Patch for discussion.
Last patch should not do this: diff --git a/Koha/Notice/Util.pm b/Koha/Notice/Util.pm index 4e515d4b5db..903550b893e 100644 --- a/Koha/Notice/Util.pm +++ b/Koha/Notice/Util.pm @@ -122,7 +122,7 @@ sub _get_domain_count { message_transport_type => 'email', status => 'sent', to_address => { 'LIKE', '%'.$domain }, - })->filter_by_last_update({ timestamp_column_name => 'updated_on', from => $start_dt, datetime => 1 })->count; + })->filter_by_last_update({ timestamp_column_name => 'updated_on', from => $start_dt, })->count; } $limits->{$group}->{count} = $sum; }
/usr/share/koha/t/db_dependent/Koha/Old/Checkouts.t (Wstat: 256 Tests: 2 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=2, 7 wallclock secs ( 0.05 usr 0.00 sys + 5.75 cusr 1.14 csys = 6.94 CPU) Result: FAIL /usr/share/koha/t/db_dependent/Koha/Objects.t (Wstat: 256 Tests: 23 Failed: 1) Failed test: 19 Non-zero exit status: 1 Files=1, Tests=23, 19 wallclock secs ( 0.07 usr 0.01 sys + 14.52 cusr 3.00 csys = 17.60 CPU) Result: FAIL
Without last patch: Running tests (2) * Proving /usr/share/koha/t/db_dependent/Koha/Objects.t OK! * Proving /usr/share/koha/t/db_dependent/Koha/Old/Checkouts.t OK!
(In reply to Jonathan Druart from comment #5) > The datetime switch is weird. The name could be improved? Was struggling with that. I had first something like truncate_to_days. Open for suggestions here. Note that I did this to keep backward compatibility (or 'no behavior change'). > I am wondering if we should not switch to always datetime, and adjust the > existing occurrence, I've caught only one. Setting HH:MM to 00:00. Would > that work? I would rather do this on a separate report. Change behavior ;)
Comment on attachment 153778 [details] [review] Bug 33837: Remove datetime parameter Moving this to obsolete for now
Note that there is always a difference between last 3 days meaning TODAY, TODAY-1 and TODAY-2 (and TODAY-3?) or so and last 3*24 hours.
compare_time => 1 ?
Created attachment 153898 [details] [review] Bug 33837: Remove datetime parameter
New suggestion, tests are passing now. What do you think?
Created attachment 153900 [details] [review] Bug 33837: Remove datetime parameter
(In reply to Jonathan Druart from comment #16) > New suggestion, tests are passing now. What do you think? Thanks. Having a look now.
Kind of distracted by this code construction in the context: try { $count = $patrons->filter_by_last_update( { timestamp_column_name => 'updated_on', from => '1970-12-31' } ) ->count; print 0/0; } catch { ok( $_->isa( 'No exception raised, from and to parameters can take an iso formatted date' ) ); }; The print is from me. We pass a string too to isa. So isa compares the exception with the string and always returns false to ok. And ok raises a warning without description. The description was obviously meant for ok and not for isa.
Funny remember the cardnumber patch? t/db_dependent/Koha/Patrons.t ........ 27/43 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Duplicate entry '123456789' for key 'cardnumber' at /usr/share/koha/Koha/Object.pm line 170 Still have that borrower and it makes Patrons.t fail.
Created attachment 153915 [details] [review] Bug 33837: Add datetime comparison to filter_by_last_update Test plan: Verify that filter_by_last_update still works as expected by running cleanup_database.pl with --messages DAYS parameter. Look at the oldest messages.message_date (or insert one). Run with -days X where today - X is before that date. Run with -days Y where today - Y is just after it. Note: misc/cronjobs/cleanup_database.pl -confirm -messages -1 -v will remove all messages including one you inserted just now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153916 [details] [review] Bug 33837: Add unit test Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153917 [details] [review] Bug 33837: filter_by_last_update: Add older_than and younger_than Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153918 [details] [review] Bug 33837: Replace days_inclusive by min_days Instead of passing days and days_inclusive, this patch adds min_days as replacement. Since days_inclusive is not widely used, this can be done easily. It removes the confusion whether days_inclusive impacted other parameters or not. Test plan: Run t/db_dependent/Koha/Objects.t Run t/db_dependent/Koha/Old/Checkouts.t Run t/db_dependent/Koha/Patrons.t (verifying the change in the Koha::Patrons module for filtering by expiration date). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153919 [details] [review] Bug 33837: Remove datetime parameter Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Perltidied one line in cleanup_database.pl.
t/db_dependent/Koha/Objects.t # Subtest: filter_by_last_update ok 1 - Should raise an exception if no parameter given ok 2 - filter_by_last_update must return a Koha::Objects-based object ok 3 - 3 patrons have been updated before the last 2 days (exclusive) ok 4 - 4 patrons have been updated before the last 2 days (inclusive) ok 5 - 4 patrons have been updated before yesterday (exclusive) ok 6 - 5 patrons have been updated before yesterday (inclusive) ok 7 - 5 patrons have been updated before today (exclusive) ok 8 - 6 patrons have been updated before today (inclusive) ok 9 - 1 patron has been updated "from today" (inclusive) ok 10 - 6 patrons have been updated "to today" (inclusive) ok 11 - 3 patrons have been updated between D-4 and D-2 # Subtest: Parameters older_than, younger_than ok 1 - All updated yesterday ok 2 - Yesterday, truncated from is inclusive ok 3 - Yesterday, not truncated, one hour too late ok 4 - Yesterday - 1h, not truncated, within time frame 1..4 ok 12 - Parameters older_than, younger_than 1..12 ok 19 - filter_by_last_update
prove t/db_dependent/Koha/Pseudonymization.t All tests successful. Files=1, Tests=3, 4 wallclock secs ( 0.03 usr 0.00 sys + 3.32 cusr 0.37 csys = 3.72 CPU) Result: PASS prove t/db_dependent/Koha/Old/Checkouts.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Old/Checkouts.t .. ok t/db_dependent/Koha/Patrons.t ........ ok All tests successful. Files=2, Tests=45, 61 wallclock secs ( 0.20 usr 0.04 sys + 47.23 cusr 9.25 csys = 56.72 CPU) Result: PASS
Created attachment 153920 [details] [review] Bug 33837: Perl tidy block in Objects.t WARN t/db_dependent/Koha/Objects.t WARN tidiness The file is less tidy than before (bad/messy lines before: 319, now: 320) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153947 [details] [review] Bug 33837: Add datetime comparison to filter_by_last_update Test plan: Verify that filter_by_last_update still works as expected by running cleanup_database.pl with --messages DAYS parameter. Look at the oldest messages.message_date (or insert one). Run with -days X where today - X is before that date. Run with -days Y where today - Y is just after it. Note: misc/cronjobs/cleanup_database.pl -confirm -messages -1 -v will remove all messages including one you inserted just now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 153948 [details] [review] Bug 33837: Add unit test Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 153949 [details] [review] Bug 33837: filter_by_last_update: Add older_than and younger_than Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 153950 [details] [review] Bug 33837: Replace days_inclusive by min_days Instead of passing days and days_inclusive, this patch adds min_days as replacement. Since days_inclusive is not widely used, this can be done easily. It removes the confusion whether days_inclusive impacted other parameters or not. Test plan: Run t/db_dependent/Koha/Objects.t Run t/db_dependent/Koha/Old/Checkouts.t Run t/db_dependent/Koha/Patrons.t (verifying the change in the Koha::Patrons module for filtering by expiration date). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 153951 [details] [review] Bug 33837: Remove datetime parameter Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Perltidied one line in cleanup_database.pl. Bug 33837: Perl tidy block in Objects.t WARN t/db_dependent/Koha/Objects.t WARN tidiness The file is less tidy than before (bad/messy lines before: 319, now: 320) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Thanks
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x