From 71136669e9da9cff859e0b5c239a3de9545eab20 Mon Sep 17 00:00:00 2001 From: Son Date: Mon, 7 Mar 2022 15:44:27 +0100 Subject: [PATCH] return the block reason in should_disable() --- app/email_utils.py | 44 ++++++++++++++++----------------------- email_handler.py | 7 +++++-- tests/test_email_utils.py | 20 +++++++++--------- 3 files changed, 33 insertions(+), 38 deletions(-) diff --git a/app/email_utils.py b/app/email_utils.py index 69957e40..d4799575 100644 --- a/app/email_utils.py +++ b/app/email_utils.py @@ -1118,15 +1118,17 @@ def normalize_reply_email(reply_email: str) -> str: return "".join(ret) -def should_disable(alias: Alias) -> bool: - """Disable an alias if it has too many bounces recently""" +def should_disable(alias: Alias) -> (bool, str): + """ + Return whether an alias should be disabled and if yes, the reason why + """ # Bypass the bounce rule if alias.cannot_be_disabled: LOG.w("%s cannot be disabled", alias) - return False + return False, "" if not ALIAS_AUTOMATIC_DISABLE: - return False + return False, "" yesterday = arrow.now().shift(days=-1) nb_bounced_last_24h = ( @@ -1141,8 +1143,7 @@ def should_disable(alias: Alias) -> bool: ) # if more than 12 bounces in 24h -> disable alias if nb_bounced_last_24h > 12: - LOG.d("more than 12 bounces in the last 24h, disable alias %s", alias) - return True + return True, "more than 12 bounces in the last 24h" # if more than 5 bounces but has bounces last week -> disable alias elif nb_bounced_last_24h > 5: @@ -1159,15 +1160,13 @@ def should_disable(alias: Alias) -> bool: .count() ) if nb_bounced_7d_1d > 1: - LOG.d( - "more than 5 bounces in the last 24h and more than 1 bounces in the last 7 days, " - "disable alias %s", - alias, + return ( + True, + "more than 5 bounces in the last 24h and more than 1 bounces in the last 7 days", ) - return True else: # alias level - # if bounces at least 9 days in the last 10 days -> disable alias + # if bounces happen for at least 9 days in the last 10 days -> disable alias query = ( Session.query( func.date(EmailLog.created_at).label("date"), @@ -1183,11 +1182,7 @@ def should_disable(alias: Alias) -> bool: ) if query.count() >= 9: - LOG.d( - "Bounces every day for at least 9 days in the last 10 days, disable alias %s", - alias, - ) - return True + return True, "Bounces every day for at least 9 days in the last 10 days" # account level query = ( @@ -1206,16 +1201,13 @@ def should_disable(alias: Alias) -> bool: # if an account has more than 10 bounces every day for at least 4 days in the last 10 days, disable alias date_bounces: List[Tuple[arrow.Arrow, int]] = list(query) - if len(date_bounces) > 4: - if all([v > 10 for _, v in date_bounces]): - LOG.d( - "+10 bounces for +4 days in the last 10 days on %s, disable alias %s", - alias.user, - alias, - ) - return True + more_than_10_bounces = [ + (d, nb_bounce) for d, nb_bounce in date_bounces if nb_bounce > 10 + ] + if len(more_than_10_bounces) > 4: + return True, "+10 bounces for +4 days in the last 10 days" - return False + return False, "" def parse_id_from_bounce(email_address: str) -> int: diff --git a/email_handler.py b/email_handler.py index 525a30f9..8dc9f4b9 100644 --- a/email_handler.py +++ b/email_handler.py @@ -1396,7 +1396,8 @@ def handle_bounce_forward_phase(msg: Message, email_log: EmailLog): refused_email_url = f"{URL}/dashboard/refused_email?highlight_id={email_log.id}" # inform user of this bounce - if not should_disable(alias): + alias_will_be_disabled, reason = should_disable(alias) + if not alias_will_be_disabled: LOG.d( "Inform user %s about a bounce from contact %s to alias %s", user, @@ -1447,7 +1448,9 @@ def handle_bounce_forward_phase(msg: Message, email_log: EmailLog): ignore_smtp_error=True, ) else: - LOG.w("Disable alias %s %s. Last contact %s", alias, user, contact) + LOG.w( + f"Disable alias {alias} because {reason}. {alias.mailboxes} {alias.user}. Last contact {contact}" + ) alias.enabled = False Session.commit() diff --git a/tests/test_email_utils.py b/tests/test_email_utils.py index 48216d5f..fe183164 100644 --- a/tests/test_email_utils.py +++ b/tests/test_email_utils.py @@ -610,7 +610,7 @@ def test_should_disable(flask_client): alias = Alias.create_new_random(user) Session.commit() - assert not should_disable(alias) + assert not should_disable(alias)[0] # create a lot of bounce on this alias contact = Contact.create( @@ -629,12 +629,12 @@ def test_should_disable(flask_client): bounced=True, ) - assert should_disable(alias) + assert should_disable(alias)[0] # should not affect another alias alias2 = Alias.create_new_random(user) Session.commit() - assert not should_disable(alias2) + assert not should_disable(alias2)[0] def test_should_disable_bounces_every_day(flask_client): @@ -643,7 +643,7 @@ def test_should_disable_bounces_every_day(flask_client): alias = Alias.create_new_random(user) Session.commit() - assert not should_disable(alias) + assert not should_disable(alias)[0] # create a lot of bounce on this alias contact = Contact.create( @@ -663,7 +663,7 @@ def test_should_disable_bounces_every_day(flask_client): created_at=arrow.now().shift(days=-i), ) - assert should_disable(alias) + assert should_disable(alias)[0] def test_should_disable_bounces_account(flask_client): @@ -682,8 +682,8 @@ def test_should_disable_bounces_account(flask_client): commit=True, ) - for day in range(6): - for _ in range(10): + for day in range(5): + for _ in range(11): EmailLog.create( user_id=user.id, contact_id=contact.id, @@ -694,7 +694,7 @@ def test_should_disable_bounces_account(flask_client): ) alias2 = Alias.create_new_random(user) - assert should_disable(alias2) + assert should_disable(alias2)[0] def test_should_disable_bounce_consecutive_days(flask_client): @@ -719,7 +719,7 @@ def test_should_disable_bounce_consecutive_days(flask_client): commit=True, bounced=True, ) - assert not should_disable(alias) + assert not should_disable(alias)[0] # create 2 bounces in the last 7 days: alias should be disabled for _ in range(2): @@ -731,7 +731,7 @@ def test_should_disable_bounce_consecutive_days(flask_client): bounced=True, created_at=arrow.now().shift(days=-3), ) - assert should_disable(alias) + assert should_disable(alias)[0] def test_parse_id_from_bounce():