Bug 33837 - Objects->filter_by_last_update: Does not allow time comparison
Summary: Objects->filter_by_last_update: Does not allow time comparison
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 33843 35041
  Show dependency treegraph
 
Reported: 2023-05-25 13:55 UTC by Marcel de Rooy
Modified: 2023-10-14 12:10 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 33837: Add datetime comparison to filter_by_last_update (2.73 KB, patch)
2023-05-26 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Add unit test (1.74 KB, patch)
2023-05-26 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: filter_by_last_update: Add older_than and younger_than (4.62 KB, patch)
2023-05-26 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Replace days_inclusive by min_days (6.22 KB, patch)
2023-05-26 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Remove datetime parameter (6.84 KB, patch)
2023-07-21 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: Remove datetime parameter (6.29 KB, patch)
2023-07-26 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: Remove datetime parameter (6.27 KB, patch)
2023-07-26 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: Add datetime comparison to filter_by_last_update (2.73 KB, patch)
2023-07-26 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Add unit test (1.74 KB, patch)
2023-07-26 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: filter_by_last_update: Add older_than and younger_than (4.62 KB, patch)
2023-07-26 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Replace days_inclusive by min_days (6.22 KB, patch)
2023-07-26 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Remove datetime parameter (6.41 KB, patch)
2023-07-26 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Perl tidy block in Objects.t (1.27 KB, patch)
2023-07-26 12:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33837: Add datetime comparison to filter_by_last_update (2.76 KB, patch)
2023-07-27 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: Add unit test (1.78 KB, patch)
2023-07-27 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: filter_by_last_update: Add older_than and younger_than (4.66 KB, patch)
2023-07-27 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: Replace days_inclusive by min_days (6.26 KB, patch)
2023-07-27 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33837: Remove datetime parameter (6.76 KB, patch)
2023-07-27 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2023-05-25 13:55:51 UTC

    
Comment 1 Marcel de Rooy 2023-05-26 07:56:35 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>
Comment 2 Marcel de Rooy 2023-05-26 07:56:37 UTC
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>
Comment 3 Marcel de Rooy 2023-05-26 07:56:39 UTC
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>
Comment 4 Marcel de Rooy 2023-05-26 07:56:41 UTC
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>
Comment 5 Jonathan Druart 2023-07-21 12:06:42 UTC
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?
Comment 6 Jonathan Druart 2023-07-21 12:32:33 UTC
Created attachment 153778 [details] [review]
Bug 33837: Remove datetime parameter
Comment 7 Jonathan Druart 2023-07-21 12:32:43 UTC
Patch for discussion.
Comment 8 Marcel de Rooy 2023-07-25 12:36:05 UTC
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;
 }
Comment 9 Marcel de Rooy 2023-07-25 12:41:13 UTC
 /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
Comment 10 Marcel de Rooy 2023-07-25 12:42:38 UTC
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!
Comment 11 Marcel de Rooy 2023-07-25 13:00:24 UTC
(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 12 Marcel de Rooy 2023-07-25 13:00:48 UTC
Comment on attachment 153778 [details] [review]
Bug 33837: Remove datetime parameter

Moving this to obsolete for now
Comment 13 Marcel de Rooy 2023-07-25 13:02:22 UTC
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.
Comment 14 Marcel de Rooy 2023-07-25 20:04:28 UTC
compare_time => 1 ?
Comment 15 Jonathan Druart 2023-07-26 08:20:44 UTC
Created attachment 153898 [details] [review]
Bug 33837: Remove datetime parameter
Comment 16 Jonathan Druart 2023-07-26 08:22:34 UTC
New suggestion, tests are passing now. What do you think?
Comment 17 Jonathan Druart 2023-07-26 09:38:49 UTC
Created attachment 153900 [details] [review]
Bug 33837: Remove datetime parameter
Comment 18 Marcel de Rooy 2023-07-26 11:38:00 UTC
(In reply to Jonathan Druart from comment #16)
> New suggestion, tests are passing now. What do you think?

Thanks. Having a look now.
Comment 19 Marcel de Rooy 2023-07-26 12:06:40 UTC
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.
Comment 20 Marcel de Rooy 2023-07-26 12:09:43 UTC
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.
Comment 21 Marcel de Rooy 2023-07-26 12:12:49 UTC
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>
Comment 22 Marcel de Rooy 2023-07-26 12:12:52 UTC
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>
Comment 23 Marcel de Rooy 2023-07-26 12:12:54 UTC
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>
Comment 24 Marcel de Rooy 2023-07-26 12:12:56 UTC
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>
Comment 25 Marcel de Rooy 2023-07-26 12:12:58 UTC
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.
Comment 26 Marcel de Rooy 2023-07-26 12:13:17 UTC
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
Comment 27 Marcel de Rooy 2023-07-26 12:13:30 UTC
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
Comment 28 Marcel de Rooy 2023-07-26 12:17:36 UTC
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>
Comment 29 Jonathan Druart 2023-07-27 09:10:39 UTC
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>
Comment 30 Jonathan Druart 2023-07-27 09:10:42 UTC
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>
Comment 31 Jonathan Druart 2023-07-27 09:10:45 UTC
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>
Comment 32 Jonathan Druart 2023-07-27 09:10:48 UTC
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>
Comment 33 Jonathan Druart 2023-07-27 09:10:51 UTC
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>
Comment 34 Marcel de Rooy 2023-07-27 10:08:59 UTC
Thanks
Comment 35 Tomás Cohen Arazi 2023-10-11 13:34:17 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 36 Fridolin Somers 2023-10-12 21:05:26 UTC
Enhancement not pushed to 23.05.x