From cc1d796b730cd1316611e04c6e64239aa493c86b Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Jan 2014 08:46:01 -0500 Subject: [PATCH] Add email send error checking. Add manual transaction handling. Remove grandfather option --- .../management/commands/linkedin_mailusers.py | 186 +++++++++++++----- .../commands/tests/test_mailusers.py | 66 +------ 2 files changed, 143 insertions(+), 109 deletions(-) diff --git a/lms/djangoapps/linkedin/management/commands/linkedin_mailusers.py b/lms/djangoapps/linkedin/management/commands/linkedin_mailusers.py index 75f097cca2..3e1935dfe2 100644 --- a/lms/djangoapps/linkedin/management/commands/linkedin_mailusers.py +++ b/lms/djangoapps/linkedin/management/commands/linkedin_mailusers.py @@ -3,12 +3,27 @@ Send emails to users inviting them to add their course certificates to their LinkedIn profiles. """ +from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError, SMTPException import json +import logging import urllib +from boto.exception import AWSConnectionError +from boto.ses.exceptions import ( + SESAddressNotVerifiedError, + SESIdentityNotVerifiedError, + SESDomainNotConfirmedError, + SESAddressBlacklistedError, + SESDailyQuotaExceededError, + SESMaxSendingRateExceededError, + SESDomainEndsWithDotError, + SESLocalAddressCharacterError, + SESIllegalAddressError, +) from django.conf import settings from django.core.mail import EmailMessage from django.core.management.base import BaseCommand +from django.db import transaction from django.template import Context from django.template.loader import get_template from django.core.urlresolvers import reverse @@ -21,6 +36,53 @@ from courseware.courses import get_course_by_id, course_image_url from ...models import LinkedIn +# The following is blatantly cribbed from bulk_email/tasks.py + +# 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. +) + +# Exceptions that, if caught, should cause the task to be re-tried. +# These errors will be caught a limited number of times before the task fails. +LIMITED_RETRY_ERRORS = ( + SMTPConnectError, + SMTPServerDisconnected, + AWSConnectionError, +) + +# Errors that indicate that a mailing task should be retried without limit. +# An example is if email is being sent too quickly, but may succeed if sent +# more slowly. When caught by a task, it triggers an exponential backoff and retry. +# Retries happen continuously until the email is sent. +# Note that the SMTPDataErrors here are only those within the 4xx range. +# 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, +) + +# 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, +) + + + +MAX_ATTEMPTS = 10 + +log = logging.getLogger("linkedin") class Command(BaseCommand): """ @@ -32,18 +94,6 @@ class Command(BaseCommand): 'course certificates, inviting them to add their certificates to ' 'their LinkedIn profiles') option_list = BaseCommand.option_list + ( - make_option( - '--grandfather', - action='store_true', - dest='grandfather', - default=False, - help="Creates aggregate invitations for all certificates a user " - "has earned to date and sends a 'grandfather' email. This is " - "intended to be used when the feature is launched to invite " - "all users that have earned certificates to date to add their " - "certificates. Afterwards the default, one email per " - "certificate mail form will be used."),) - option_list = option_list + ( make_option( '--mock', action='store_true', @@ -54,36 +104,56 @@ class Command(BaseCommand): def __init__(self): super(Command, self).__init__() + @transaction.commit_manually def handle(self, *args, **options): whitelist = settings.LINKEDIN_API['EMAIL_WHITELIST'] - grandfather = options.get('grandfather', False) mock_run = options.get('mock_run', False) accounts = LinkedIn.objects.filter(has_linkedin_account=True) + for account in accounts: user = account.user if whitelist and user.email not in whitelist: # Whitelist only certain addresses for testing purposes continue - emailed = json.loads(account.emailed_courses) + + try: + emailed = json.loads(account.emailed_courses) + except Exception: + log.exception("LinkedIn: Could not parse emailed_courses for {}".format(user.username)) + continue + certificates = GeneratedCertificate.objects.filter(user=user) certificates = certificates.filter(status='downloadable') - certificates = [cert for cert in certificates - if cert.course_id not in emailed] + certificates = [cert for cert in certificates if cert.course_id not in emailed] + # Shouldn't happen, since we're only picking users who have + # certificates, but just in case... if not certificates: + log.info("LinkedIn: No certificates for user {}".format(user.username)) continue - if grandfather: - self.send_grandfather_email(user, certificates, mock_run) + + # Basic sanity checks passed, now try to send the emails + try: + success = False + success = self.send_grandfather_email(user, certificates, mock_run) + log.info("LinkedIn: Sent email for user {}".format(user.username)) if not mock_run: emailed.extend([cert.course_id for cert in certificates]) - else: - for certificate in certificates: - self.send_triggered_email(user, certificate) - emailed.append(certificate.course_id) - account.emailed_courses = json.dumps(emailed) - account.save() + if success and not mock_run: + account.emailed_courses = json.dumps(emailed) + account.save() + transaction.commit() + except BULK_EMAIL_FAILURE_ERRORS: + log.exception("LinkedIn: No further email sending will work, aborting") + transaction.commit() + return -1 + except Exception: + log.exception("LinkedIn: User {} couldn't be processed".format(user.username)) - def certificate_url(self, certificate, grandfather=False): + transaction.commit() + + + def certificate_url(self, certificate): """ Generates a certificate URL based on LinkedIn's documentation. The documentation is from a Word document: DAT_DOCUMENTATION_v3.12.docx @@ -94,7 +164,7 @@ class Command(BaseCommand): 'prof', # the 'product'--no idea what that's supposed to mean 'edX', # Partner's name course.number, # Certificate's name - 'gf' if grandfather else 'T']) + 'gf']) query = [ ('pfCertificationName', course.display_name_with_default), ('pfAuthorityName', settings.PLATFORM_NAME), @@ -133,36 +203,58 @@ class Command(BaseCommand): 'course_title': course_title, 'course_image_url': course_img_url, 'course_end_date': course_end_date, - 'linkedin_add_url': self.certificate_url(cert, True), + 'linkedin_add_url': self.certificate_url(cert), }) context = {'courses_list': courses_list, 'num_courses': len(courses_list)} body = render_to_string('linkedin/linkedin_email.html', context) subject = '{}, Add your Achievements to your LinkedIn Profile'.format(user.profile.name) - if not mock_run: - self.send_email(user, subject, body) + if mock_run: + return True + else: + return self.send_email(user, subject, body) - def send_triggered_email(self, user, certificate): + def send_email(self, user, subject, body, num_attempts=MAX_ATTEMPTS): """ - Email a user that recently earned a certificate, inviting them to post - their certificate on their LinkedIn profile. - """ - template = get_template("linkedin_email.html") - url = self.certificate_url(certificate) - context = Context({ - 'student_name': user.profile.name, - 'course_name': certificate.name, - 'url': url}) - body = template.render(context) - subject = 'Congratulations! Put your certificate on LinkedIn' - self.send_email(user, subject, body) - - def send_email(self, user, subject, body): - """ - Send an email. + Send an email. Return True if it succeeded, False if it didn't. """ fromaddr = settings.DEFAULT_FROM_EMAIL toaddr = '%s <%s>' % (user.profile.name, user.email) msg = EmailMessage(subject, body, fromaddr, (toaddr,)) msg.content_subtype = "html" - msg.send() + + i = 0 + while i < num_attempts: + try: + msg.send() + return True # Happy path! + except SINGLE_EMAIL_FAILURE_ERRORS: + # Something unrecoverable is wrong about the email acct we're sending to + log.exception( + "LinkedIn: Email send failed for user {}, email {}" + .format(user.username, user.email) + ) + return False + except LIMITED_RETRY_ERRORS: + # Something went wrong (probably an intermittent connection error), + # but maybe if we beat our heads against the wall enough times, + # we can crack our way through. Thwack! Thwack! Thwack! + # Give up after num_attempts though (for loop exits), let's not + # get carried away. + log.exception( + u"LinkedIn: Email send for user {}, email {}, encountered error, attempt #{}" + .format(user.username, user.email, i) + ) + i += 1 + continue + except INFINITE_RETRY_ERRORS: + # Dude, it will *totally* work if I just... sleep... a little... + # Things like max send rate exceeded. The smart thing would be + # to do exponential backoff. The lazy thing to do would be just + # sleep some arbitrary amount and trust that it'll probably work. + # GUESS WHAT WE'RE DOING BOYS AND GIRLS!?! + log.exception("LinkedIn: temporary error encountered, retrying") + time.sleep(1) + + # If we hit here, we went through all our attempts without success + return False diff --git a/lms/djangoapps/linkedin/management/commands/tests/test_mailusers.py b/lms/djangoapps/linkedin/management/commands/tests/test_mailusers.py index c6dc66c56d..942d6d9fb3 100644 --- a/lms/djangoapps/linkedin/management/commands/tests/test_mailusers.py +++ b/lms/djangoapps/linkedin/management/commands/tests/test_mailusers.py @@ -69,24 +69,6 @@ class MailusersTests(TestCase): course_id='TESTX/3/TEST3') cert3.save() - def test_mail_users(self): - """ - Test emailing users. - """ - fut = mailusers.Command().handle - fut() - self.assertEqual( - json.loads(self.fred.linkedin.emailed_courses), ['TESTX/1/TEST1', 'TESTX/2/TEST2']) - self.assertEqual( - json.loads(self.barney.linkedin.emailed_courses), ['TESTX/3/TEST3']) - self.assertEqual(len(mail.outbox), 3) - self.assertEqual(mail.outbox[0].from_email, settings.DEFAULT_FROM_EMAIL) - self.assertEqual( - mail.outbox[0].to, ['Fred Flintstone ']) - self.assertEqual( - mail.outbox[1].to, ['Fred Flintstone ']) - self.assertEqual( - mail.outbox[2].to, ['Barney Rubble ']) @mock.patch.dict('django.conf.settings.LINKEDIN_API', {'EMAIL_WHITELIST': ['barney@bedrock.gov']}) @@ -107,7 +89,7 @@ class MailusersTests(TestCase): Test sending grandfather emails. """ fut = mailusers.Command().handle - fut(grandfather=True) + fut() self.assertEqual( json.loads(self.fred.linkedin.emailed_courses), ['TESTX/1/TEST1', 'TESTX/2/TEST2']) self.assertEqual( @@ -127,53 +109,13 @@ class MailusersTests(TestCase): test that we aren't sending anything when in mock_run mode """ fut = mailusers.Command().handle - fut(grandfather=True, mock_run=True) + fut(mock_run=True) self.assertEqual( json.loads(self.fred.linkedin.emailed_courses), []) self.assertEqual( json.loads(self.barney.linkedin.emailed_courses), []) self.assertEqual(len(mail.outbox), 0) - def test_mail_users_only_new_courses(self): - """ - Test emailing users, making sure they are only emailed about new - certificates. - """ - self.fred.linkedin.emailed_courses = json.dumps(['TESTX/1/TEST1']) - self.fred.linkedin.save() - fut = mailusers.Command().handle - fut() - fred = User.objects.get(username='fred') - self.assertEqual( - json.loads(fred.linkedin.emailed_courses), ['TESTX/1/TEST1', 'TESTX/2/TEST2']) - self.assertEqual( - json.loads(self.barney.linkedin.emailed_courses), ['TESTX/3/TEST3']) - self.assertEqual(len(mail.outbox), 2) - self.assertEqual( - mail.outbox[0].to, ['Fred Flintstone ']) - self.assertEqual( - mail.outbox[1].to, ['Barney Rubble ']) - - def test_mail_users_barney_has_no_new_courses(self): - """ - Test emailing users, making sure they are only emailed about new - certificates. - """ - self.barney.linkedin.emailed_courses = json.dumps(['TESTX/3/TEST3']) - self.barney.linkedin.save() - fut = mailusers.Command().handle - fut() - fred = User.objects.get(username='fred') - self.assertEqual( - json.loads(fred.linkedin.emailed_courses), ['TESTX/1/TEST1', 'TESTX/2/TEST2']) - self.assertEqual( - json.loads(self.barney.linkedin.emailed_courses), ['TESTX/3/TEST3']) - self.assertEqual(len(mail.outbox), 2) - self.assertEqual( - mail.outbox[0].to, ['Fred Flintstone ']) - self.assertEqual( - mail.outbox[1].to, ['Fred Flintstone ']) - def test_certificate_url(self): self.cert1.created_date = datetime.datetime( 2010, 8, 15, 0, 0, tzinfo=utc) @@ -182,8 +124,8 @@ class MailusersTests(TestCase): self.assertEqual( fut(self.cert1), 'http://www.linkedin.com/profile/guided?' - 'pfCertificationName=TestX%2FIntro101&pfAuthorityName=edX&' + 'pfCertificationName=TEST1&pfAuthorityName=edX&' 'pfAuthorityId=0000000&' 'pfCertificationUrl=http%3A%2F%2Ftest.foo%2Ftest&pfLicenseNo=TESTX%2F1%2FTEST1&' 'pfCertStartDate=201005&_mSplash=1&' - 'trk=eml-prof-TESTX-1-T&startTask=CERTIFICATION_NAME&force=true') + 'trk=eml-prof-edX-1-gf&startTask=CERTIFICATION_NAME&force=true')