diff --git a/lms/djangoapps/mailing/management/commands/mailchimp_id.py b/lms/djangoapps/mailing/management/commands/mailchimp_id.py index 7159b9478f..922d10b80d 100644 --- a/lms/djangoapps/mailing/management/commands/mailchimp_id.py +++ b/lms/djangoapps/mailing/management/commands/mailchimp_id.py @@ -25,6 +25,7 @@ class Command(BaseCommand): ) def parse_options(self, options): + """Parses `options` of the command.""" if not options['key']: raise CommandError('missing key') diff --git a/lms/djangoapps/mailing/management/commands/mailchimp_sync_announcements.py b/lms/djangoapps/mailing/management/commands/mailchimp_sync_announcements.py index e160abfce9..718f25b34d 100644 --- a/lms/djangoapps/mailing/management/commands/mailchimp_sync_announcements.py +++ b/lms/djangoapps/mailing/management/commands/mailchimp_sync_announcements.py @@ -31,6 +31,7 @@ class Command(BaseCommand): ) def parse_options(self, options): + """Parses `options` of the command.""" if not options['key']: raise CommandError('missing key') @@ -62,8 +63,7 @@ def get_enrolled(): """ Filter out all users who signed up via a Microsite, which UserSignupSource tracks """ - ## TODO (Feanil) This grabs all INactive students and MUST be changed - ## TODO (Feanil) blame cdodge for your troubles + ## TODO (Feanil) This grabs all inactive students and MUST be changed (or, could exclude inactive users in get_data) return User.objects.raw('SELECT * FROM auth_user where id not in (SELECT user_id from student_usersignupsource)') diff --git a/lms/djangoapps/mailing/management/commands/mailchimp_sync_course.py b/lms/djangoapps/mailing/management/commands/mailchimp_sync_course.py index ac1e4b04f1..3a1919fbbe 100644 --- a/lms/djangoapps/mailing/management/commands/mailchimp_sync_course.py +++ b/lms/djangoapps/mailing/management/commands/mailchimp_sync_course.py @@ -9,7 +9,6 @@ from itertools import chain from optparse import make_option from collections import namedtuple - from django.core.management.base import BaseCommand, CommandError from mailsnake import MailSnake @@ -48,6 +47,7 @@ class Command(BaseCommand): ) def parse_options(self, options): + """Parses `options` of the command.""" if not options['key']: raise CommandError('missing key') @@ -131,29 +131,50 @@ def verify_list(mailchimp, list_id, course_id): def get_student_data(students, exclude=None): + """ + Given a QuerySet of Django users, extracts id, username, and is_anonymous data. + Excludes any users provided in the optional `exclude` set. + + Returns a list of dictionaries for each user, where the dictionary has keys + 'EMAIL', 'FULLNAME', and 'EDX_ID'. + """ # To speed the query, we won't retrieve the full User object, only # two of its values. The namedtuple simulates the User object. - FakeUser = namedtuple('Fake', 'id username is_anonymous') + FakeUser = namedtuple('Fake', 'id username is_anonymous') # pylint: disable=invalid-name exclude = exclude if exclude else set() - def make(v): - e = {'EMAIL': v['user__email'], - 'FULLNAME': v['name'].title()} + def make(svalue): + """ + Given a User value entry `svalue`, extracts the student's email and fullname, + and provides a unique id for the user. - fake_user = FakeUser(v['user_id'], v['user__username'], lambda: True) - e['EDX_ID'] = unique_id_for_user(fake_user) + Returns a dictionary with keys 'EMAIL', 'FULLNAME', and 'EDX_ID'. + """ + fake_user = FakeUser(svalue['user_id'], svalue['user__username'], lambda: True) - return e + entry = { + 'EMAIL': svalue['user__email'], + 'FULLNAME': svalue['name'].title(), + 'EDX_ID': unique_id_for_user(fake_user) + } + + return entry fields = 'user__email', 'name', 'user_id', 'user__username' values = students.values(*fields) + # TODO: Since `students` is a QuerySet, can we chain a filter here that would be more + # performant than calling a lambda for every user? exclude_func = lambda s: s['user__email'] in exclude return [make(s) for s in values if not exclude_func(s)] def get_enrolled_students(course_id): + """ + Given a course_id, returns a QuerySet of all the active students + in the course. + """ objects = UserProfile.objects course_key = CourseKey.from_string(course_id) students = objects.filter(user__courseenrollment__course_id=course_key, @@ -162,18 +183,32 @@ def get_enrolled_students(course_id): def get_subscribed(mailchimp, list_id): + """Returns a set of email addresses subscribed to `list_id`""" return get_members(mailchimp, list_id, 'subscribed') def get_unsubscribed(mailchimp, list_id): + """Returns a set of email addresses that have unsubscribed from `list_id`""" return get_members(mailchimp, list_id, 'unsubscribed') def get_cleaned(mailchimp, list_id): + """ + Returns a set of email addresses that have been cleaned from `list_id` + + These email addresses may be invalid or have caused bounces, so you don't want + to re-add them back to the list. + """ return get_members(mailchimp, list_id, 'cleaned') def get_members(mailchimp, list_id, status): + """ + Given a mailchimp list id and a user status to filter on, returns all + members of the mailchimp list with that status. + + Returns a set of email addresses. + """ mc_get_members = mailchimp.listMembers members = set() @@ -193,6 +228,10 @@ def get_members(mailchimp, list_id, status): def unsubscribe(mailchimp, list_id, emails): + """ + Batch unsubscribe the given email addresses from the list represented + by `list_id` + """ batch_unsubscribe = mailchimp.listBatchUnsubscribe result = batch_unsubscribe(id=list_id, emails=emails, @@ -202,6 +241,15 @@ def unsubscribe(mailchimp, list_id, emails): def update_merge_tags(mailchimp, list_id, tag_names): + """ + This function is rather inscrutable. Given tag_names, which + in this code seems to be a list of ['FULLNAME', 'EMAIL', 'EDX_ID'], + we grab tags from the mailchimp list, then we verify tag_names has + 'FULLNAME' and 'EMAIL' present, we get more data from mailchimp, then + sync the variables up to mailchimp using `listMergeVarAdd`. + + The purpose of this function is unclear. + """ mc_vars = mailchimp.listMergeVars(id=list_id) mc_names = set(v['name'] for v in mc_vars) @@ -213,6 +261,9 @@ def update_merge_tags(mailchimp, list_id, tag_names): tag = name_to_tag(name) # verify FULLNAME is present + # TODO: Why is this under the for loop? It does nothing with the loop + # variable and seems like things would work if this was executed before or + # after the loop. if 'FULLNAME' not in tags: result = mc_merge(id=list_id, tag='FULLNAME', @@ -235,6 +286,13 @@ def update_merge_tags(mailchimp, list_id, tag_names): def subscribe_with_data(mailchimp, list_id, user_data): + """ + Given user_data in the form of a list of dictionaries for each user, + where the dictionary has keys 'EMAIL', 'FULLNAME', and 'EDX_ID', batch + subscribe the users to the given `list_id` via a Mailchimp api method. + + Returns None + """ format_entry = lambda e: {name_to_tag(k): v for k, v in e.iteritems()} formated_data = list(format_entry(e) for e in user_data) @@ -244,23 +302,37 @@ def subscribe_with_data(mailchimp, list_id, user_data): batch=batch, double_optin=False, update_existing=True) - + log.debug( "Added: %s Error on: %s", result['add_count'], result['error_count'] ) def make_segments(mailchimp, list_id, count, emails): + """ + Segments the list of email addresses `emails` into `count` segments, + if count is nonzero. + + For unknown historical reasons, lost to the winds of time, this is done with + a random order to the email addresses. + + First, existing 'random_' mailchimp segments are deleted. + + Then, the list of emails (the whole, large list) is shuffled. + + Finally, the shuffled emails are chunked into `count` segments and re-uploaded + to mailchimp as 'random_'-prefixed segments. + """ if count > 0: # reset segments segments = mailchimp.listStaticSegments(id=list_id) - for s in segments: - if s['name'].startswith('random'): - mailchimp.listStaticSegmentDel(id=list_id, seg_id=s['id']) + for seg in segments: + if seg['name'].startswith('random'): + mailchimp.listStaticSegmentDel(id=list_id, seg_id=seg['id']) # shuffle and split emails emails = list(emails) - random.shuffle(emails) + random.shuffle(emails) # Why do we do this? chunk_size = int(math.ceil(float(len(emails)) / count)) chunks = list(chunk(emails, chunk_size)) @@ -278,7 +350,13 @@ def make_segments(mailchimp, list_id, count, emails): def name_to_tag(name): - return (name[:10] if len(name) > 10 else name).replace(' ', '_').strip() + """ + Returns sanitized str `name`: no more than 10 characters, + with spaces replaced with `_` + """ + if len(name) > 10: + name = name[:10] + return name.replace(' ', '_').strip() def chunk(elist, size):