|
Description
Marcel de Rooy
2023-05-25 13:55:51 UTC
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 |