Attachment 154777 Details for Bug 33537 – Bug 33537: Connect the dots

[patch] Bug 33537: Connect the dots

Bug-33537-Connect-the-dots.patch (text/plain), 13.13 KB, created by Marcel de Rooy on 2023-08-25 08:21:02 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2023-08-25 08:21:02 UTC

Size: 13.13 KB

patch

obsolete

>From a14a3c8b028e1647d048e2aec9d10550a75d0bee Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Mon, 10 Jul 2023 11:41:01 +0000
>Subject: [PATCH] Bug 33537: Connect the dots
>Content-Type: text/plain; charset=utf-8
>
>Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits.
>Only use koha-conf as fallback when table is still empty. (This is
>just like SMTP servers.)
>
>The test is adjusted accordingly and has moved more to the database
>oriented side, first proving that both approaches return the same.
>
>Test plan:
>Run t/db_dependent/Koha/Notice_Util.t
>
>NOTE: Replace A and B hereunder by two domains of your own choice.
>Add a limit to KOHA-CONF: domain A, 1 per 1 hour.
>Add another limit in WEB CONFIG: domain B, 1 per 1 hour
>Send 2 notices to domain B. Is 1 message delayed?
>Remove the table entry for B.
>Run process_message_queue.pl. Is the delayed notice to B sent?
>Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)?
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> Koha/Notice/Util.pm               | 78 +++++++++++++++++++++----------
> t/db_dependent/Koha/Notice_Util.t | 56 +++++++++++++---------
> 2 files changed, 88 insertions(+), 46 deletions(-)
>
>diff --git a/Koha/Notice/Util.pm b/Koha/Notice/Util.pm
>index 98f6182cf5..2099d3332e 100644
>--- a/Koha/Notice/Util.pm
>+++ b/Koha/Notice/Util.pm
>@@ -22,8 +22,11 @@ use Data::Dumper qw/Dumper/;
> 
> use C4::Context;
> use Koha::DateUtils qw/dt_from_string/;
>+use Koha::MailDomainLimits;
> use Koha::Notice::Messages;
> 
>+use constant ABBREV => { m => 'minutes', h => 'hours', d => 'days' };
>+
> =head1 NAME
> 
> Koha::Notice::Util - Utility class related to Koha notice messages
>@@ -38,35 +41,66 @@ Koha::Notice::Util - Utility class related to Koha notice messages
> 
> sub load_domain_limits {
>     my ( $class ) = @_;
>+    my $rs = Koha::MailDomainLimits->search_with_group_domain;
>+    if ( $rs->count ) {
>+        return { map { _init_domain_entry($_) } @{$rs->unblessed} };
>+    } else { # fall back to koha-conf
>+        return _load_from_file();
>+    }
>+}
> 
>+sub _load_from_file {
>+    my $config = C4::Context->config('message_domain_limits');
>     my $domain_limits;
>-    my $entry = C4::Context->config('message_domain_limits');
>-    if( ref($entry) eq 'HASH' ) {
>-        if( exists $entry->{domain} ) {
>+    if( ref($config) eq 'HASH' ) {
>+        if( exists $config->{domain} ) {
>             # Turn single hash entry into array
>-            $domain_limits = ref($entry->{domain}) eq 'HASH'
>-                ? [ $entry->{domain} ]
>-                : $entry->{domain};
>+            $domain_limits = ref($config->{domain}) eq 'HASH'
>+                ? [ $config->{domain} ]
>+                : $config->{domain};
>             # Convert to hash structure by domain name
>-            $domain_limits = { map { _init_domain_entry($_); } @$domain_limits };
>+            $domain_limits = { map { _init_domain_entry({ %$_ }, 1); } @$domain_limits }; # copy hashes to prevent changes
>         }
>     }
>     return $domain_limits;
> }
> 
> sub _init_domain_entry {
>-    my ( $config_entry ) = @_;
>-    # Return either a hash like ( name => { limit => , unit =>, count => } ) for regular entries
>+    my ( $entry, $old_school ) = @_;
>+    # entry comes from table or koha-conf
>+    # Return either a hash like ( name => { limit => , units =>, unit_type =>, count => } ) for regular entries
>     # or return a hash like ( name => { belongs_to => } ) for a domain that is part of a group
>+    # We need old_school when passing data from koha-conf.xml
>+
>+    if( $old_school ) {
>+        ( $entry->{units}, $entry->{unit_type} ) = _unit_info( delete $entry->{unit} );
>+    } else {
>+        $entry->{name} = delete $entry->{domain_name};
>+        $entry->{limit} = delete $entry->{domain_limit};
>+        $entry->{belongs_to} = delete $entry->{group_domain};
>+    }
> 
>-    return if ref($config_entry) ne 'HASH' || !exists $config_entry->{name};
>+    return if !exists $entry->{name};
>     my $elements;
>-    if( $config_entry->{belongs_to} ) {
>-        $elements = { belongs_to => lc $config_entry->{belongs_to} };
>+    if( $entry->{belongs_to} ) {
>+        $elements = { belongs_to => lc $entry->{belongs_to} };
>     } else {
>-        $elements = { limit => $config_entry->{limit}, unit => $config_entry->{unit}, count => undef };
>+        $elements = {
>+            limit     => $entry->{limit},
>+            units     => $entry->{units},
>+            unit_type => $entry->{unit_type},
>+            count     => undef,
>+        };
>     }
>-    return ( lc $config_entry->{name}, $elements );
>+    return ( lc $entry->{name}, $elements );
>+}
>+
>+sub _unit_info { # Converting old-style unit like '3m' to ( 3, 'minutes' ) etc.
>+    my ( $unit ) = @_;
>+    if( $unit && $unit =~ /(\d+)([mhd])/ ) {
>+        return ( $1, ABBREV->{$2} );
>+    }
>+    return;
> }
> 
> =head2 exceeds_limit
>@@ -92,8 +126,7 @@ sub exceeds_limit {
> 
>     if( $incr ) {
>         $domain_limits->{$group}->{count}++;
>-        warn "Sending messages: domain $group reached limit of ".
>-          $domain_limits->{$group}->{limit}. '/'. $domain_limits->{$group}->{unit}
>+        warn "Sending messages: domain $group reached limit of ". $domain_limits->{$group}->{limit}
>             if $domain_limits->{$group}->{count} == $domain_limits->{$group}->{limit};
>     }
>     return 0;
>@@ -117,7 +150,7 @@ sub _get_domain_count {
> 
>     my $sum = 0;
>     my $dt_parser = Koha::Database->new->schema->storage->datetime_parser;
>-    my $start_dt = _convert_unit( undef, $limits->{$group}->{unit} );
>+    my $start_dt = _calc_startdate( undef, $limits->{$group}->{units}, $limits->{$group}->{unit_type} );
>     foreach my $domain ( @domains ) {
>         $sum += Koha::Notice::Messages->search({
>             message_transport_type => 'email',
>@@ -129,14 +162,11 @@ sub _get_domain_count {
>     $limits->{$group}->{count} = $sum;
> }
> 
>-sub _convert_unit { # unit should be like \d+(m|h|d)
>-    my ( $dt, $unit ) = @_;
>+sub _calc_startdate {
>+    my ( $dt, $units, $unit_type ) = @_;
>     $dt //= dt_from_string();
>-    if( $unit && $unit =~ /(\d+)([mhd])/ ) {
>-        my $abbrev = { m => 'minutes', h => 'hours', d => 'days' };
>-        foreach my $h ( 0, 1 ) { # try hour before too when subtract fails (like: change to summertime)
>-            eval { $dt->subtract( hours => $h )->subtract( $abbrev->{$2} => $1 ) } and last;
>-        }
>+    foreach my $h ( 0, 1 ) { # try hour before too when subtract fails (like: change to summertime)
>+        eval { $dt->subtract( hours => $h )->subtract( $unit_type => $units ) } and last;
>     }
>     return $dt;
> }
>diff --git a/t/db_dependent/Koha/Notice_Util.t b/t/db_dependent/Koha/Notice_Util.t
>index 805d6313ad..7f216e44ea 100755
>--- a/t/db_dependent/Koha/Notice_Util.t
>+++ b/t/db_dependent/Koha/Notice_Util.t
>@@ -18,7 +18,6 @@
> # along with Koha; if not, see <http://www.gnu.org/licenses>.
> 
> use Modern::Perl;
>-#use Data::Dumper qw/Dumper/;
> use Test::More tests => 4;
> use Test::MockModule;
> use Test::Warn;
>@@ -26,6 +25,7 @@ use Test::Warn;
> use C4::Context;
> use Koha::Database;
> use Koha::DateUtils qw/dt_from_string/;
>+use Koha::MailDomainLimits;
> use Koha::Notice::Util;
> 
> use t::lib::TestBuilder;
>@@ -33,12 +33,13 @@ use t::lib::Mocks;
> 
> my $schema = Koha::Database->new->schema;
> $schema->storage->txn_begin;
>-
> my $builder = t::lib::TestBuilder->new;
> 
> subtest 'load_domain_limits' => sub {
>-    plan tests => 8;
>+    plan tests => 11;
> 
>+    # Starting testing without limits in database
>+    Koha::MailDomainLimits->delete;
>     my $domain_limits;
>     t::lib::Mocks::mock_config( 'message_domain_limits', undef );
>     is( Koha::Notice::Util->load_domain_limits, undef, 'koha-conf does not contain entry' );
>@@ -48,14 +49,30 @@ subtest 'load_domain_limits' => sub {
>     $domain_limits = Koha::Notice::Util->load_domain_limits;
>     is( keys %$domain_limits, 1, 'koha-conf contains one domain' );
>     is( $domain_limits->{a}->{limit}, 2, 'check limit of first entry' );
>-    is( $domain_limits->{a}->{unit}, '1d', 'check unit of first entry' );
>-    t::lib::Mocks::mock_config( 'message_domain_limits',
>-        { domain => [ { name => 'A', limit => 2, unit => '2d' }, { name => 'B', limit => 3, unit => '3h' } ] },
>+    is( $domain_limits->{a}->{units}, 1, 'check unit of first entry' );
>+    is( $domain_limits->{a}->{unit_type}, 'days', 'check unit of first entry' );
>+    t::lib::Mocks::mock_config(
>+        'message_domain_limits',
>+        {
>+            domain => [
>+                { name => 'A', limit      => 2, unit => '2d' },
>+                { name => 'B', limit      => 3, unit => '3h' },
>+                { name => 'C', belongs_to => 'B' },
>+            ],
>+        }
>     );
>     $domain_limits = Koha::Notice::Util->load_domain_limits;
>-    is( keys %$domain_limits, 2, 'koha-conf contains two domains' );
>+    is( keys %$domain_limits, 3, 'koha-conf contains two domains' );
>     is( $domain_limits->{b}->{limit}, 3, 'check limit of second entry' );
>     is( $domain_limits->{b}->{count}, undef, 'check if count still undefined' );
>+    is( $domain_limits->{c}->{belongs_to}, 'b', 'check group domain of C' );
>+
>+    # Test with database
>+    my $limit01 = Koha::MailDomainLimit->new({ domain_name => 'A', domain_limit => 2, units => '2', unit_type => 'days' })->store;
>+    my $limit02 = Koha::MailDomainLimit->new({ domain_name => 'B', domain_limit => 3, units => '3', unit_type => 'hours' })->store;
>+    my $limit03 = Koha::MailDomainLimit->new({ domain_name => 'C', belongs_to => $limit02->id })->store;
>+    my $domain_limits_db = Koha::Notice::Util->load_domain_limits;
>+    is_deeply( $domain_limits_db, $domain_limits, 'Same results for limits in database' );
> };
> 
> subtest 'counting in exceeds_limit' => sub {
>@@ -73,12 +90,10 @@ subtest 'counting in exceeds_limit' => sub {
>     $builder->build_object({ class => 'Koha::Notice::Messages',
>         value => { @values, to_address => 'c@A', updated_on => $today->clone->subtract( days => 3 ) }});
> 
>-    $domain_limits = Koha::Notice::Util->load_domain_limits; # still using last mocked config A:2/2d
>+    $domain_limits = Koha::Notice::Util->load_domain_limits; # still using last config for A: 2/2d
>     Koha::Notice::Util->exceeds_limit({ to => '@A', limits => $domain_limits, incr => 0 }); # force counting
>     is( $domain_limits->{a}->{count}, 1, '1 message to A within unit of 2d' );
>-    t::lib::Mocks::mock_config( 'message_domain_limits',
>-        { domain => [ { name => 'A', limit => 2, unit => '50h' }, { name => 'B', limit => 3, unit => '3h' } ] },
>-    );
>+    Koha::MailDomainLimits->search({ domain_name => 'A' })->next->set({ domain_limit => 2, units => '50', unit_type => 'hours' })->store;
>     $domain_limits = Koha::Notice::Util->load_domain_limits;
>     Koha::Notice::Util->exceeds_limit({ to => 'x@A ', limits => $domain_limits, incr => 0 }); # force counting
>     is( $domain_limits->{a}->{count}, 2, '2 messages to A within unit of 50h' );
>@@ -86,17 +101,15 @@ subtest 'counting in exceeds_limit' => sub {
>     ok( !defined $domain_limits->{b}->{count}, 'Prove that we did not count b if not asked for' );
> };
> 
>-subtest '_convert_unit' => sub {
>-    plan tests => 3;
>+subtest '_calc_startdate' => sub {
>+    plan tests => 2;
> 
>     # Date subtraction - edge case (start of summer time)
>     my $mock_context = Test::MockModule->new('C4::Context');
>     $mock_context->mock( 'tz', sub { return DateTime::TimeZone->new( name => 'Europe/Amsterdam' )} );
>     my $dt = dt_from_string( '2023-03-31 02:30:00', 'iso', '+02:00' );
>-    is( Koha::Notice::Util::_convert_unit( $dt, '4d')->stringify, '2023-03-27T02:30:00', '02:30 is fine' );
>-    is( Koha::Notice::Util::_convert_unit( $dt, '1d')->stringify, '2023-03-26T01:30:00', 'Moved 02:30 to 01:30' );
>-    # Test bad unit
>-    is( Koha::Notice::Util::_convert_unit( $dt, 'y')->stringify, '2023-03-26T01:30:00', 'No further shift for bad unit' );
>+    is( Koha::Notice::Util::_calc_startdate( $dt, 4, 'days')->stringify, '2023-03-27T02:30:00', '02:30 is fine' );
>+    is( Koha::Notice::Util::_calc_startdate( $dt, 1, 'days')->stringify, '2023-03-26T01:30:00', 'Moved 02:30 to 01:30' );
>     $mock_context->unmock('tz');
> };
> 
>@@ -104,15 +117,14 @@ subtest 'exceeds_limit with group domains' => sub {
>     plan tests => 12;
> 
>     my $domain_limits;
>+    Koha::MailDomainLimits->delete;
>     t::lib::Mocks::mock_config( 'message_domain_limits', undef );
>     $domain_limits = Koha::Notice::Util->load_domain_limits;
>     is( Koha::Notice::Util->exceeds_limit({ to => 'marcel@koha.nl', limits => $domain_limits }), 0, 'False when having no limits' );
> 
>-    t::lib::Mocks::mock_config( 'message_domain_limits', { domain => [
>-        { name => 'A', limit => 3, unit => '5m' },
>-        { name => 'B', limit => 2, unit => '5m' },
>-        { name => 'C', belongs_to => 'A' },
>-    ]});
>+    my $limit01 = Koha::MailDomainLimit->new({ domain_name => 'A', domain_limit => 3, units => 5, unit_type => 'minutes' })->store;
>+    my $limit02 = Koha::MailDomainLimit->new({ domain_name => 'B', domain_limit => 2, units => 5, unit_type => 'minutes' })->store;
>+    my $limit03 = Koha::MailDomainLimit->new({ domain_name => 'C', belongs_to => $limit01->id })->store;
>     $domain_limits = Koha::Notice::Util->load_domain_limits;
>     my $result;
>     is( Koha::Notice::Util->exceeds_limit({ to => '1@A', limits => $domain_limits }), 0, 'First message to A' );
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 33537: 153351 | 153352 | 153353 | 153354 | 153355 | 153356 | 153357 | 153358 | 153359 | 153361 | 153362 | 153363 | 153364 | 154769 | 154770 | 154771 | 154772 | 154773 | 154774 | 154775 | 154776 | 154777 | 154778 | 154779 | 154818 | 154819 | 154820 | 154821 | 154822 | 154823 | 154824 | 154825 | 154826 | 154827 | 154828 | 154829 | 154830 | 154831 | 154832 | 154833 | 154869 | 154870 | 154871 | 154872 | 154873 | 157017 | 157018 | 157019 | 157020 | 157021 | 157022 | 157023 | 157024 | 157025 | 157026 | 157027 | 157028 | 157029 | 157030 | 157031 | 157032 | 157038 | 157039 | 157040 | 157041 | 157042 | 157043 | 157044 | 157045 | 157046 | 157047 | 157048