From fcb4c4d09818a75fead154fa554ded699fd2db1e Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Thu, 25 May 2023 17:45:11 +0500 Subject: [PATCH] fix: replacing exceptions (#31827) * fix: fixing test. * fix: updating exceptions. --- lms/djangoapps/bulk_email/tasks.py | 96 +++++++++---------- lms/djangoapps/bulk_email/tests/test_tasks.py | 64 +++++-------- 2 files changed, 68 insertions(+), 92 deletions(-) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 4d86edcc10..afad888fe0 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -10,21 +10,10 @@ import re import time from collections import Counter from datetime import datetime -from smtplib import SMTPConnectError, SMTPDataError, SMTPException, SMTPServerDisconnected, SMTPSenderRefused +from smtplib import SMTPConnectError, SMTPDataError, SMTPException, SMTPSenderRefused, SMTPServerDisconnected from time import sleep -from boto.exception import AWSConnectionError -from boto.ses.exceptions import ( - SESAddressBlacklistedError, - SESAddressNotVerifiedError, - SESDailyQuotaExceededError, - SESDomainEndsWithDotError, - SESDomainNotConfirmedError, - SESIdentityNotVerifiedError, - SESIllegalAddressError, - SESLocalAddressCharacterError, - SESMaxSendingRateExceededError -) +from botocore.exceptions import ClientError, EndpointConnectionError from celery import current_task, shared_task from celery.exceptions import RetryTaskError from celery.states import FAILURE, RETRY, SUCCESS @@ -34,8 +23,8 @@ from django.core.mail import get_connection from django.core.mail.message import forbid_multi_line_headers from django.urls import reverse from django.utils import timezone -from django.utils.translation import override as override_language from django.utils.translation import gettext as _ +from django.utils.translation import override as override_language from edx_django_utils.monitoring import set_code_owner_attribute from markupsafe import escape @@ -43,15 +32,12 @@ from common.djangoapps.util.date_utils import get_default_time_display from common.djangoapps.util.string_utils import _has_non_ascii_characters from lms.djangoapps.branding.api import get_logo_url_for_email from lms.djangoapps.bulk_email.api import get_unsubscribed_link -from lms.djangoapps.bulk_email.messages import ( - DjangoEmail, - ACEEmail, -) +from lms.djangoapps.bulk_email.messages import ACEEmail, DjangoEmail +from lms.djangoapps.bulk_email.models import CourseEmail, Optout from lms.djangoapps.bulk_email.toggles import ( is_bulk_email_edx_ace_enabled, - is_email_use_course_id_from_for_bulk_enabled, + is_email_use_course_id_from_for_bulk_enabled ) -from lms.djangoapps.bulk_email.models import CourseEmail, Optout from lms.djangoapps.courseware.courses import get_course from lms.djangoapps.instructor_task.models import InstructorTask from lms.djangoapps.instructor_task.subtasks import ( @@ -66,13 +52,11 @@ from openedx.core.lib.courses import course_image_url log = logging.getLogger('edx.celery.task') + # Errors that an individual email is failing to be sent, and should just # be treated as a fail. SINGLE_EMAIL_FAILURE_ERRORS = ( - SESAddressBlacklistedError, # Recipient's email address has been temporarily blacklisted. - SESDomainEndsWithDotError, # Recipient's email address' domain ends with a period/dot. - SESIllegalAddressError, # Raised when an illegal address is encountered. - SESLocalAddressCharacterError, # An address contained a control or whitespace character. + ClientError ) # Exceptions that, if caught, should cause the task to be re-tried. @@ -80,7 +64,7 @@ SINGLE_EMAIL_FAILURE_ERRORS = ( LIMITED_RETRY_ERRORS = ( SMTPConnectError, SMTPServerDisconnected, - AWSConnectionError, + EndpointConnectionError, ) # Errors that indicate that a mailing task should be retried without limit. @@ -91,20 +75,17 @@ LIMITED_RETRY_ERRORS = ( # Those not in this range (i.e. in the 5xx range) are treated as hard failures # and thus like SINGLE_EMAIL_FAILURE_ERRORS. INFINITE_RETRY_ERRORS = ( - SESMaxSendingRateExceededError, # Your account's requests/second limit has been exceeded. SMTPDataError, SMTPSenderRefused, + ClientError ) # Errors that are known to indicate an inability to send any more emails, # and should therefore not be retried. For example, exceeding a quota for emails. # Also, any SMTP errors that are not explicitly enumerated above. BULK_EMAIL_FAILURE_ERRORS = ( - SESAddressNotVerifiedError, # Raised when a "Reply-To" address has not been validated in SES yet. - SESIdentityNotVerifiedError, # Raised when an identity has not been verified in SES yet. - SESDomainNotConfirmedError, # Raised when domain ownership is not confirmed for DKIM. - SESDailyQuotaExceededError, # 24-hour allotment of outbound email has been exceeded. - SMTPException, + ClientError, + SMTPException ) @@ -588,13 +569,16 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas except SINGLE_EMAIL_FAILURE_ERRORS as exc: # This will fall through and not retry the message. - total_recipients_failed += 1 - log.exception( - f"BulkEmail ==> Status: Failed(SINGLE_EMAIL_FAILURE_ERRORS), Task: {parent_task_id}, SubTask: " - f"{task_id}, EmailId: {email_id}, Recipient num: {recipient_num}/{total_recipients}, Recipient " - f"UserId: {current_recipient['pk']}" - ) - subtask_status.increment(failed=1) + if exc.response['Error']['Code'] in ['MessageRejected', 'MailFromDomainNotVerified', 'MailFromDomainNotVerifiedException', 'FromEmailAddressNotVerifiedException']: # lint-amnesty, pylint: disable=line-too-long + total_recipients_failed += 1 + log.exception( + f"BulkEmail ==> Status: Failed(SINGLE_EMAIL_FAILURE_ERRORS), Task: {parent_task_id}, SubTask: " + f"{task_id}, EmailId: {email_id}, Recipient num: {recipient_num}/{total_recipients}, Recipient " + f"UserId: {current_recipient['pk']}" + ) + subtask_status.increment(failed=1) + else: + raise exc else: total_recipients_successful += 1 @@ -631,10 +615,13 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas except INFINITE_RETRY_ERRORS as exc: # Increment the "retried_nomax" counter, update other counters with progress to date, # and set the state to RETRY: - subtask_status.increment(retried_nomax=1, state=RETRY) - return _submit_for_retry( - entry_id, email_id, to_list, global_email_context, exc, subtask_status, skip_retry_max=True - ) + if isinstance(exc, (SMTPDataError, SMTPSenderRefused)) or exc.response['Error']['Code'] in ['LimitExceededException']: # lint-amnesty, pylint: disable=line-too-long + subtask_status.increment(retried_nomax=1, state=RETRY) + return _submit_for_retry( + entry_id, email_id, to_list, global_email_context, exc, subtask_status, skip_retry_max=True + ) + else: + raise exc except LIMITED_RETRY_ERRORS as exc: # Errors caught here cause the email to be retried. The entire task is actually retried @@ -642,21 +629,28 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas # Errors caught are those that indicate a temporary condition that might succeed on retry. # Increment the "retried_withmax" counter, update other counters with progress to date, # and set the state to RETRY: + subtask_status.increment(retried_withmax=1, state=RETRY) return _submit_for_retry( entry_id, email_id, to_list, global_email_context, exc, subtask_status, skip_retry_max=False ) except BULK_EMAIL_FAILURE_ERRORS as exc: - num_pending = len(to_list) - log.exception( - f"Task {task_id}: email with id {email_id} caused send_course_email task to fail with 'fatal' exception. " - f"{num_pending} emails unsent." - ) - # Update counters with progress to date, counting unsent emails as failures, - # and set the state to FAILURE: - subtask_status.increment(failed=num_pending, state=FAILURE) - return subtask_status, exc + if isinstance(exc, SMTPException) or exc.response['Error']['Code'] in [ + 'AccountSendingPausedException', 'MailFromDomainNotVerifiedException', 'LimitExceededException' + ]: + num_pending = len(to_list) + log.exception( + f"Task {task_id}: email with id {email_id} caused send_course_email " + f"task to fail with 'fatal' exception. " + f"{num_pending} emails unsent." + ) + # Update counters with progress to date, counting unsent emails as failures, + # and set the state to FAILURE: + subtask_status.increment(failed=num_pending, state=FAILURE) + return subtask_status, exc + else: + raise exc except Exception as exc: # pylint: disable=broad-except # Errors caught here cause the email to be retried. The entire task is actually retried diff --git a/lms/djangoapps/bulk_email/tests/test_tasks.py b/lms/djangoapps/bulk_email/tests/test_tasks.py index e73db046aa..96c48e0d9e 100644 --- a/lms/djangoapps/bulk_email/tests/test_tasks.py +++ b/lms/djangoapps/bulk_email/tests/test_tasks.py @@ -7,27 +7,23 @@ paths actually work. """ -from datetime import datetime -from dateutil.relativedelta import relativedelta import json # lint-amnesty, pylint: disable=wrong-import-order +from datetime import datetime from itertools import chain, cycle, repeat # lint-amnesty, pylint: disable=wrong-import-order -from smtplib import SMTPAuthenticationError, SMTPConnectError, SMTPDataError, SMTPServerDisconnected, SMTPSenderRefused # lint-amnesty, pylint: disable=wrong-import-order +from smtplib import ( # lint-amnesty, pylint: disable=wrong-import-order + SMTPAuthenticationError, + SMTPConnectError, + SMTPDataError, + SMTPSenderRefused, + SMTPServerDisconnected +) from unittest.mock import Mock, patch # lint-amnesty, pylint: disable=wrong-import-order from uuid import uuid4 # lint-amnesty, pylint: disable=wrong-import-order + import pytest -from boto.exception import AWSConnectionError -from boto.ses.exceptions import ( - SESAddressBlacklistedError, - SESAddressNotVerifiedError, - SESDailyQuotaExceededError, - SESDomainEndsWithDotError, - SESDomainNotConfirmedError, - SESIdentityNotVerifiedError, - SESIllegalAddressError, - SESLocalAddressCharacterError, - SESMaxSendingRateExceededError -) +from botocore.exceptions import ClientError, EndpointConnectionError from celery.states import FAILURE, SUCCESS +from dateutil.relativedelta import relativedelta from django.conf import settings from django.core.management import call_command from django.test.utils import override_settings @@ -268,19 +264,22 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): def test_ses_blacklisted_user(self): # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESAddressBlacklistedError(554, "Email address is blacklisted")) + + operation_name = '' + parsed_response = {'Error': {'Code': 'MessageRejected', 'Message': 'Error Uploading'}} + self._test_email_address_failures(ClientError(parsed_response, operation_name)) def test_ses_illegal_address(self): # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESIllegalAddressError(554, "Email address is illegal")) - - def test_ses_local_address_character_error(self): - # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESLocalAddressCharacterError(554, "Email address contains a bad character")) + operation_name = '' + parsed_response = {'Error': {'Code': 'MailFromDomainNotVerifiedException', 'Message': 'Error Uploading'}} + self._test_email_address_failures(ClientError(parsed_response, operation_name)) def test_ses_domain_ends_with_dot(self): # Test that celery handles permanent SMTPDataErrors by failing and not retrying. - self._test_email_address_failures(SESDomainEndsWithDotError(554, "Email address ends with a dot")) + operation_name = '' + parsed_response = {'Error': {'Code': 'MailFromDomainNotVerifiedException', 'Message': 'invalid domain'}} + self._test_email_address_failures(ClientError(parsed_response, operation_name)) def test_bulk_email_skip_with_non_ascii_emails(self): """ @@ -367,12 +366,12 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): def test_retry_after_aws_connect_error(self): self._test_retry_after_limited_retry_error( - AWSConnectionError("Unable to provide secure connection through proxy") + EndpointConnectionError(endpoint_url="Could not connect to the endpoint URL:") ) def test_max_retry_after_aws_connect_error(self): self._test_max_retry_limit_causes_failure( - AWSConnectionError("Unable to provide secure connection through proxy") + EndpointConnectionError(endpoint_url="Could not connect to the endpoint URL:") ) def test_retry_after_general_error(self): @@ -416,11 +415,6 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): SMTPSenderRefused(421, "Throttling: Sending rate exceeded", self.instructor.email) ) - def test_retry_after_ses_throttling_error(self): - self._test_retry_after_unlimited_retry_error( - SESMaxSendingRateExceededError(455, "Throttling: Sending rate exceeded") - ) - def _test_immediate_failure(self, exception): """Test that celery can hit a maximum number of retries.""" # Doesn't really matter how many recipients, since we expect @@ -444,18 +438,6 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): def test_failure_on_unhandled_smtp(self): self._test_immediate_failure(SMTPAuthenticationError(403, "That password doesn't work!")) - def test_failure_on_ses_quota_exceeded(self): - self._test_immediate_failure(SESDailyQuotaExceededError(403, "You're done for the day!")) - - def test_failure_on_ses_address_not_verified(self): - self._test_immediate_failure(SESAddressNotVerifiedError(403, "Who *are* you?")) - - def test_failure_on_ses_identity_not_verified(self): - self._test_immediate_failure(SESIdentityNotVerifiedError(403, "May I please see an ID!")) - - def test_failure_on_ses_domain_not_confirmed(self): - self._test_immediate_failure(SESDomainNotConfirmedError(403, "You're out of bounds!")) - def test_bulk_emails_with_unicode_course_image_name(self): # Test bulk email with unicode characters in course image name course_image = '在淡水測試.jpg'