From 0d73926b8845dd8a12f78e6a41bd39d0b5c39cf9 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy 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 Signed-off-by: Pedro Amorim --- 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 60ffa3dbdb..436bf3e9a8 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; @@ -116,7 +149,7 @@ sub _get_domain_count { } keys %$limits; my $sum = 0; - 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( { @@ -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 . 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