From 31db37eaf9c79ec69d4c80415b46d61af2d8c98c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 Oct 2017 21:03:08 -0400 Subject: [PATCH 01/29] Move binning queries into resolvers.py to be closer to the Resolvers where they will live --- .../core/djangoapps/schedules/resolvers.py | 255 +++++++++++++++++- openedx/core/djangoapps/schedules/tasks.py | 247 +---------------- 2 files changed, 260 insertions(+), 242 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 681b1da83e..71cbd9490b 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -1,9 +1,22 @@ import datetime +from itertools import groupby +import logging + +from django.conf import settings +from django.contrib.auth.models import User +from django.contrib.staticfiles.templatetags.staticfiles import static +from django.core.urlresolvers import reverse +from django.db.models import F, Min, Q +from django.utils.formats import dateformat, get_format + from edx_ace.recipient_resolver import RecipientResolver from edx_ace.utils.date import serialize -from openedx.core.djangoapps.schedules.models import ScheduleConfig +from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid +from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG +from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist +from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig from openedx.core.djangoapps.schedules.tasks import ( DEFAULT_NUM_BINS, RECURRING_NUDGE_NUM_BINS, @@ -14,8 +27,19 @@ from openedx.core.djangoapps.schedules.tasks import ( course_update_schedule_bin, ) from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin +from openedx.core.djangoapps.schedules.template_context import ( + absolute_url, + encode_url, + encode_urls_in_dict, + get_base_template_context +) from openedx.core.djangoapps.site_configuration.models import SiteConfiguration +from request_cache.middleware import request_cached +from xmodule.modulestore.django import modulestore + + +LOG = logging.getLogger(__name__) class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): """ @@ -109,6 +133,118 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): self.log_prefix = 'Scheduled Nudge' +def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, + num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, + order_by='enrollment__user__id'): + """ + Returns Schedules with the target_date, related to Users whose id matches the bin_num, and filtered by org_list. + + Arguments: + schedule_date_field -- string field name to query on the User's Schedule model + current_datetime -- datetime that will be used as "right now" in the query + target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under + bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num + num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) + org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) + exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list + or strictly include (False) them (default: False) + order_by -- string for field to sort the resulting Schedules by + """ + target_day = _get_datetime_beginning_of_day(target_datetime) + schedule_day_equals_target_day_filter = { + 'courseenrollment__schedule__{}__gte'.format(schedule_date_field): target_day, + 'courseenrollment__schedule__{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), + } + users = User.objects.filter( + courseenrollment__is_active=True, + **schedule_day_equals_target_day_filter + ).annotate( + id_mod=F('id') % num_bins + ).filter( + id_mod=bin_num + ) + + schedule_day_equals_target_day_filter = { + '{}__gte'.format(schedule_date_field): target_day, + '{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), + } + schedules = Schedule.objects.select_related( + 'enrollment__user__profile', + 'enrollment__course', + ).prefetch_related( + 'enrollment__course__modes' + ).filter( + Q(enrollment__course__end__isnull=True) | Q( + enrollment__course__end__gte=current_datetime), + enrollment__user__in=users, + enrollment__is_active=True, + **schedule_day_equals_target_day_filter + ).order_by(order_by) + + if org_list is not None: + if exclude_orgs: + schedules = schedules.exclude(enrollment__course__org__in=org_list) + else: + schedules = schedules.filter(enrollment__course__org__in=org_list) + + if "read_replica" in settings.DATABASES: + schedules = schedules.using("read_replica") + + LOG.debug('Query = %r', schedules.query.sql_with_params()) + + with function_trace('schedule_query_set_evaluation'): + # This will run the query and cache all of the results in memory. + num_schedules = len(schedules) + + # This should give us a sense of the volume of data being processed by each task. + set_custom_metric('num_schedules', num_schedules) + + return schedules + + +def _recurring_nudge_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='start', + current_datetime=current_datetime, + target_datetime=target_datetime, + bin_num=bin_num, + num_bins=RECURRING_NUDGE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, + ) + + for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): + user_schedules = list(user_schedules) + course_id_strs = [str(schedule.enrollment.course_id) + for schedule in user_schedules] + + first_schedule = user_schedules[0] + template_context = get_base_template_context(site) + template_context.update({ + 'student_name': user.profile.name, + + 'course_name': first_schedule.enrollment.course.display_name, + 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), + + # This is used by the bulk email optout policy + 'course_ids': course_id_strs, + }) + + # Information for including upsell messaging in template. + _add_upsell_button_information_to_template_context( + user, first_schedule, template_context) + + yield (user, first_schedule.enrollment.course.language, template_context) + + +def _get_datetime_beginning_of_day(dt): + """ + Truncates hours, minutes, seconds, and microseconds to zero on given datetime. + """ + return dt.replace(hour=0, minute=0, second=0, microsecond=0) + + class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. @@ -122,6 +258,73 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): self.log_prefix = 'Upgrade Reminder' +def _upgrade_reminder_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='upgrade_deadline', + current_datetime=current_datetime, + target_datetime=target_datetime, + bin_num=bin_num, + num_bins=RECURRING_NUDGE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, + ) + + for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): + user_schedules = list(user_schedules) + course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] + + first_schedule = user_schedules[0] + template_context = get_base_template_context(self.site) + template_context.update({ + 'student_name': user.profile.name, + 'course_links': [ + { + 'url': absolute_url(self.site, reverse('course_root', args=[str(s.enrollment.course_id)])), + 'name': s.enrollment.course.display_name + } for s in user_schedules + ], + 'first_course_name': first_schedule.enrollment.course.display_name, + 'cert_image': absolute_url(self.site, static('course_experience/images/verified-cert.png')), + + # This is used by the bulk email optout policy + 'course_ids': course_id_strs, + }) + + _add_upsell_button_information_to_template_context(user, first_schedule, template_context) + + yield (user, first_schedule.enrollment.course.language, template_context) + + +def _add_upsell_button_information_to_template_context(user, schedule, template_context): + enrollment = schedule.enrollment + course = enrollment.course + + verified_upgrade_link = _get_link_to_purchase_verified_certificate( + user, schedule) + has_verified_upgrade_link = verified_upgrade_link is not None + + if has_verified_upgrade_link: + template_context['upsell_link'] = verified_upgrade_link + template_context['user_schedule_upgrade_deadline_time'] = dateformat.format( + enrollment.dynamic_upgrade_deadline, + get_format( + 'DATE_FORMAT', + lang=course.language, + use_l10n=True + ) + ) + + template_context['show_upsell'] = has_verified_upgrade_link + + +def _get_link_to_purchase_verified_certificate(a_user, a_schedule): + enrollment = a_schedule.enrollment + if enrollment.dynamic_upgrade_deadline is None or not verified_upgrade_link_is_valid(enrollment): + return None + + return verified_upgrade_deadline_link(a_user, enrollment.course) + + class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset`` and the @@ -134,3 +337,53 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): def __init__(self, *args, **kwargs): super(CourseUpdateResolver, self).__init__(*args, **kwargs) self.log_prefix = 'Course Update' + + +def _course_update_schedules_for_bin(site, current_datetime, target_datetime, day_offset, bin_num, org_list, + exclude_orgs=False): + week_num = abs(day_offset) / 7 + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='start', + current_datetime=current_datetime, + target_datetime=target_datetime, + bin_num=bin_num, + num_bins=COURSE_UPDATE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, + order_by='enrollment__course', + ) + + for schedule in schedules: + enrollment = schedule.enrollment + try: + week_summary = get_course_week_summary( + enrollment.course_id, week_num) + except CourseUpdateDoesNotExist: + continue + + user = enrollment.user + course_id_str = str(enrollment.course_id) + + template_context = get_base_template_context(site) + template_context.update({ + 'student_name': user.profile.name, + 'user_personal_address': user.profile.name if user.profile.name else user.username, + 'course_name': schedule.enrollment.course.display_name, + 'course_url': absolute_url(site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), + 'week_num': week_num, + 'week_summary': week_summary, + + # This is used by the bulk email optout policy + 'course_ids': [course_id_str], + }) + + yield (user, schedule.enrollment.course.language, template_context) + + +@request_cached +def get_course_week_summary(course_id, week_num): + if COURSE_UPDATE_WAFFLE_FLAG.is_enabled(course_id): + course = modulestore().get_course(course_id) + return course.week_summary(week_num) + else: + raise CourseUpdateDoesNotExist() diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index d878a3454d..3a7ced3841 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -1,15 +1,12 @@ import datetime -from itertools import groupby import logging from celery.task import task from django.conf import settings -from django.contrib.auth.models import User from django.contrib.sites.models import Site from django.contrib.staticfiles.templatetags.staticfiles import static from django.core.exceptions import ValidationError from django.core.urlresolvers import reverse -from django.db.models import F, Q from django.db.utils import DatabaseError from django.utils.formats import dateformat, get_format @@ -19,16 +16,12 @@ from edx_ace.recipient import Recipient from edx_ace.utils.date import deserialize from opaque_keys.edx.keys import CourseKey -from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid from openedx.core.djangoapps.monitoring_utils import set_custom_metric, function_trace -from request_cache.middleware import request_cached -from xmodule.modulestore.django import modulestore -from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG -from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist -from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig -from openedx.core.djangoapps.schedules.template_context import absolute_url, get_base_template_context +from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType +from openedx.core.djangoapps.schedules import resolvers + LOG = logging.getLogger(__name__) @@ -95,7 +88,7 @@ def recurring_nudge_schedule_bin( _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - for (user, language, context) in _recurring_nudge_schedules_for_bin( + for (user, language, context) in resolvers._recurring_nudge_schedules_for_bin( site, current_datetime, target_datetime, @@ -131,40 +124,6 @@ def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_of set_custom_metric('send_uuid', message_type.uuid) -def _recurring_nudge_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): - - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='start', - current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, - num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, - ) - - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] - - first_schedule = user_schedules[0] - template_context = get_base_template_context(site) - template_context.update({ - 'student_name': user.profile.name, - - 'course_name': first_schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), - - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, - }) - - # Information for including upsell messaging in template. - _add_upsell_button_information_to_template_context(user, first_schedule, template_context) - - yield (user, first_schedule.enrollment.course.language, template_context) - - class UpgradeReminder(ScheduleMessageType): pass @@ -181,7 +140,7 @@ def upgrade_reminder_schedule_bin( _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - for (user, language, context) in _upgrade_reminder_schedules_for_bin( + for (user, language, context) in resolvers._upgrade_reminder_schedules_for_bin( site, current_datetime, target_datetime, @@ -211,47 +170,6 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -def _upgrade_reminder_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='upgrade_deadline', - current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, - num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, - ) - - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] - - first_schedule = user_schedules[0] - template_context = get_base_template_context(site) - template_context.update({ - 'student_name': user.profile.name, - 'user_personal_address': user.profile.name if user.profile.name else user.username, - - 'course_links': [ - { - 'url': absolute_url(site, reverse('course_root', args=[str(s.enrollment.course_id)])), - 'name': s.enrollment.course.display_name - } for s in user_schedules - ], - - 'first_course_name': first_schedule.enrollment.course.display_name, - - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, - - 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), - }) - - _add_upsell_button_information_to_template_context(user, first_schedule, template_context) - - yield (user, first_schedule.enrollment.course.language, template_context) - - class CourseUpdate(ScheduleMessageType): pass @@ -268,7 +186,7 @@ def course_update_schedule_bin( _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - for (user, language, context) in _course_update_schedules_for_bin( + for (user, language, context) in resolvers._course_update_schedules_for_bin( site, current_datetime, target_datetime, @@ -297,156 +215,3 @@ def _course_update_schedule_send(site_id, msg_str): msg = Message.from_string(msg_str) ace.send(msg) - - -def _course_update_schedules_for_bin(site, current_datetime, target_datetime, day_offset, bin_num, org_list, - exclude_orgs=False): - week_num = abs(day_offset) / 7 - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='start', - current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, - num_bins=COURSE_UPDATE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, - order_by='enrollment__course', - ) - - for schedule in schedules: - enrollment = schedule.enrollment - try: - week_summary = get_course_week_summary(enrollment.course_id, week_num) - except CourseUpdateDoesNotExist: - continue - - user = enrollment.user - course_id_str = str(enrollment.course_id) - - template_context = get_base_template_context(site) - template_context.update({ - 'student_name': user.profile.name, - 'user_personal_address': user.profile.name if user.profile.name else user.username, - 'course_name': schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), - 'week_num': week_num, - 'week_summary': week_summary, - - # This is used by the bulk email optout policy - 'course_ids': [course_id_str], - }) - - yield (user, schedule.enrollment.course.language, template_context) - - -@request_cached -def get_course_week_summary(course_id, week_num): - if COURSE_UPDATE_WAFFLE_FLAG.is_enabled(course_id): - course = modulestore().get_course(course_id) - return course.week_summary(week_num) - else: - raise CourseUpdateDoesNotExist() - - -def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, - num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, - order_by='enrollment__user__id'): - """ - Returns Schedules with the target_date, related to Users whose id matches the bin_num, and filtered by org_list. - - Arguments: - schedule_date_field -- string field name to query on the User's Schedule model - current_datetime -- datetime that will be used as "right now" in the query - target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under - bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num - num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) - org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) - exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list - or strictly include (False) them (default: False) - order_by -- string for field to sort the resulting Schedules by - """ - target_day = _get_datetime_beginning_of_day(target_datetime) - schedule_day_equals_target_day_filter = { - 'courseenrollment__schedule__{}__gte'.format(schedule_date_field): target_day, - 'courseenrollment__schedule__{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), - } - users = User.objects.filter( - courseenrollment__is_active=True, - **schedule_day_equals_target_day_filter - ).annotate( - id_mod=F('id') % num_bins - ).filter( - id_mod=bin_num - ) - - schedule_day_equals_target_day_filter = { - '{}__gte'.format(schedule_date_field): target_day, - '{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), - } - schedules = Schedule.objects.select_related( - 'enrollment__user__profile', - 'enrollment__course', - ).prefetch_related( - 'enrollment__course__modes' - ).filter( - Q(enrollment__course__end__isnull=True) | Q(enrollment__course__end__gte=current_datetime), - enrollment__user__in=users, - enrollment__is_active=True, - **schedule_day_equals_target_day_filter - ).order_by(order_by) - - if org_list is not None: - if exclude_orgs: - schedules = schedules.exclude(enrollment__course__org__in=org_list) - else: - schedules = schedules.filter(enrollment__course__org__in=org_list) - - if "read_replica" in settings.DATABASES: - schedules = schedules.using("read_replica") - - LOG.debug('Query = %r', schedules.query.sql_with_params()) - - with function_trace('schedule_query_set_evaluation'): - # This will run the query and cache all of the results in memory. - num_schedules = len(schedules) - - # This should give us a sense of the volume of data being processed by each task. - set_custom_metric('num_schedules', num_schedules) - - return schedules - - -def _add_upsell_button_information_to_template_context(user, schedule, template_context): - enrollment = schedule.enrollment - course = enrollment.course - - verified_upgrade_link = _get_link_to_purchase_verified_certificate(user, schedule) - has_verified_upgrade_link = verified_upgrade_link is not None - - if has_verified_upgrade_link: - template_context['upsell_link'] = verified_upgrade_link - template_context['user_schedule_upgrade_deadline_time'] = dateformat.format( - enrollment.dynamic_upgrade_deadline, - get_format( - 'DATE_FORMAT', - lang=course.language, - use_l10n=True - ) - ) - - template_context['show_upsell'] = has_verified_upgrade_link - - -def _get_link_to_purchase_verified_certificate(a_user, a_schedule): - enrollment = a_schedule.enrollment - if enrollment.dynamic_upgrade_deadline is None or not verified_upgrade_link_is_valid(enrollment): - return None - - return verified_upgrade_deadline_link(a_user, enrollment.course) - - -def _get_datetime_beginning_of_day(dt): - """ - Truncates hours, minutes, seconds, and microseconds to zero on given datetime. - """ - return dt.replace(hour=0, minute=0, second=0, microsecond=0) From 4700656980476efed45c59bbf4d3ec8bc49a9520 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 Oct 2017 21:10:06 -0400 Subject: [PATCH 02/29] Move bin-count constants into resolvers.py nearer to the queries that use them --- .../tests/test_send_recurring_nudge.py | 18 +++++++++--------- .../tests/test_send_upgrade_reminder.py | 16 ++++++++-------- openedx/core/djangoapps/schedules/resolvers.py | 9 +++++---- openedx/core/djangoapps/schedules/tasks.py | 4 ---- .../schedules/tests/test_resolvers.py | 2 +- 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 85c9c8477f..761ec19816 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -81,7 +81,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): retry=False, ) mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_day), -3, tasks.RECURRING_NUDGE_NUM_BINS - 1, [], True, None), + (self.site_config.site.id, serialize(test_day), -3, resolvers.RECURRING_NUDGE_NUM_BINS - 1, [], True, None), retry=False, ) self.assertFalse(mock_ace.send.called) @@ -97,11 +97,11 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): ) for i in range(schedule_count) ] - bins_in_use = frozenset((s.enrollment.user.id % tasks.RECURRING_NUDGE_NUM_BINS) for s in schedules) + bins_in_use = frozenset((s.enrollment.user.id % resolvers.RECURRING_NUDGE_NUM_BINS) for s in schedules) test_datetime = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - for b in range(tasks.RECURRING_NUDGE_NUM_BINS): + for b in range(resolvers.RECURRING_NUDGE_NUM_BINS): expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES if b in bins_in_use: # to fetch course modes for valid schedules @@ -126,7 +126,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - for b in range(tasks.RECURRING_NUDGE_NUM_BINS): + for b in range(resolvers.RECURRING_NUDGE_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, @@ -143,7 +143,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): @patch.object(tasks, '_recurring_nudge_schedule_send') def test_send_after_course_end(self, mock_schedule_send): - user1 = UserFactory.create(id=tasks.RECURRING_NUDGE_NUM_BINS) + user1 = UserFactory.create(id=resolvers.RECURRING_NUDGE_NUM_BINS) schedule = ScheduleFactory.create( start=datetime.datetime(2017, 8, 3, 20, 34, 30, tzinfo=pytz.UTC), @@ -199,8 +199,8 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): for config in (limited_config, unlimited_config): ScheduleConfigFactory.create(site=config.site) - user1 = UserFactory.create(id=tasks.RECURRING_NUDGE_NUM_BINS) - user2 = UserFactory.create(id=tasks.RECURRING_NUDGE_NUM_BINS * 2) + user1 = UserFactory.create(id=resolvers.RECURRING_NUDGE_NUM_BINS) + user2 = UserFactory.create(id=resolvers.RECURRING_NUDGE_NUM_BINS * 2) ScheduleFactory.create( start=datetime.datetime(2017, 8, 3, 17, 44, 30, tzinfo=pytz.UTC), @@ -247,7 +247,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, - bin_num=user.id % tasks.RECURRING_NUDGE_NUM_BINS, + bin_num=user.id % resolvers.RECURRING_NUDGE_NUM_BINS, org_list=[schedules[0].enrollment.course.org], ) self.assertEqual(mock_schedule_send.apply_async.call_count, 1) @@ -434,7 +434,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): return templates_override def _calculate_bin_for_user(self, user): - return user.id % tasks.RECURRING_NUDGE_NUM_BINS + return user.id % resolvers.RECURRING_NUDGE_NUM_BINS def _contains_upsell_attribute(self, msg_attr): msg = Message.from_string(msg_attr) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 67a8967d46..1134bfbb06 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -85,7 +85,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): retry=False, ) mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_day), 2, tasks.UPGRADE_REMINDER_NUM_BINS - 1, [], True, None), + (self.site_config.site.id, serialize(test_day), 2, resolvers.UPGRADE_REMINDER_NUM_BINS - 1, [], True, None), retry=False, ) self.assertFalse(mock_ace.send.called) @@ -101,11 +101,11 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): ) for i in range(schedule_count) ] - bins_in_use = frozenset((s.enrollment.user.id % tasks.UPGRADE_REMINDER_NUM_BINS) for s in schedules) + bins_in_use = frozenset((s.enrollment.user.id % resolvers.UPGRADE_REMINDER_NUM_BINS) for s in schedules) test_datetime = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): + for b in range(resolvers.UPGRADE_REMINDER_NUM_BINS): expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES if b in bins_in_use: # to fetch course modes for valid schedules @@ -131,7 +131,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): + for b in range(resolvers.UPGRADE_REMINDER_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, @@ -183,8 +183,8 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): for config in (limited_config, unlimited_config): ScheduleConfigFactory.create(site=config.site) - user1 = UserFactory.create(id=tasks.UPGRADE_REMINDER_NUM_BINS) - user2 = UserFactory.create(id=tasks.UPGRADE_REMINDER_NUM_BINS * 2) + user1 = UserFactory.create(id=resolvers.UPGRADE_REMINDER_NUM_BINS) + user2 = UserFactory.create(id=resolvers.UPGRADE_REMINDER_NUM_BINS * 2) ScheduleFactory.create( upgrade_deadline=datetime.datetime(2017, 8, 3, 17, 44, 30, tzinfo=pytz.UTC), @@ -231,7 +231,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, - bin_num=user.id % tasks.UPGRADE_REMINDER_NUM_BINS, + bin_num=user.id % resolvers.UPGRADE_REMINDER_NUM_BINS, org_list=[schedules[0].enrollment.course.org], ) self.assertEqual(mock_schedule_send.apply_async.call_count, 1) @@ -309,4 +309,4 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): return templates_override def _calculate_bin_for_user(self, user): - return user.id % tasks.RECURRING_NUDGE_NUM_BINS + return user.id % resolvers.RECURRING_NUDGE_NUM_BINS diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 71cbd9490b..7fe74d6e87 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -18,10 +18,6 @@ from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig from openedx.core.djangoapps.schedules.tasks import ( - DEFAULT_NUM_BINS, - RECURRING_NUDGE_NUM_BINS, - UPGRADE_REMINDER_NUM_BINS, - COURSE_UPDATE_NUM_BINS, recurring_nudge_schedule_bin, upgrade_reminder_schedule_bin, course_update_schedule_bin, @@ -41,6 +37,11 @@ from xmodule.modulestore.django import modulestore LOG = logging.getLogger(__name__) +DEFAULT_NUM_BINS = 24 +RECURRING_NUDGE_NUM_BINS = DEFAULT_NUM_BINS +UPGRADE_REMINDER_NUM_BINS = DEFAULT_NUM_BINS +COURSE_UPDATE_NUM_BINS = DEFAULT_NUM_BINS + class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): """ Starts num_bins number of async tasks, each of which sends emails to an equal group of learners. diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 3a7ced3841..45b76e1a6a 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -31,10 +31,6 @@ KNOWN_RETRY_ERRORS = ( # Errors we expect occasionally that could resolve on re DatabaseError, ValidationError, ) -DEFAULT_NUM_BINS = 24 -RECURRING_NUDGE_NUM_BINS = DEFAULT_NUM_BINS -UPGRADE_REMINDER_NUM_BINS = DEFAULT_NUM_BINS -COURSE_UPDATE_NUM_BINS = DEFAULT_NUM_BINS @task(bind=True, default_retry_delay=30, routing_key=ROUTING_KEY) diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py index 0854c52b56..1638f2158a 100644 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -6,7 +6,7 @@ from django.conf import settings from mock import patch from openedx.core.djangoapps.schedules.resolvers import BinnedSchedulesBaseResolver -from openedx.core.djangoapps.schedules.tasks import DEFAULT_NUM_BINS +from openedx.core.djangoapps.schedules.resolvers import DEFAULT_NUM_BINS from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms From bb050381a0716ce5a95daa7e1839767cb8793f5e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 Oct 2017 21:19:51 -0400 Subject: [PATCH 03/29] Move async_send_task binding to commands to allow tasks.py to import resolvers.py --- .../schedules/management/commands/__init__.py | 3 ++- .../management/commands/send_course_update.py | 2 ++ .../commands/send_recurring_nudge.py | 2 ++ .../commands/send_upgrade_reminder.py | 2 ++ .../management/commands/tests/test_base.py | 3 ++- .../tests/test_send_recurring_nudge.py | 22 +++++++++++++------ .../tests/test_send_upgrade_reminder.py | 22 +++++++++++++------ .../core/djangoapps/schedules/resolvers.py | 21 ++++++------------ .../schedules/tests/test_resolvers.py | 4 ++-- 9 files changed, 49 insertions(+), 32 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/__init__.py b/openedx/core/djangoapps/schedules/management/commands/__init__.py index 3be3b9f1a9..258fdc9b35 100644 --- a/openedx/core/djangoapps/schedules/management/commands/__init__.py +++ b/openedx/core/djangoapps/schedules/management/commands/__init__.py @@ -9,6 +9,7 @@ from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): resolver_class = None # define in subclass + async_send_task = None # define in subclass def add_arguments(self, parser): parser.add_argument( @@ -36,7 +37,7 @@ class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): site = Site.objects.get(domain__iexact=options['site_domain_name']) self.log_debug('Running for site %s', site.domain) - return self.resolver_class(site, current_date) + return self.resolver_class(site, current_date, async_send_task=self.async_send_task) def send_emails(self, resolver, *args, **options): pass # define in subclass diff --git a/openedx/core/djangoapps/schedules/management/commands/send_course_update.py b/openedx/core/djangoapps/schedules/management/commands/send_course_update.py index 7846fc5ea5..a74339f779 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_course_update.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_course_update.py @@ -1,9 +1,11 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand from openedx.core.djangoapps.schedules.resolvers import CourseUpdateResolver +from openedx.core.djangoapps.schedules.tasks import course_update_schedule_bin class Command(SendEmailBaseCommand): resolver_class = CourseUpdateResolver + async_send_task = course_update_schedule_bin def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py index 5f3b316e60..1de9c799aa 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py @@ -1,9 +1,11 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand from openedx.core.djangoapps.schedules.resolvers import ScheduleStartResolver +from openedx.core.djangoapps.schedules.tasks import recurring_nudge_schedule_bin class Command(SendEmailBaseCommand): resolver_class = ScheduleStartResolver + async_send_task = recurring_nudge_schedule_bin def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py index c251c85275..c53f122e7e 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py @@ -1,9 +1,11 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand from openedx.core.djangoapps.schedules.resolvers import UpgradeReminderResolver +from openedx.core.djangoapps.schedules.tasks import upgrade_reminder_schedule_bin class Command(SendEmailBaseCommand): resolver_class = UpgradeReminderResolver + async_send_task = upgrade_reminder_schedule_bin def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py index ce75828775..c69e89aa3e 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py @@ -28,7 +28,8 @@ class TestSendEmailBaseCommand(CacheIsolationTestCase): self.command.make_resolver(site_domain_name='example.com', date='2017-09-29') resolver_class.assert_called_once_with( example_site, - datetime.datetime(2017, 9, 29, tzinfo=pytz.UTC) + datetime.datetime(2017, 9, 29, tzinfo=pytz.UTC), + async_send_task=None, ) def test_handle(self): diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 761ec19816..8b549b3ecb 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -64,16 +64,20 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): def test_handle(self, mock_resolver): test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) nudge.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) - mock_resolver.assert_called_with(self.site_config.site, test_day) + mock_resolver.assert_called_with( + self.site_config.site, + test_day, + async_send_task=nudge.Command.async_send_task, + ) for day in (-3, -10): mock_resolver().send.assert_any_call(day, None) @patch.object(tasks, 'ace') - @patch.object(resolvers.ScheduleStartResolver, 'async_send_task') - def test_resolver_send(self, mock_schedule_bin, mock_ace): + def test_resolver_send(self, mock_ace): current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - nudge.ScheduleStartResolver(self.site_config.site, current_day).send(-3) + mock_schedule_bin = Mock() + nudge.ScheduleStartResolver(self.site_config.site, current_day, mock_schedule_bin).send(-3) test_day = current_day + datetime.timedelta(days=-3) self.assertFalse(mock_schedule_bin.called) mock_schedule_bin.apply_async.assert_any_call( @@ -171,12 +175,16 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(resolvers.ScheduleStartResolver, 'async_send_task') - def test_enqueue_disabled(self, mock_schedule_bin, mock_ace): + def test_enqueue_disabled(self, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_recurring_nudge=False) + mock_schedule_bin = Mock() current_datetime = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - nudge.ScheduleStartResolver(self.site_config.site, current_datetime).send(3) + nudge.ScheduleStartResolver( + self.site_config.site, + current_datetime, + mock_schedule_bin, + ).send(3) self.assertFalse(mock_schedule_bin.called) self.assertFalse(mock_schedule_bin.apply_async.called) self.assertFalse(mock_ace.send.called) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 1134bfbb06..5ceaff397e 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -67,18 +67,22 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): def test_handle(self, mock_resolver): test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) reminder.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) - mock_resolver.assert_called_with(self.site_config.site, test_day) + mock_resolver.assert_called_with( + self.site_config.site, + test_day, + async_send_task=reminder.Command.async_send_task, + ) mock_resolver().send.assert_any_call(2, None) @patch.object(tasks, 'ace') - @patch.object(resolvers.UpgradeReminderResolver, 'async_send_task') - def test_resolver_send(self, mock_schedule_bin, mock_ace): + def test_resolver_send(self, mock_ace): current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) test_day = current_day + datetime.timedelta(days=2) ScheduleFactory.create(upgrade_deadline=datetime.datetime(2017, 8, 3, 15, 34, 30, tzinfo=pytz.UTC)) + mock_schedule_bin = Mock() - reminder.UpgradeReminderResolver(self.site_config.site, current_day).send(2) + reminder.UpgradeReminderResolver(self.site_config.site, current_day, mock_schedule_bin).send(2) self.assertFalse(mock_schedule_bin.called) mock_schedule_bin.apply_async.assert_any_call( (self.site_config.site.id, serialize(test_day), 2, 0, [], True, None), @@ -155,12 +159,16 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(resolvers.UpgradeReminderResolver, 'async_send_task') - def test_enqueue_disabled(self, mock_schedule_bin, mock_ace): + def test_enqueue_disabled(self, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_upgrade_reminder=False) + mock_schedule_bin = Mock() current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - reminder.UpgradeReminderResolver(self.site_config.site, current_day).send(3) + reminder.UpgradeReminderResolver( + self.site_config.site, + current_day, + async_send_task=mock_schedule_bin, + ).send(3) self.assertFalse(mock_schedule_bin.called) self.assertFalse(mock_schedule_bin.apply_async.called) self.assertFalse(mock_ace.send.called) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 7fe74d6e87..35b16f884d 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -14,14 +14,10 @@ from edx_ace.recipient_resolver import RecipientResolver from edx_ace.utils.date import serialize from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid +from openedx.core.djangoapps.monitoring_utils import set_custom_metric, function_trace from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig -from openedx.core.djangoapps.schedules.tasks import ( - recurring_nudge_schedule_bin, - upgrade_reminder_schedule_bin, - course_update_schedule_bin, -) from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin from openedx.core.djangoapps.schedules.template_context import ( absolute_url, @@ -49,20 +45,20 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): Arguments: site -- Site object that filtered Schedules will be a part of current_date -- datetime that will be used (with time zeroed-out) as the current date in the queries + async_send_task -- celery task function which this resolver will call out to Static attributes: - async_send_task -- celery task function which this resolver will call out to num_bins -- the int number of bins to split the users into enqueue_config_var -- the string field name of the config variable on ScheduleConfig to check before enqueuing """ - async_send_task = None # define in subclass num_bins = DEFAULT_NUM_BINS enqueue_config_var = None # define in subclass - def __init__(self, site, current_date, *args, **kwargs): + def __init__(self, site, current_date, async_send_task, *args, **kwargs): super(BinnedSchedulesBaseResolver, self).__init__(*args, **kwargs) self.site = site self.current_date = current_date.replace(hour=0, minute=0, second=0) + self.async_send_task = async_send_task def send(self, day_offset, override_recipient_email=None): if not self.is_enqueue_enabled(): @@ -125,7 +121,6 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. """ - async_send_task = recurring_nudge_schedule_bin num_bins = RECURRING_NUDGE_NUM_BINS enqueue_config_var = 'enqueue_recurring_nudge' @@ -250,7 +245,6 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. """ - async_send_task = upgrade_reminder_schedule_bin num_bins = UPGRADE_REMINDER_NUM_BINS enqueue_config_var = 'enqueue_upgrade_reminder' @@ -275,17 +269,17 @@ def _upgrade_reminder_schedules_for_bin(site, current_datetime, target_datetime, course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] first_schedule = user_schedules[0] - template_context = get_base_template_context(self.site) + template_context = get_base_template_context(site) template_context.update({ 'student_name': user.profile.name, 'course_links': [ { - 'url': absolute_url(self.site, reverse('course_root', args=[str(s.enrollment.course_id)])), + 'url': absolute_url(site, reverse('course_root', args=[str(s.enrollment.course_id)])), 'name': s.enrollment.course.display_name } for s in user_schedules ], 'first_course_name': first_schedule.enrollment.course.display_name, - 'cert_image': absolute_url(self.site, static('course_experience/images/verified-cert.png')), + 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), # This is used by the bulk email optout policy 'course_ids': course_id_strs, @@ -331,7 +325,6 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset`` and the course has updates. """ - async_send_task = course_update_schedule_bin num_bins = COURSE_UPDATE_NUM_BINS enqueue_config_var = 'enqueue_course_update' diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py index 1638f2158a..3c65ea5dcf 100644 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -24,12 +24,12 @@ class TestBinnedSchedulesBaseResolver(CacheIsolationTestCase): self.site_config = SiteConfigurationFactory.create(site=self.site) self.schedule_config = ScheduleConfigFactory.create(site=self.site) - def setup_resolver(self, site=None, current_date=None): + def setup_resolver(self, site=None, current_date=None, async_send_task=None): if site is None: site = self.site if current_date is None: current_date = datetime.datetime.now() - resolver = BinnedSchedulesBaseResolver(self.site, current_date) + resolver = BinnedSchedulesBaseResolver(self.site, current_date, async_send_task) return resolver def test_init_site(self): From 2e4e479f4f1c0813c187e2b025dc7fdad99580ed Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 Oct 2017 22:58:06 -0400 Subject: [PATCH 04/29] Move get_schedules_with_target_date_by_bin_and_orgs closer to its eventual home --- .../core/djangoapps/schedules/resolvers.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 35b16f884d..2a66d7dc2f 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -117,18 +117,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): return exclude_orgs, org_list -class ScheduleStartResolver(BinnedSchedulesBaseResolver): - """ - Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. - """ - num_bins = RECURRING_NUDGE_NUM_BINS - enqueue_config_var = 'enqueue_recurring_nudge' - - def __init__(self, *args, **kwargs): - super(ScheduleStartResolver, self).__init__(*args, **kwargs) - self.log_prefix = 'Scheduled Nudge' - - def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, order_by='enrollment__user__id'): @@ -198,6 +186,18 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_ return schedules +class ScheduleStartResolver(BinnedSchedulesBaseResolver): + """ + Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. + """ + num_bins = RECURRING_NUDGE_NUM_BINS + enqueue_config_var = 'enqueue_recurring_nudge' + + def __init__(self, *args, **kwargs): + super(ScheduleStartResolver, self).__init__(*args, **kwargs) + self.log_prefix = 'Scheduled Nudge' + + def _recurring_nudge_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( From 4d230d629d88732b426e77638df91834b2e0dcb1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 Oct 2017 23:49:17 -0400 Subject: [PATCH 05/29] Move the core of the bin enumeration code into resolvers.py --- .../core/djangoapps/schedules/resolvers.py | 135 +++++++++++++++++- openedx/core/djangoapps/schedules/tasks.py | 129 ++++------------- 2 files changed, 158 insertions(+), 106 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 2a66d7dc2f..21203fc8a2 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -4,20 +4,24 @@ import logging from django.conf import settings from django.contrib.auth.models import User +from django.contrib.sites.models import Site from django.contrib.staticfiles.templatetags.staticfiles import static from django.core.urlresolvers import reverse from django.db.models import F, Min, Q from django.utils.formats import dateformat, get_format + from edx_ace.recipient_resolver import RecipientResolver -from edx_ace.utils.date import serialize +from edx_ace.recipient import Recipient +from edx_ace.utils.date import serialize, deserialize from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid from openedx.core.djangoapps.monitoring_utils import set_custom_metric, function_trace from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig +from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin from openedx.core.djangoapps.schedules.template_context import ( absolute_url, @@ -186,6 +190,12 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_ return schedules +class RecurringNudge(ScheduleMessageType): + def __init__(self, day, *args, **kwargs): + super(RecurringNudge, self).__init__(*args, **kwargs) + self.name = "recurringnudge_day{}".format(day) + + class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. @@ -198,6 +208,56 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): self.log_prefix = 'Scheduled Nudge' +def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_offset): + # This identifies the type of message being sent, for example: schedules.recurring_nudge3. + set_custom_metric('message_name', '{0}.{1}'.format( + message_type.app_label, message_type.name)) + # The domain name of the site we are sending the message for. + set_custom_metric('site', site.domain) + # This is the "bin" of data being processed. We divide up the work into chunks so that we don't tie up celery + # workers for too long. This could help us identify particular bins that are problematic. + set_custom_metric('bin', bin_num) + # The date we are processing data for. + set_custom_metric('target_day', target_day_str) + # The number of days relative to the current date to process data for. + set_custom_metric('day_offset', day_offset) + # A unique identifier for this batch of messages being sent. + set_custom_metric('send_uuid', message_type.uuid) + + +def recurring_nudge_schedule_bin( + async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, +): + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) + msg_type = RecurringNudge(abs(day_offset)) + site = Site.objects.get(id=site_id) + + _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) + + for (user, language, context) in _recurring_nudge_schedules_for_bin( + site, + current_datetime, + target_datetime, + bin_num, + org_list, + exclude_orgs + ): + msg = msg_type.personalize( + Recipient( + user.username, + override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + async_send_task.apply_async( + (site_id, str(msg)), retry=False) + + + def _recurring_nudge_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( @@ -241,6 +301,9 @@ def _get_datetime_beginning_of_day(dt): return dt.replace(hour=0, minute=0, second=0, microsecond=0) +class UpgradeReminder(ScheduleMessageType): + pass + class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. @@ -253,6 +316,38 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): self.log_prefix = 'Upgrade Reminder' +def upgrade_reminder_schedule_bin( + async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, +): + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) + msg_type = UpgradeReminder() + site = Site.objects.get(id=site_id) + + _annotate_for_monitoring(msg_type, site, bin_num,target_day_str, day_offset) + + for (user, language, context) in _upgrade_reminder_schedules_for_bin( + site, + current_datetime, + target_datetime, + bin_num, + org_list, + exclude_orgs + ): + msg = msg_type.personalize( + Recipient( + user.username, + override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + async_send_task.apply_async( + (site_id, str(msg)), retry=False) + + def _upgrade_reminder_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='upgrade_deadline', @@ -320,6 +415,10 @@ def _get_link_to_purchase_verified_certificate(a_user, a_schedule): return verified_upgrade_deadline_link(a_user, enrollment.course) +class CourseUpdate(ScheduleMessageType): + pass + + class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset`` and the @@ -333,6 +432,40 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): self.log_prefix = 'Course Update' +def course_update_schedule_bin( + async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, +): + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) + msg_type = CourseUpdate() + site = Site.objects.get(id=site_id) + + _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) + + for (user, language, context) in _course_update_schedules_for_bin( + site, + current_datetime, + target_datetime, + day_offset, + bin_num, + org_list, + exclude_orgs + ): + msg = msg_type.personalize( + Recipient( + user.username, + override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + async_send_task.apply_async( + (site_id, str(msg)), retry=False) + + + def _course_update_schedules_for_bin(site, current_datetime, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False): week_num = abs(day_offset) / 7 diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 45b76e1a6a..f56d7a3b68 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -16,10 +16,9 @@ from edx_ace.recipient import Recipient from edx_ace.utils.date import deserialize from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.monitoring_utils import set_custom_metric, function_trace +from openedx.core.djangoapps.monitoring_utils import set_custom_metric from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig -from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType from openedx.core.djangoapps.schedules import resolvers @@ -50,12 +49,6 @@ def update_course_schedules(self, **kwargs): raise self.retry(kwargs=kwargs, exc=exc) -class RecurringNudge(ScheduleMessageType): - def __init__(self, day, *args, **kwargs): - super(RecurringNudge, self).__init__(*args, **kwargs) - self.name = "recurringnudge_day{}".format(day) - - @task(ignore_result=True, routing_key=ROUTING_KEY) def _recurring_nudge_schedule_send(site_id, msg_str): site = Site.objects.get(pk=site_id) @@ -76,85 +69,32 @@ def _recurring_nudge_schedule_send(site_id, msg_str): def recurring_nudge_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_datetime = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = RecurringNudge(abs(day_offset)) - site = Site.objects.get(id=site_id) - - _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - - for (user, language, context) in resolvers._recurring_nudge_schedules_for_bin( - site, - current_datetime, - target_datetime, + return resolvers.recurring_nudge_schedule_bin( + _recurring_nudge_schedule_send, + site_id, + target_day_str, + day_offset, bin_num, org_list, - exclude_orgs - ): - msg = msg_type.personalize( - Recipient( - user.username, - override_recipient_email or user.email, - ), - language, - context, - ) - with function_trace('enqueue_send_task'): - _recurring_nudge_schedule_send.apply_async((site_id, str(msg)), retry=False) - - -def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_offset): - # This identifies the type of message being sent, for example: schedules.recurring_nudge3. - set_custom_metric('message_name', '{0}.{1}'.format(message_type.app_label, message_type.name)) - # The domain name of the site we are sending the message for. - set_custom_metric('site', site.domain) - # This is the "bin" of data being processed. We divide up the work into chunks so that we don't tie up celery - # workers for too long. This could help us identify particular bins that are problematic. - set_custom_metric('bin', bin_num) - # The date we are processing data for. - set_custom_metric('target_day', target_day_str) - # The number of days relative to the current date to process data for. - set_custom_metric('day_offset', day_offset) - # A unique identifier for this batch of messages being sent. - set_custom_metric('send_uuid', message_type.uuid) - - -class UpgradeReminder(ScheduleMessageType): - pass + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) @task(ignore_result=True, routing_key=ROUTING_KEY) def upgrade_reminder_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_datetime = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = UpgradeReminder() - site = Site.objects.get(id=site_id) - - _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - - for (user, language, context) in resolvers._upgrade_reminder_schedules_for_bin( - site, - current_datetime, - target_datetime, + return resolvers.upgrade_reminder_schedule_bin( + _upgrade_reminder_schedule_send, + site_id, + target_day_str, + day_offset, bin_num, org_list, - exclude_orgs - ): - msg = msg_type.personalize( - Recipient( - user.username, - override_recipient_email or user.email, - ), - language, - context, - ) - with function_trace('enqueue_send_task'): - _upgrade_reminder_schedule_send.apply_async((site_id, str(msg)), retry=False) - + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) @task(ignore_result=True, routing_key=ROUTING_KEY) def _upgrade_reminder_schedule_send(site_id, msg_str): @@ -166,41 +106,20 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -class CourseUpdate(ScheduleMessageType): - pass - - @task(ignore_result=True, routing_key=ROUTING_KEY) def course_update_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_datetime = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = CourseUpdate() - site = Site.objects.get(id=site_id) - - _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - - for (user, language, context) in resolvers._course_update_schedules_for_bin( - site, - current_datetime, - target_datetime, + return resolvers.course_update_schedule_bin( + _course_update_schedule_send, + site_id, + target_day_str, day_offset, bin_num, org_list, - exclude_orgs - ): - msg = msg_type.personalize( - Recipient( - user.username, - override_recipient_email or user.email, - ), - language, - context, - ) - with function_trace('enqueue_send_task'): - _course_update_schedule_send.apply_async((site_id, str(msg)), retry=False) + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) @task(ignore_result=True, routing_key=ROUTING_KEY) From 0c5d788a9a9885b52aed230bcd50553dfff99044 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 18 Oct 2017 10:17:06 -0400 Subject: [PATCH 06/29] Convert bin-scheduling tasks into classes --- .../management/commands/send_course_update.py | 4 +- .../commands/send_recurring_nudge.py | 4 +- .../commands/send_upgrade_reminder.py | 4 +- .../tests/test_send_recurring_nudge.py | 20 ++-- .../tests/test_send_upgrade_reminder.py | 10 +- openedx/core/djangoapps/schedules/tasks.py | 95 ++++++++++--------- 6 files changed, 73 insertions(+), 64 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_course_update.py b/openedx/core/djangoapps/schedules/management/commands/send_course_update.py index a74339f779..b1e38599fc 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_course_update.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_course_update.py @@ -1,11 +1,11 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand from openedx.core.djangoapps.schedules.resolvers import CourseUpdateResolver -from openedx.core.djangoapps.schedules.tasks import course_update_schedule_bin +from openedx.core.djangoapps.schedules.tasks import ScheduleCourseUpdate class Command(SendEmailBaseCommand): resolver_class = CourseUpdateResolver - async_send_task = course_update_schedule_bin + async_send_task = ScheduleCourseUpdate def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py index 1de9c799aa..0202048695 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py @@ -1,11 +1,11 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand from openedx.core.djangoapps.schedules.resolvers import ScheduleStartResolver -from openedx.core.djangoapps.schedules.tasks import recurring_nudge_schedule_bin +from openedx.core.djangoapps.schedules.tasks import ScheduleRecurringNudge class Command(SendEmailBaseCommand): resolver_class = ScheduleStartResolver - async_send_task = recurring_nudge_schedule_bin + async_send_task = ScheduleRecurringNudge def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py index c53f122e7e..fbfa0fb1e6 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py @@ -1,11 +1,11 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand from openedx.core.djangoapps.schedules.resolvers import UpgradeReminderResolver -from openedx.core.djangoapps.schedules.tasks import upgrade_reminder_schedule_bin +from openedx.core.djangoapps.schedules.tasks import ScheduleUpgradeReminder class Command(SendEmailBaseCommand): resolver_class = UpgradeReminderResolver - async_send_task = upgrade_reminder_schedule_bin + async_send_task = ScheduleUpgradeReminder def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 8b549b3ecb..3fae1c53ac 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -112,7 +112,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): expected_queries += NUM_COURSE_MODES_QUERIES with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.recurring_nudge_schedule_bin( + tasks.ScheduleRecurringNudge().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) @@ -132,7 +132,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime_str = serialize(test_datetime) for b in range(resolvers.RECURRING_NUDGE_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): - tasks.recurring_nudge_schedule_bin( + tasks.ScheduleRecurringNudge().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, org_list=[schedule.enrollment.course.org], ) @@ -159,7 +159,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - tasks.recurring_nudge_schedule_bin.apply_async( + tasks.ScheduleRecurringNudge.apply_async( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0, org_list=[schedule.enrollment.course.org], ) @@ -229,7 +229,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.recurring_nudge_schedule_bin( + tasks.ScheduleRecurringNudge().run( limited_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, ) @@ -253,7 +253,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.recurring_nudge_schedule_bin( + tasks.ScheduleRecurringNudge().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=user.id % resolvers.RECURRING_NUDGE_NUM_BINS, org_list=[schedules[0].enrollment.course.org], @@ -292,7 +292,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.recurring_nudge_schedule_bin( + tasks.ScheduleRecurringNudge().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], ) @@ -339,7 +339,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): user, schedule.enrollment.course.org ] - sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.recurring_nudge_schedule_bin, + sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.ScheduleRecurringNudge, stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), bin_task_params=bin_task_parameters) @@ -371,7 +371,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): user, schedule.enrollment.course.org ] - sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.recurring_nudge_schedule_bin, + sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.ScheduleRecurringNudge, stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), bin_task_params=bin_task_parameters) @@ -410,7 +410,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): user, schedule.enrollment.course.org ] - sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.recurring_nudge_schedule_bin, + sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.ScheduleRecurringNudge, stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), bin_task_params=bin_task_parameters) @@ -426,7 +426,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) - bin_task( + bin_task().run( self.site_config.site.id, target_day_str=bin_task_params[0], day_offset=bin_task_params[1], diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 5ceaff397e..99cf2829c1 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -116,7 +116,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): expected_queries += NUM_COURSE_MODES_QUERIES with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.upgrade_reminder_schedule_bin( + tasks.ScheduleUpgradeReminder().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) @@ -137,7 +137,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime_str = serialize(test_datetime) for b in range(resolvers.UPGRADE_REMINDER_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): - tasks.upgrade_reminder_schedule_bin( + tasks.ScheduleUpgradeReminder().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, org_list=[schedule.enrollment.course.org], ) @@ -213,7 +213,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.upgrade_reminder_schedule_bin( + tasks.ScheduleUpgradeReminder().run( limited_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, ) @@ -237,7 +237,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.upgrade_reminder_schedule_bin( + tasks.ScheduleUpgradeReminder().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=user.id % resolvers.UPGRADE_REMINDER_NUM_BINS, org_list=[schedules[0].enrollment.course.org], @@ -292,7 +292,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): # since we create a new course for each schedule in this test, we expect there to be one per message num_expected_queries = NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_WITH_DEADLINE with self.assertNumQueries(num_expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.upgrade_reminder_schedule_bin( + tasks.ScheduleUpgradeReminder().run( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index f56d7a3b68..a13bd5cfe1 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -1,7 +1,7 @@ import datetime import logging -from celery.task import task +from celery.task import task, Task from django.conf import settings from django.contrib.sites.models import Site from django.contrib.staticfiles.templatetags.staticfiles import static @@ -65,36 +65,42 @@ def _recurring_nudge_schedule_send(site_id, msg_str): ace.send(msg) -@task(ignore_result=True, routing_key=ROUTING_KEY) -def recurring_nudge_schedule_bin( - site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, -): - return resolvers.recurring_nudge_schedule_bin( - _recurring_nudge_schedule_send, - site_id, - target_day_str, - day_offset, - bin_num, - org_list, - exclude_orgs=exclude_orgs, - override_recipient_email=override_recipient_email, - ) +class ScheduleRecurringNudge(Task): + ignore_result=True + routing_key=ROUTING_KEY + + def run( + self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + ): + return resolvers.recurring_nudge_schedule_bin( + _recurring_nudge_schedule_send, + site_id, + target_day_str, + day_offset, + bin_num, + org_list, + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) -@task(ignore_result=True, routing_key=ROUTING_KEY) -def upgrade_reminder_schedule_bin( - site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, -): - return resolvers.upgrade_reminder_schedule_bin( - _upgrade_reminder_schedule_send, - site_id, - target_day_str, - day_offset, - bin_num, - org_list, - exclude_orgs=exclude_orgs, - override_recipient_email=override_recipient_email, - ) +class ScheduleUpgradeReminder(Task): + ignore_result=True + routing_key=ROUTING_KEY + + def run( + self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + ): + return resolvers.upgrade_reminder_schedule_bin( + _upgrade_reminder_schedule_send, + site_id, + target_day_str, + day_offset, + bin_num, + org_list, + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) @task(ignore_result=True, routing_key=ROUTING_KEY) def _upgrade_reminder_schedule_send(site_id, msg_str): @@ -106,20 +112,23 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -@task(ignore_result=True, routing_key=ROUTING_KEY) -def course_update_schedule_bin( - site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, -): - return resolvers.course_update_schedule_bin( - _course_update_schedule_send, - site_id, - target_day_str, - day_offset, - bin_num, - org_list, - exclude_orgs=exclude_orgs, - override_recipient_email=override_recipient_email, - ) +class ScheduleCourseUpdate(Task): + ignore_result=True + routing_key=ROUTING_KEY + + def run( + self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + ): + return resolvers.course_update_schedule_bin( + _course_update_schedule_send, + site_id, + target_day_str, + day_offset, + bin_num, + org_list, + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) @task(ignore_result=True, routing_key=ROUTING_KEY) From 352fa067aeb788f3617e31ee80fb4955a3b49dbc Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 18 Oct 2017 10:22:06 -0400 Subject: [PATCH 07/29] Induce a base class for the scheduling tasks --- openedx/core/djangoapps/schedules/tasks.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index a13bd5cfe1..0fd135aa24 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -65,10 +65,11 @@ def _recurring_nudge_schedule_send(site_id, msg_str): ace.send(msg) -class ScheduleRecurringNudge(Task): - ignore_result=True - routing_key=ROUTING_KEY +class ScheduleMessageBaseTask(Task): + ignore_result = True + routing_key = ROUTING_KEY +class ScheduleRecurringNudge(ScheduleMessageBaseTask): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): @@ -84,10 +85,7 @@ class ScheduleRecurringNudge(Task): ) -class ScheduleUpgradeReminder(Task): - ignore_result=True - routing_key=ROUTING_KEY - +class ScheduleUpgradeReminder(ScheduleMessageBaseTask): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): @@ -112,10 +110,7 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -class ScheduleCourseUpdate(Task): - ignore_result=True - routing_key=ROUTING_KEY - +class ScheduleCourseUpdate(ScheduleMessageBaseTask): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): From d222b2d71849cd36577d86ca4636f6cc74053b38 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 18 Oct 2017 14:01:40 -0400 Subject: [PATCH 08/29] Move bin-task enqueuing into a classmethod (rather than having it on the RecipientResolvers --- .../schedules/management/commands/__init__.py | 20 +- .../management/commands/send_course_update.py | 6 +- .../commands/send_recurring_nudge.py | 6 +- .../commands/send_upgrade_reminder.py | 6 +- .../management/commands/tests/test_base.py | 27 +- .../tests/test_send_recurring_nudge.py | 47 +- .../tests/test_send_upgrade_reminder.py | 40 +- .../core/djangoapps/schedules/resolvers.py | 460 ++++++++---------- openedx/core/djangoapps/schedules/tasks.py | 97 +++- .../schedules/tests/test_resolvers.py | 110 ----- .../djangoapps/schedules/tests/test_tasks.py | 101 ++++ 11 files changed, 456 insertions(+), 464 deletions(-) delete mode 100644 openedx/core/djangoapps/schedules/tests/test_resolvers.py create mode 100644 openedx/core/djangoapps/schedules/tests/test_tasks.py diff --git a/openedx/core/djangoapps/schedules/management/commands/__init__.py b/openedx/core/djangoapps/schedules/management/commands/__init__.py index 258fdc9b35..0aaeefd701 100644 --- a/openedx/core/djangoapps/schedules/management/commands/__init__.py +++ b/openedx/core/djangoapps/schedules/management/commands/__init__.py @@ -8,7 +8,6 @@ from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): - resolver_class = None # define in subclass async_send_task = None # define in subclass def add_arguments(self, parser): @@ -24,20 +23,27 @@ class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): parser.add_argument('site_domain_name') def handle(self, *args, **options): - resolver = self.make_resolver(*args, **options) - self.send_emails(resolver, *args, **options) + self.log_debug('Args = %r', options) - def make_resolver(self, *args, **options): current_date = datetime.datetime( *[int(x) for x in options['date'].split('-')], tzinfo=pytz.UTC ) - self.log_debug('Args = %r', options) self.log_debug('Current date = %s', current_date.isoformat()) site = Site.objects.get(domain__iexact=options['site_domain_name']) self.log_debug('Running for site %s', site.domain) - return self.resolver_class(site, current_date, async_send_task=self.async_send_task) - def send_emails(self, resolver, *args, **options): + override_recipient_email = options.get('override_recipient_email') + self.send_emails(site, current_date, override_recipient_email) + + def enqueue(self, day_offset, site, current_date, override_recipient_email=None): + self.async_send_task.enqueue( + site, + current_date, + day_offset, + override_recipient_email, + ) + + def send_emails(self, *args, **kwargs): pass # define in subclass diff --git a/openedx/core/djangoapps/schedules/management/commands/send_course_update.py b/openedx/core/djangoapps/schedules/management/commands/send_course_update.py index b1e38599fc..2bfa393e29 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_course_update.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_course_update.py @@ -1,16 +1,14 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand -from openedx.core.djangoapps.schedules.resolvers import CourseUpdateResolver from openedx.core.djangoapps.schedules.tasks import ScheduleCourseUpdate class Command(SendEmailBaseCommand): - resolver_class = CourseUpdateResolver async_send_task = ScheduleCourseUpdate def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) self.log_prefix = 'Upgrade Reminder' - def send_emails(self, resolver, *args, **options): + def send_emails(self, *args, **kwargs): for day_offset in xrange(-7, -77, -7): - resolver.send(day_offset, options.get('override_recipient_email')) + self.enqueue(day_offset, *args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py index 0202048695..b5732e20b9 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py @@ -1,16 +1,14 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand -from openedx.core.djangoapps.schedules.resolvers import ScheduleStartResolver from openedx.core.djangoapps.schedules.tasks import ScheduleRecurringNudge class Command(SendEmailBaseCommand): - resolver_class = ScheduleStartResolver async_send_task = ScheduleRecurringNudge def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) self.log_prefix = 'Scheduled Nudge' - def send_emails(self, resolver, *args, **options): + def send_emails(self, *args, **kwargs): for day_offset in (-3, -10): - resolver.send(day_offset, options.get('override_recipient_email')) + self.enqueue(day_offset, *args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py index fbfa0fb1e6..58a6b46a96 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py @@ -1,15 +1,13 @@ from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand -from openedx.core.djangoapps.schedules.resolvers import UpgradeReminderResolver from openedx.core.djangoapps.schedules.tasks import ScheduleUpgradeReminder class Command(SendEmailBaseCommand): - resolver_class = UpgradeReminderResolver async_send_task = ScheduleUpgradeReminder def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) self.log_prefix = 'Upgrade Reminder' - def send_emails(self, resolver, *args, **options): - resolver.send(2, options.get('override_recipient_email')) + def send_emails(self, *args, **kwargs): + self.enqueue(2, *args, **kwargs) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py index c69e89aa3e..3cb6ebc59f 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py @@ -18,24 +18,13 @@ from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_un class TestSendEmailBaseCommand(CacheIsolationTestCase): def setUp(self): self.command = SendEmailBaseCommand() - - def test_init_resolver_class(self): - assert self.command.resolver_class is None - - def test_make_resolver(self): - with patch.object(self.command, 'resolver_class') as resolver_class: - example_site = SiteFactory(domain='example.com') - self.command.make_resolver(site_domain_name='example.com', date='2017-09-29') - resolver_class.assert_called_once_with( - example_site, - datetime.datetime(2017, 9, 29, tzinfo=pytz.UTC), - async_send_task=None, - ) + self.site = SiteFactory() def test_handle(self): - with patch.object(self.command, 'make_resolver') as make_resolver: - make_resolver.return_value = 'resolver' - with patch.object(self.command, 'send_emails') as send_emails: - self.command.handle(date='2017-09-29') - make_resolver.assert_called_once_with(date='2017-09-29') - send_emails.assert_called_once_with('resolver', date='2017-09-29') + with patch.object(self.command, 'send_emails') as send_emails: + self.command.handle(site_domain_name=self.site.domain, date='2017-09-29') + send_emails.assert_called_once_with( + self.site, + datetime.datetime(2017, 9, 29, tzinfo=pytz.UTC), + None + ) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 3fae1c53ac..082d8f61c2 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -60,35 +60,33 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) - @patch.object(nudge.Command, 'resolver_class') - def test_handle(self, mock_resolver): + @patch.object(nudge.Command, 'async_send_task') + def test_handle(self, mock_send): test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) nudge.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) - mock_resolver.assert_called_with( - self.site_config.site, - test_day, - async_send_task=nudge.Command.async_send_task, - ) - for day in (-3, -10): - mock_resolver().send.assert_any_call(day, None) + mock_send.enqueue.assert_any_call( + self.site_config.site, + test_day, + day, + None + ) @patch.object(tasks, 'ace') def test_resolver_send(self, mock_ace): current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - mock_schedule_bin = Mock() - nudge.ScheduleStartResolver(self.site_config.site, current_day, mock_schedule_bin).send(-3) - test_day = current_day + datetime.timedelta(days=-3) - self.assertFalse(mock_schedule_bin.called) - mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_day), -3, 0, [], True, None), - retry=False, - ) - mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_day), -3, resolvers.RECURRING_NUDGE_NUM_BINS - 1, [], True, None), - retry=False, - ) - self.assertFalse(mock_ace.send.called) + with patch.object(tasks.ScheduleRecurringNudge, 'apply_async') as mock_apply_async: + tasks.ScheduleRecurringNudge.enqueue(self.site_config.site, current_day, -3) + test_day = current_day + datetime.timedelta(days=-3) + mock_apply_async.assert_any_call( + (self.site_config.site.id, serialize(test_day), -3, 0, [], True, None), + retry=False, + ) + mock_apply_async.assert_any_call( + (self.site_config.site.id, serialize(test_day), -3, resolvers.RECURRING_NUDGE_NUM_BINS - 1, [], True, None), + retry=False, + ) + self.assertFalse(mock_ace.send.called) @ddt.data(1, 10, 100) @patch.object(tasks, 'ace') @@ -180,11 +178,12 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_bin = Mock() current_datetime = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - nudge.ScheduleStartResolver( + tasks.ScheduleRecurringNudge.enqueue( self.site_config.site, current_datetime, mock_schedule_bin, - ).send(3) + 3 + ) self.assertFalse(mock_schedule_bin.called) self.assertFalse(mock_schedule_bin.apply_async.called) self.assertFalse(mock_ace.send.called) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 99cf2829c1..f0c1875aaa 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -63,36 +63,34 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) - @patch.object(reminder.Command, 'resolver_class') - def test_handle(self, mock_resolver): + @patch.object(reminder.Command, 'async_send_task') + def test_handle(self, mock_send): test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) reminder.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) - mock_resolver.assert_called_with( + mock_send.enqueue.assert_called_with( self.site_config.site, test_day, - async_send_task=reminder.Command.async_send_task, + 2, + None ) - mock_resolver().send.assert_any_call(2, None) - @patch.object(tasks, 'ace') def test_resolver_send(self, mock_ace): current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) test_day = current_day + datetime.timedelta(days=2) ScheduleFactory.create(upgrade_deadline=datetime.datetime(2017, 8, 3, 15, 34, 30, tzinfo=pytz.UTC)) - mock_schedule_bin = Mock() - reminder.UpgradeReminderResolver(self.site_config.site, current_day, mock_schedule_bin).send(2) - self.assertFalse(mock_schedule_bin.called) - mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_day), 2, 0, [], True, None), - retry=False, - ) - mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_day), 2, resolvers.UPGRADE_REMINDER_NUM_BINS - 1, [], True, None), - retry=False, - ) - self.assertFalse(mock_ace.send.called) + with patch.object(tasks.ScheduleUpgradeReminder, 'apply_async') as mock_apply_async: + tasks.ScheduleUpgradeReminder.enqueue(self.site_config.site, current_day, 2) + mock_apply_async.assert_any_call( + (self.site_config.site.id, serialize(test_day), 2, 0, [], True, None), + retry=False, + ) + mock_apply_async.assert_any_call( + (self.site_config.site.id, serialize(test_day), 2, resolvers.UPGRADE_REMINDER_NUM_BINS - 1, [], True, None), + retry=False, + ) + self.assertFalse(mock_ace.send.called) @ddt.data(1, 10, 100) @patch.object(tasks, 'ace') @@ -164,11 +162,11 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_bin = Mock() current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - reminder.UpgradeReminderResolver( + tasks.ScheduleUpgradeReminder.enqueue( self.site_config.site, current_day, - async_send_task=mock_schedule_bin, - ).send(3) + day_offset=3, + ) self.assertFalse(mock_schedule_bin.called) self.assertFalse(mock_schedule_bin.apply_async.called) self.assertFalse(mock_ace.send.called) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 21203fc8a2..b8c47cb6fd 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -55,71 +55,8 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): num_bins -- the int number of bins to split the users into enqueue_config_var -- the string field name of the config variable on ScheduleConfig to check before enqueuing """ - num_bins = DEFAULT_NUM_BINS - enqueue_config_var = None # define in subclass - - def __init__(self, site, current_date, async_send_task, *args, **kwargs): - super(BinnedSchedulesBaseResolver, self).__init__(*args, **kwargs) - self.site = site - self.current_date = current_date.replace(hour=0, minute=0, second=0) - self.async_send_task = async_send_task - - def send(self, day_offset, override_recipient_email=None): - if not self.is_enqueue_enabled(): - self.log_debug('Message queuing disabled for site %s', self.site.domain) - return - - exclude_orgs, org_list = self.get_course_org_filter() - - target_date = self.current_date + datetime.timedelta(days=day_offset) - self.log_debug('Target date = %s', target_date.isoformat()) - for bin in range(self.num_bins): - task_args = ( - self.site.id, serialize(target_date), day_offset, bin, org_list, exclude_orgs, override_recipient_email, - ) - self.log_debug('Launching task with args = %r', task_args) - self.async_send_task.apply_async( - task_args, - retry=False, - ) - - def is_enqueue_enabled(self): - if self.enqueue_config_var: - return getattr(ScheduleConfig.current(self.site), self.enqueue_config_var) - return False - - def get_course_org_filter(self): - """ - Given the configuration of sites, get the list of orgs that should be included or excluded from this send. - - Returns: - tuple: Returns a tuple (exclude_orgs, org_list). If exclude_orgs is True, then org_list is a list of the - only orgs that should be included in this send. If exclude_orgs is False, then org_list is a list of - orgs that should be excluded from this send. All other orgs should be included. - """ - try: - site_config = SiteConfiguration.objects.get(site_id=self.site.id) - org_list = site_config.get_value('course_org_filter') - exclude_orgs = False - if not org_list: - not_orgs = set() - for other_site_config in SiteConfiguration.objects.all(): - other = other_site_config.get_value('course_org_filter') - if not isinstance(other, list): - if other is not None: - not_orgs.add(other) - else: - not_orgs.update(other) - org_list = list(not_orgs) - exclude_orgs = True - elif not isinstance(org_list, list): - org_list = [org_list] - except SiteConfiguration.DoesNotExist: - org_list = None - exclude_orgs = False - finally: - return exclude_orgs, org_list - + def send(self, msg_type): + pass def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, @@ -196,18 +133,6 @@ class RecurringNudge(ScheduleMessageType): self.name = "recurringnudge_day{}".format(day) -class ScheduleStartResolver(BinnedSchedulesBaseResolver): - """ - Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. - """ - num_bins = RECURRING_NUDGE_NUM_BINS - enqueue_config_var = 'enqueue_recurring_nudge' - - def __init__(self, *args, **kwargs): - super(ScheduleStartResolver, self).__init__(*args, **kwargs) - self.log_prefix = 'Scheduled Nudge' - - def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_offset): # This identifies the type of message being sent, for example: schedules.recurring_nudge3. set_custom_metric('message_name', '{0}.{1}'.format( @@ -225,73 +150,81 @@ def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_of set_custom_metric('send_uuid', message_type.uuid) -def recurring_nudge_schedule_bin( - async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, -): - target_datetime = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = RecurringNudge(abs(day_offset)) - site = Site.objects.get(id=site_id) +class ScheduleStartResolver(BinnedSchedulesBaseResolver): + """ + Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. + """ - _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) + def __init__(self, *args, **kwargs): + super(ScheduleStartResolver, self).__init__(*args, **kwargs) + self.log_prefix = 'Scheduled Nudge' - for (user, language, context) in _recurring_nudge_schedules_for_bin( - site, - current_datetime, - target_datetime, - bin_num, - org_list, - exclude_orgs + def recurring_nudge_schedule_bin( + self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - msg = msg_type.personalize( - Recipient( - user.username, - override_recipient_email or user.email, - ), - language, - context, + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) + msg_type = RecurringNudge(abs(day_offset)) + site = Site.objects.get(id=site_id) + + _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) + + for (user, language, context) in self._recurring_nudge_schedules_for_bin( + site, + current_datetime, + target_datetime, + bin_num, + org_list, + exclude_orgs + ): + msg = msg_type.personalize( + Recipient( + user.username, + override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + async_send_task.apply_async( + (site_id, str(msg)), retry=False) + + + def _recurring_nudge_schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='start', + current_datetime=current_datetime, + target_datetime=target_datetime, + bin_num=bin_num, + num_bins=RECURRING_NUDGE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, ) - with function_trace('enqueue_send_task'): - async_send_task.apply_async( - (site_id, str(msg)), retry=False) + LOG.debug('Recurring Nudge: Query = %r', schedules.query.sql_with_params()) + for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): + user_schedules = list(user_schedules) + course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] -def _recurring_nudge_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + first_schedule = user_schedules[0] + template_context = get_base_template_context(site) + template_context.update({ + 'student_name': user.profile.name, - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='start', - current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, - num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, - ) + 'course_name': first_schedule.enrollment.course.display_name, + 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) - for schedule in user_schedules] + # This is used by the bulk email optout policy + 'course_ids': course_id_strs, + }) - first_schedule = user_schedules[0] - template_context = get_base_template_context(site) - template_context.update({ - 'student_name': user.profile.name, + # Information for including upsell messaging in template. + _add_upsell_button_information_to_template_context( + user, first_schedule, template_context) - 'course_name': first_schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), - - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, - }) - - # Information for including upsell messaging in template. - _add_upsell_button_information_to_template_context( - user, first_schedule, template_context) - - yield (user, first_schedule.enrollment.course.language, template_context) + yield (user, first_schedule.enrollment.course.language, template_context) def _get_datetime_beginning_of_day(dt): @@ -308,81 +241,78 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. """ - num_bins = UPGRADE_REMINDER_NUM_BINS - enqueue_config_var = 'enqueue_upgrade_reminder' - def __init__(self, *args, **kwargs): super(UpgradeReminderResolver, self).__init__(*args, **kwargs) self.log_prefix = 'Upgrade Reminder' - -def upgrade_reminder_schedule_bin( - async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, -): - target_datetime = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = UpgradeReminder() - site = Site.objects.get(id=site_id) - - _annotate_for_monitoring(msg_type, site, bin_num,target_day_str, day_offset) - - for (user, language, context) in _upgrade_reminder_schedules_for_bin( - site, - current_datetime, - target_datetime, - bin_num, - org_list, - exclude_orgs + def upgrade_reminder_schedule_bin( + self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - msg = msg_type.personalize( - Recipient( - user.username, - override_recipient_email or user.email, - ), - language, - context, + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) + msg_type = UpgradeReminder() + site = Site.objects.get(id=site_id) + + _annotate_for_monitoring(msg_type, site, bin_num, + target_day_str, day_offset) + + for (user, language, context) in self._upgrade_reminder_schedules_for_bin( + site, + current_datetime, + target_datetime, + bin_num, + org_list, + exclude_orgs + ): + msg = msg_type.personalize( + Recipient( + user.username, + override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + async_send_task.apply_async( + (site_id, str(msg)), retry=False) + + + def _upgrade_reminder_schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='upgrade_deadline', + current_datetime=current_datetime, + target_datetime=target_datetime, + bin_num=bin_num, + num_bins=RECURRING_NUDGE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, ) - with function_trace('enqueue_send_task'): - async_send_task.apply_async( - (site_id, str(msg)), retry=False) + for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): + user_schedules = list(user_schedules) + course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] -def _upgrade_reminder_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='upgrade_deadline', - current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, - num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, - ) + first_schedule = user_schedules[0] + template_context = get_base_template_context(site) + template_context.update({ + 'student_name': user.profile.name, + 'course_links': [ + { + 'url': absolute_url(site, reverse('course_root', args=[str(s.enrollment.course_id)])), + 'name': s.enrollment.course.display_name + } for s in user_schedules + ], + 'first_course_name': first_schedule.enrollment.course.display_name, + 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] + # This is used by the bulk email optout policy + 'course_ids': course_id_strs, + }) - first_schedule = user_schedules[0] - template_context = get_base_template_context(site) - template_context.update({ - 'student_name': user.profile.name, - 'course_links': [ - { - 'url': absolute_url(site, reverse('course_root', args=[str(s.enrollment.course_id)])), - 'name': s.enrollment.course.display_name - } for s in user_schedules - ], - 'first_course_name': first_schedule.enrollment.course.display_name, - 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), + _add_upsell_button_information_to_template_context(user, first_schedule, template_context) - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, - }) - - _add_upsell_button_information_to_template_context(user, first_schedule, template_context) - - yield (user, first_schedule.enrollment.course.language, template_context) + yield (user, first_schedule.enrollment.course.language, template_context) def _add_upsell_button_information_to_template_context(user, schedule, template_context): @@ -424,87 +354,83 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset`` and the course has updates. """ - num_bins = COURSE_UPDATE_NUM_BINS - enqueue_config_var = 'enqueue_course_update' - def __init__(self, *args, **kwargs): super(CourseUpdateResolver, self).__init__(*args, **kwargs) self.log_prefix = 'Course Update' - -def course_update_schedule_bin( - async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, -): - target_datetime = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = CourseUpdate() - site = Site.objects.get(id=site_id) - - _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - - for (user, language, context) in _course_update_schedules_for_bin( - site, - current_datetime, - target_datetime, - day_offset, - bin_num, - org_list, - exclude_orgs + def course_update_schedule_bin( + self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - msg = msg_type.personalize( - Recipient( - user.username, - override_recipient_email or user.email, - ), - language, - context, + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) + msg_type = CourseUpdate() + site = Site.objects.get(id=site_id) + + _annotate_for_monitoring(msg_type, site, bin_num, + target_day_str, day_offset) + + for (user, language, context) in self._course_update_schedules_for_bin( + site, + current_datetime, + target_datetime, + day_offset, + bin_num, + org_list, + exclude_orgs + ): + msg = msg_type.personalize( + Recipient( + user.username, + override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + async_send_task.apply_async( + (site_id, str(msg)), retry=False) + + def _course_update_schedules_for_bin(self, site, current_datetime, target_datetime, day_offset, bin_num, org_list, + exclude_orgs=False): + week_num = abs(day_offset) / 7 + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='start', + current_datetime=current_datetime, + target_datetime=target_datetime, + bin_num=bin_num, + num_bins=COURSE_UPDATE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, + order_by='enrollment__course', ) - with function_trace('enqueue_send_task'): - async_send_task.apply_async( - (site_id, str(msg)), retry=False) + LOG.debug('Course Update: Query = %r', schedules.query.sql_with_params()) + for schedule in schedules: + enrollment = schedule.enrollment + try: + week_summary = get_course_week_summary( + enrollment.course_id, week_num) + except CourseUpdateDoesNotExist: + continue + user = enrollment.user + course_id_str = str(enrollment.course_id) -def _course_update_schedules_for_bin(site, current_datetime, target_datetime, day_offset, bin_num, org_list, - exclude_orgs=False): - week_num = abs(day_offset) / 7 - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='start', - current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, - num_bins=COURSE_UPDATE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, - order_by='enrollment__course', - ) + template_context = get_base_template_context(site) + template_context.update({ + 'student_name': user.profile.name, + 'user_personal_address': user.profile.name if user.profile.name else user.username, + 'course_name': schedule.enrollment.course.display_name, + 'course_url': absolute_url(site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), + 'week_num': week_num, + 'week_summary': week_summary, - for schedule in schedules: - enrollment = schedule.enrollment - try: - week_summary = get_course_week_summary( - enrollment.course_id, week_num) - except CourseUpdateDoesNotExist: - continue + # This is used by the bulk email optout policy + 'course_ids': [course_id_str], + }) - user = enrollment.user - course_id_str = str(enrollment.course_id) - - template_context = get_base_template_context(site) - template_context.update({ - 'student_name': user.profile.name, - 'user_personal_address': user.profile.name if user.profile.name else user.username, - 'course_name': schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), - 'week_num': week_num, - 'week_summary': week_summary, - - # This is used by the bulk email optout policy - 'course_ids': [course_id_str], - }) - - yield (user, schedule.enrollment.course.language, template_context) + yield (user, schedule.enrollment.course.language, template_context) @request_cached diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 0fd135aa24..f21b320d00 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -13,13 +13,14 @@ from django.utils.formats import dateformat, get_format from edx_ace import ace from edx_ace.message import Message from edx_ace.recipient import Recipient -from edx_ace.utils.date import deserialize +from edx_ace.utils.date import deserialize, serialize from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.monitoring_utils import set_custom_metric from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig from openedx.core.djangoapps.schedules import resolvers +from openedx.core.djangoapps.site_configuration.models import SiteConfiguration LOG = logging.getLogger(__name__) @@ -68,12 +69,91 @@ def _recurring_nudge_schedule_send(site_id, msg_str): class ScheduleMessageBaseTask(Task): ignore_result = True routing_key = ROUTING_KEY + num_bins = resolvers.DEFAULT_NUM_BINS + enqueue_config_var = None # define in subclass + log_prefix = None + + @classmethod + def log_debug(cls, message, *args, **kwargs): + LOG.debug(cls.log_prefix + ': ' + message, *args, **kwargs) + + @classmethod + def enqueue(cls, site, current_date, day_offset, override_recipient_email=None): + current_date = current_date.replace(hour=0, minute=0, second=0) + + if not cls.is_enqueue_enabled(site): + cls.log_debug( + 'Message queuing disabled for site %s', site.domain) + return + + exclude_orgs, org_list = cls.get_course_org_filter(site) + + target_date = current_date + datetime.timedelta(days=day_offset) + cls.log_debug('Target date = %s', target_date.isoformat()) + for bin in range(cls.num_bins): + task_args = ( + site.id, + serialize(target_date), + day_offset, + bin, + org_list, + exclude_orgs, + override_recipient_email, + ) + cls.log_debug('Launching task with args = %r', task_args) + cls.apply_async( + task_args, + retry=False, + ) + + @classmethod + def is_enqueue_enabled(cls, site): + if cls.enqueue_config_var: + return getattr(ScheduleConfig.current(site), cls.enqueue_config_var) + return False + + @classmethod + def get_course_org_filter(cls, site): + """ + Given the configuration of sites, get the list of orgs that should be included or excluded from this send. + + Returns: + tuple: Returns a tuple (exclude_orgs, org_list). If exclude_orgs is True, then org_list is a list of the + only orgs that should be included in this send. If exclude_orgs is False, then org_list is a list of + orgs that should be excluded from this send. All other orgs should be included. + """ + try: + site_config = SiteConfiguration.objects.get(site_id=site.id) + org_list = site_config.get_value('course_org_filter') + exclude_orgs = False + if not org_list: + not_orgs = set() + for other_site_config in SiteConfiguration.objects.all(): + other = other_site_config.get_value('course_org_filter') + if not isinstance(other, list): + if other is not None: + not_orgs.add(other) + else: + not_orgs.update(other) + org_list = list(not_orgs) + exclude_orgs = True + elif not isinstance(org_list, list): + org_list = [org_list] + except SiteConfiguration.DoesNotExist: + org_list = None + exclude_orgs = False + finally: + return exclude_orgs, org_list class ScheduleRecurringNudge(ScheduleMessageBaseTask): + num_bins = resolvers.RECURRING_NUDGE_NUM_BINS + enqueue_config_var = 'enqueue_recurring_nudge' + log_prefix = 'Scheduled Nudge' + def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return resolvers.recurring_nudge_schedule_bin( + return resolvers.ScheduleStartResolver().recurring_nudge_schedule_bin( _recurring_nudge_schedule_send, site_id, target_day_str, @@ -86,10 +166,15 @@ class ScheduleRecurringNudge(ScheduleMessageBaseTask): class ScheduleUpgradeReminder(ScheduleMessageBaseTask): + num_bins = resolvers.UPGRADE_REMINDER_NUM_BINS + enqueue_config_var = 'enqueue_upgrade_reminder' + log_prefix = 'Course Update' + + def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return resolvers.upgrade_reminder_schedule_bin( + return resolvers.UpgradeReminderResolver().upgrade_reminder_schedule_bin( _upgrade_reminder_schedule_send, site_id, target_day_str, @@ -111,10 +196,14 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): class ScheduleCourseUpdate(ScheduleMessageBaseTask): + num_bins = resolvers.COURSE_UPDATE_NUM_BINS + enqueue_config_var = 'enqueue_course_update' + log_prefix = 'Course Update' + def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return resolvers.course_update_schedule_bin( + return resolvers.CourseUpdateResolver().course_update_schedule_bin( _course_update_schedule_send, site_id, target_day_str, diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py deleted file mode 100644 index 3c65ea5dcf..0000000000 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ /dev/null @@ -1,110 +0,0 @@ -import datetime -from unittest import skipUnless - -import ddt -from django.conf import settings -from mock import patch - -from openedx.core.djangoapps.schedules.resolvers import BinnedSchedulesBaseResolver -from openedx.core.djangoapps.schedules.resolvers import DEFAULT_NUM_BINS -from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory -from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory -from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms - - -@ddt.ddt -@skip_unless_lms -@skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS, - "Can't test schedules if the app isn't installed") -class TestBinnedSchedulesBaseResolver(CacheIsolationTestCase): - def setUp(self): - super(TestBinnedSchedulesBaseResolver, self).setUp() - - self.site = SiteFactory.create() - self.site_config = SiteConfigurationFactory.create(site=self.site) - self.schedule_config = ScheduleConfigFactory.create(site=self.site) - - def setup_resolver(self, site=None, current_date=None, async_send_task=None): - if site is None: - site = self.site - if current_date is None: - current_date = datetime.datetime.now() - resolver = BinnedSchedulesBaseResolver(self.site, current_date, async_send_task) - return resolver - - def test_init_site(self): - resolver = self.setup_resolver() - assert resolver.site == self.site - - def test_init_current_date(self): - current_time = datetime.datetime.now() - resolver = self.setup_resolver(current_date=current_time) - current_date = current_time.replace(hour=0, minute=0, second=0) - assert resolver.current_date == current_date - - def test_init_async_send_task(self): - resolver = self.setup_resolver() - assert resolver.async_send_task is None - - def test_init_num_bins(self): - resolver = self.setup_resolver() - assert resolver.num_bins == DEFAULT_NUM_BINS - - def test_send_enqueue_disabled(self): - resolver = self.setup_resolver() - resolver.is_enqueue_enabled = lambda: False - with patch.object(resolver, 'async_send_task') as send: - with patch.object(resolver, 'log_debug') as log_debug: - resolver.send(day_offset=2) - log_debug.assert_called_once_with('Message queuing disabled for site %s', self.site.domain) - send.apply_async.assert_not_called() - - @ddt.data(0, 2, -3) - def test_send_enqueue_enabled(self, day_offset): - resolver = self.setup_resolver() - resolver.is_enqueue_enabled = lambda: True - resolver.get_course_org_filter = lambda: (False, None) - with patch.object(resolver, 'async_send_task') as send: - with patch.object(resolver, 'log_debug') as log_debug: - resolver.send(day_offset=day_offset) - target_date = resolver.current_date + datetime.timedelta(day_offset) - log_debug.assert_any_call('Target date = %s', target_date.isoformat()) - assert send.apply_async.call_count == DEFAULT_NUM_BINS - - @ddt.data(True, False) - def test_is_enqueue_enabled(self, enabled): - resolver = self.setup_resolver() - resolver.enqueue_config_var = 'enqueue_recurring_nudge' - self.schedule_config.enqueue_recurring_nudge = enabled - self.schedule_config.save() - assert resolver.is_enqueue_enabled() == enabled - - @ddt.unpack - @ddt.data( - ('course1', ['course1']), - (['course1', 'course2'], ['course1', 'course2']) - ) - def test_get_course_org_filter_include(self, course_org_filter, expected_org_list): - resolver = self.setup_resolver() - self.site_config.values['course_org_filter'] = course_org_filter - self.site_config.save() - exclude_orgs, org_list = resolver.get_course_org_filter() - assert not exclude_orgs - assert org_list == expected_org_list - - @ddt.unpack - @ddt.data( - (None, []), - ('course1', [u'course1']), - (['course1', 'course2'], [u'course1', u'course2']) - ) - def test_get_course_org_filter_exclude(self, course_org_filter, expected_org_list): - resolver = self.setup_resolver() - self.other_site = SiteFactory.create() - self.other_site_config = SiteConfigurationFactory.create( - site=self.other_site, - values={'course_org_filter': course_org_filter}, - ) - exclude_orgs, org_list = resolver.get_course_org_filter() - assert exclude_orgs - self.assertItemsEqual(org_list, expected_org_list) diff --git a/openedx/core/djangoapps/schedules/tests/test_tasks.py b/openedx/core/djangoapps/schedules/tests/test_tasks.py new file mode 100644 index 0000000000..63517a4d01 --- /dev/null +++ b/openedx/core/djangoapps/schedules/tests/test_tasks.py @@ -0,0 +1,101 @@ +import datetime +from unittest import skipUnless + +import ddt +from django.conf import settings +from mock import patch, DEFAULT, Mock + +from openedx.core.djangoapps.schedules.tasks import ScheduleMessageBaseTask +from openedx.core.djangoapps.schedules.resolvers import DEFAULT_NUM_BINS +from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory +from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms + + +@ddt.ddt +@skip_unless_lms +@skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS, + "Can't test schedules if the app isn't installed") +class TestScheduleMessageBaseTask(CacheIsolationTestCase): + def setUp(self): + super(TestScheduleMessageBaseTask, self).setUp() + + self.site = SiteFactory.create() + self.site_config = SiteConfigurationFactory.create(site=self.site) + self.schedule_config = ScheduleConfigFactory.create(site=self.site) + self.basetask = ScheduleMessageBaseTask + + def test_send_enqueue_disabled(self): + send = Mock(name='async_send_task') + with patch.multiple( + self.basetask, + is_enqueue_enabled=Mock(return_value=False), + log_debug=DEFAULT, + run=send, + ) as patches: + self.basetask.enqueue( + site=self.site, + current_date=datetime.datetime.now(), + day_offset=2 + ) + patches['log_debug'].assert_called_once_with( + 'Message queuing disabled for site %s', self.site.domain) + send.apply_async.assert_not_called() + + @ddt.data(0, 2, -3) + def test_send_enqueue_enabled(self, day_offset): + send = Mock(name='async_send_task') + current_date = datetime.datetime.now() + with patch.multiple( + self.basetask, + is_enqueue_enabled=Mock(return_value=True), + get_course_org_filter=Mock(return_value=(False, None)), + log_debug=DEFAULT, + run=send, + ) as patches: + self.basetask.enqueue( + site=self.site, + current_date=current_date, + day_offset=day_offset + ) + target_date = current_date.replace(hour=0, minute=0, second=0) + \ + datetime.timedelta(day_offset) + print(patches['log_debug'].mock_calls) + patches['log_debug'].assert_any_call( + 'Target date = %s', target_date.isoformat()) + assert send.call_count == DEFAULT_NUM_BINS + + @ddt.data(True, False) + def test_is_enqueue_enabled(self, enabled): + with patch.object(self.basetask, 'enqueue_config_var', 'enqueue_recurring_nudge'): + self.schedule_config.enqueue_recurring_nudge = enabled + self.schedule_config.save() + assert self.basetask.is_enqueue_enabled(self.site) == enabled + + @ddt.unpack + @ddt.data( + ('course1', ['course1']), + (['course1', 'course2'], ['course1', 'course2']) + ) + def test_get_course_org_filter_include(self, course_org_filter, expected_org_list): + self.site_config.values['course_org_filter'] = course_org_filter + self.site_config.save() + exclude_orgs, org_list = self.basetask.get_course_org_filter(self.site) + assert not exclude_orgs + assert org_list == expected_org_list + + @ddt.unpack + @ddt.data( + (None, []), + ('course1', [u'course1']), + (['course1', 'course2'], [u'course1', u'course2']) + ) + def test_get_course_org_filter_exclude(self, course_org_filter, expected_org_list): + self.other_site = SiteFactory.create() + self.other_site_config = SiteConfigurationFactory.create( + site=self.other_site, + values={'course_org_filter': course_org_filter}, + ) + exclude_orgs, org_list = self.basetask.get_course_org_filter(self.site) + assert exclude_orgs + self.assertItemsEqual(org_list, expected_org_list) From 41b27e049865a0c05442d51414a81404c12be187 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 18 Oct 2017 14:23:49 -0400 Subject: [PATCH 09/29] Removed redundant name information from schedule_bin methods --- openedx/core/djangoapps/schedules/resolvers.py | 17 +++++++++-------- openedx/core/djangoapps/schedules/tasks.py | 6 +++--- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index b8c47cb6fd..b339b2a666 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -159,7 +159,8 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): super(ScheduleStartResolver, self).__init__(*args, **kwargs) self.log_prefix = 'Scheduled Nudge' - def recurring_nudge_schedule_bin( + + def schedule_bin( self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): target_datetime = deserialize(target_day_str) @@ -170,7 +171,7 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - for (user, language, context) in self._recurring_nudge_schedules_for_bin( + for (user, language, context) in self.schedules_for_bin( site, current_datetime, target_datetime, @@ -191,7 +192,7 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): (site_id, str(msg)), retry=False) - def _recurring_nudge_schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + def schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', current_datetime=current_datetime, @@ -245,7 +246,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): super(UpgradeReminderResolver, self).__init__(*args, **kwargs) self.log_prefix = 'Upgrade Reminder' - def upgrade_reminder_schedule_bin( + def schedule_bin( self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): target_datetime = deserialize(target_day_str) @@ -257,7 +258,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) - for (user, language, context) in self._upgrade_reminder_schedules_for_bin( + for (user, language, context) in self.schedules_for_bin( site, current_datetime, target_datetime, @@ -278,7 +279,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): (site_id, str(msg)), retry=False) - def _upgrade_reminder_schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + def schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='upgrade_deadline', current_datetime=current_datetime, @@ -358,7 +359,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): super(CourseUpdateResolver, self).__init__(*args, **kwargs) self.log_prefix = 'Course Update' - def course_update_schedule_bin( + def schedule_bin( self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): target_datetime = deserialize(target_day_str) @@ -391,7 +392,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): async_send_task.apply_async( (site_id, str(msg)), retry=False) - def _course_update_schedules_for_bin(self, site, current_datetime, target_datetime, day_offset, bin_num, org_list, + def schedules_for_bin(self, site, current_datetime, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False): week_num = abs(day_offset) / 7 schedules = get_schedules_with_target_date_by_bin_and_orgs( diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index f21b320d00..62259a053c 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -153,7 +153,7 @@ class ScheduleRecurringNudge(ScheduleMessageBaseTask): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return resolvers.ScheduleStartResolver().recurring_nudge_schedule_bin( + return resolvers.ScheduleStartResolver().schedule_bin( _recurring_nudge_schedule_send, site_id, target_day_str, @@ -174,7 +174,7 @@ class ScheduleUpgradeReminder(ScheduleMessageBaseTask): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return resolvers.UpgradeReminderResolver().upgrade_reminder_schedule_bin( + return resolvers.UpgradeReminderResolver().schedule_bin( _upgrade_reminder_schedule_send, site_id, target_day_str, @@ -203,7 +203,7 @@ class ScheduleCourseUpdate(ScheduleMessageBaseTask): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return resolvers.CourseUpdateResolver().course_update_schedule_bin( + return resolvers.CourseUpdateResolver().schedule_bin( _course_update_schedule_send, site_id, target_day_str, From 505e039653e911bd2bd6d3666c7b9eccee07eae8 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 10:13:14 -0400 Subject: [PATCH 10/29] Don't swallow exceptions by returning inside finally --- openedx/core/djangoapps/schedules/tasks.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 62259a053c..0f9243be86 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -142,8 +142,9 @@ class ScheduleMessageBaseTask(Task): except SiteConfiguration.DoesNotExist: org_list = None exclude_orgs = False - finally: - return exclude_orgs, org_list + + return exclude_orgs, org_list + class ScheduleRecurringNudge(ScheduleMessageBaseTask): num_bins = resolvers.RECURRING_NUDGE_NUM_BINS From 0f1023232c7d1470bfc2c6a141be9e0bdee76188 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 10:40:19 -0400 Subject: [PATCH 11/29] Use official celery interfaces for class-based tasks --- .../commands/tests/test_send_recurring_nudge.py | 12 ++++++------ .../commands/tests/test_send_upgrade_reminder.py | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 082d8f61c2..9ef818088f 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -110,7 +110,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): expected_queries += NUM_COURSE_MODES_QUERIES with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleRecurringNudge().run( + tasks.ScheduleRecurringNudge.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) @@ -130,7 +130,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime_str = serialize(test_datetime) for b in range(resolvers.RECURRING_NUDGE_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleRecurringNudge().run( + tasks.ScheduleRecurringNudge.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, org_list=[schedule.enrollment.course.org], ) @@ -228,7 +228,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleRecurringNudge().run( + tasks.ScheduleRecurringNudge.delay( limited_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, ) @@ -252,7 +252,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleRecurringNudge().run( + tasks.ScheduleRecurringNudge.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=user.id % resolvers.RECURRING_NUDGE_NUM_BINS, org_list=[schedules[0].enrollment.course.org], @@ -291,7 +291,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleRecurringNudge().run( + tasks.ScheduleRecurringNudge.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], ) @@ -425,7 +425,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) - bin_task().run( + bin_task.delay( self.site_config.site.id, target_day_str=bin_task_params[0], day_offset=bin_task_params[1], diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index f0c1875aaa..2e9bdfba8e 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -114,7 +114,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): expected_queries += NUM_COURSE_MODES_QUERIES with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleUpgradeReminder().run( + tasks.ScheduleUpgradeReminder.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) @@ -135,7 +135,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime_str = serialize(test_datetime) for b in range(resolvers.UPGRADE_REMINDER_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleUpgradeReminder().run( + tasks.ScheduleUpgradeReminder.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, org_list=[schedule.enrollment.course.org], ) @@ -211,7 +211,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleUpgradeReminder().run( + tasks.ScheduleUpgradeReminder.delay( limited_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, ) @@ -235,7 +235,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleUpgradeReminder().run( + tasks.ScheduleUpgradeReminder.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=user.id % resolvers.UPGRADE_REMINDER_NUM_BINS, org_list=[schedules[0].enrollment.course.org], @@ -290,7 +290,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): # since we create a new course for each schedule in this test, we expect there to be one per message num_expected_queries = NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_WITH_DEADLINE with self.assertNumQueries(num_expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleUpgradeReminder().run( + tasks.ScheduleUpgradeReminder.delay( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], From 199468eca9c4796966dd221cad8653cf0a64c685 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 11:07:17 -0400 Subject: [PATCH 12/29] DRY up the Schedule*.run task methods --- .../tests/test_send_recurring_nudge.py | 26 ++-- .../tests/test_send_upgrade_reminder.py | 17 ++- openedx/core/djangoapps/schedules/tasks.py | 122 ++++++++---------- 3 files changed, 76 insertions(+), 89 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 9ef818088f..eb40d4dbbc 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -90,7 +90,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): @ddt.data(1, 10, 100) @patch.object(tasks, 'ace') - @patch.object(tasks, '_recurring_nudge_schedule_send') + @patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') def test_schedule_bin(self, schedule_count, mock_schedule_send, mock_ace): schedules = [ ScheduleFactory.create( @@ -117,7 +117,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count) self.assertFalse(mock_ace.send.called) - @patch.object(tasks, '_recurring_nudge_schedule_send') + @patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') def test_no_course_overview(self, mock_schedule_send): schedule = ScheduleFactory.create( start=datetime.datetime(2017, 8, 3, 20, 34, 30, tzinfo=pytz.UTC), @@ -143,7 +143,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): # is null. self.assertEqual(mock_schedule_send.apply_async.call_count, 0) - @patch.object(tasks, '_recurring_nudge_schedule_send') + @patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') def test_send_after_course_end(self, mock_schedule_send): user1 = UserFactory.create(id=resolvers.RECURRING_NUDGE_NUM_BINS) @@ -173,23 +173,21 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - def test_enqueue_disabled(self, mock_ace): + @patch.object(tasks.ScheduleUpgradeReminder, 'apply_async') + def test_enqueue_disabled(self, mock_ace, mock_apply_async): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_recurring_nudge=False) - mock_schedule_bin = Mock() current_datetime = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) tasks.ScheduleRecurringNudge.enqueue( self.site_config.site, current_datetime, - mock_schedule_bin, 3 ) - self.assertFalse(mock_schedule_bin.called) - self.assertFalse(mock_schedule_bin.apply_async.called) + self.assertFalse(mock_apply_async.called) self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(tasks, '_recurring_nudge_schedule_send') + @patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') @ddt.data( ((['filtered_org'], False, 1)), ((['filtered_org'], True, 2)) @@ -237,7 +235,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(tasks, '_recurring_nudge_schedule_send') + @patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') def test_multiple_enrollments(self, mock_schedule_send, mock_ace): user = UserFactory.create() schedules = [ @@ -287,7 +285,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): sent_messages = [] with self.settings(TEMPLATES=self._get_template_overrides()): - with patch.object(tasks, '_recurring_nudge_schedule_send') as mock_schedule_send: + with patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') as mock_schedule_send: mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): @@ -339,7 +337,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): schedule.enrollment.course.org ] sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.ScheduleRecurringNudge, - stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), + stubbed_send_task=patch.object(tasks.ScheduleRecurringNudge, 'async_send_task'), bin_task_params=bin_task_parameters) self.assertEqual(len(sent_messages), 1) @@ -371,7 +369,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): schedule.enrollment.course.org ] sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.ScheduleRecurringNudge, - stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), + stubbed_send_task=patch.object(tasks.ScheduleRecurringNudge, 'async_send_task'), bin_task_params=bin_task_parameters) self.assertEqual(len(sent_messages), 1) @@ -410,7 +408,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): schedule.enrollment.course.org ] sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.ScheduleRecurringNudge, - stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), + stubbed_send_task=patch.object(tasks.ScheduleRecurringNudge, 'async_send_task'), bin_task_params=bin_task_parameters) self.assertEqual(len(sent_messages), 1) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 2e9bdfba8e..85ed9f7d60 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -94,7 +94,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): @ddt.data(1, 10, 100) @patch.object(tasks, 'ace') - @patch.object(tasks, '_upgrade_reminder_schedule_send') + @patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') def test_schedule_bin(self, schedule_count, mock_schedule_send, mock_ace): schedules = [ ScheduleFactory.create( @@ -122,7 +122,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count) self.assertFalse(mock_ace.send.called) - @patch.object(tasks, '_upgrade_reminder_schedule_send') + @patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') def test_no_course_overview(self, mock_schedule_send): schedule = ScheduleFactory.create( @@ -157,22 +157,21 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - def test_enqueue_disabled(self, mock_ace): + @patch.object(tasks.ScheduleUpgradeReminder, 'apply_async') + def test_enqueue_disabled(self, mock_ace, mock_apply_async): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_upgrade_reminder=False) - mock_schedule_bin = Mock() current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) tasks.ScheduleUpgradeReminder.enqueue( self.site_config.site, current_day, day_offset=3, ) - self.assertFalse(mock_schedule_bin.called) - self.assertFalse(mock_schedule_bin.apply_async.called) + self.assertFalse(mock_apply_async.called) self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(tasks, '_upgrade_reminder_schedule_send') + @patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') @ddt.data( ((['filtered_org'], False, 1)), ((['filtered_org'], True, 2)) @@ -220,7 +219,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(tasks, '_upgrade_reminder_schedule_send') + @patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') def test_multiple_enrollments(self, mock_schedule_send, mock_ace): user = UserFactory.create() schedules = [ @@ -283,7 +282,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): sent_messages = [] with self.settings(TEMPLATES=self._get_template_overrides()): - with patch.object(tasks, '_upgrade_reminder_schedule_send') as mock_schedule_send: + with patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') as mock_schedule_send: mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) # we execute one query per course to see if it's opted out of dynamic upgrade deadlines, however, diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 0f9243be86..f426d57deb 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -50,28 +50,14 @@ def update_course_schedules(self, **kwargs): raise self.retry(kwargs=kwargs, exc=exc) -@task(ignore_result=True, routing_key=ROUTING_KEY) -def _recurring_nudge_schedule_send(site_id, msg_str): - site = Site.objects.get(pk=site_id) - if not ScheduleConfig.current(site).deliver_recurring_nudge: - LOG.debug('Recurring Nudge: Message delivery disabled for site %s', site.domain) - return - - msg = Message.from_string(msg_str) - # A unique identifier for this batch of messages being sent. - set_custom_metric('send_uuid', msg.send_uuid) - # A unique identifier for this particular message. - set_custom_metric('uuid', msg.uuid) - LOG.debug('Recurring Nudge: Sending message = %s', msg_str) - ace.send(msg) - - class ScheduleMessageBaseTask(Task): ignore_result = True routing_key = ROUTING_KEY num_bins = resolvers.DEFAULT_NUM_BINS enqueue_config_var = None # define in subclass log_prefix = None + resolver = None # define in subclass + async_send_task = None # define in subclass @classmethod def log_debug(cls, message, *args, **kwargs): @@ -145,46 +131,45 @@ class ScheduleMessageBaseTask(Task): return exclude_orgs, org_list + def run( + self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + ): + return self.resolver().schedule_bin( + self.async_send_task, + site_id, + target_day_str, + day_offset, + bin_num, + org_list, + exclude_orgs=exclude_orgs, + override_recipient_email=override_recipient_email, + ) + + +@task(ignore_result=True, routing_key=ROUTING_KEY) +def _recurring_nudge_schedule_send(site_id, msg_str): + site = Site.objects.get(pk=site_id) + if not ScheduleConfig.current(site).deliver_recurring_nudge: + LOG.debug( + 'Recurring Nudge: Message delivery disabled for site %s', site.domain) + return + + msg = Message.from_string(msg_str) + # A unique identifier for this batch of messages being sent. + set_custom_metric('send_uuid', msg.send_uuid) + # A unique identifier for this particular message. + set_custom_metric('uuid', msg.uuid) + LOG.debug('Recurring Nudge: Sending message = %s', msg_str) + ace.send(msg) + class ScheduleRecurringNudge(ScheduleMessageBaseTask): num_bins = resolvers.RECURRING_NUDGE_NUM_BINS enqueue_config_var = 'enqueue_recurring_nudge' log_prefix = 'Scheduled Nudge' + resolver = resolvers.ScheduleStartResolver + async_send_task = _recurring_nudge_schedule_send - def run( - self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, - ): - return resolvers.ScheduleStartResolver().schedule_bin( - _recurring_nudge_schedule_send, - site_id, - target_day_str, - day_offset, - bin_num, - org_list, - exclude_orgs=exclude_orgs, - override_recipient_email=override_recipient_email, - ) - - -class ScheduleUpgradeReminder(ScheduleMessageBaseTask): - num_bins = resolvers.UPGRADE_REMINDER_NUM_BINS - enqueue_config_var = 'enqueue_upgrade_reminder' - log_prefix = 'Course Update' - - - def run( - self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, - ): - return resolvers.UpgradeReminderResolver().schedule_bin( - _upgrade_reminder_schedule_send, - site_id, - target_day_str, - day_offset, - bin_num, - org_list, - exclude_orgs=exclude_orgs, - override_recipient_email=override_recipient_email, - ) @task(ignore_result=True, routing_key=ROUTING_KEY) def _upgrade_reminder_schedule_send(site_id, msg_str): @@ -193,27 +178,20 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): return msg = Message.from_string(msg_str) + # A unique identifier for this batch of messages being sent. + set_custom_metric('send_uuid', msg.send_uuid) + # A unique identifier for this particular message. + set_custom_metric('uuid', msg.uuid) ace.send(msg) -class ScheduleCourseUpdate(ScheduleMessageBaseTask): - num_bins = resolvers.COURSE_UPDATE_NUM_BINS - enqueue_config_var = 'enqueue_course_update' +class ScheduleUpgradeReminder(ScheduleMessageBaseTask): + num_bins = resolvers.UPGRADE_REMINDER_NUM_BINS + enqueue_config_var = 'enqueue_upgrade_reminder' log_prefix = 'Course Update' + resolver = resolvers.UpgradeReminderResolver + async_send_task = _upgrade_reminder_schedule_send - def run( - self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, - ): - return resolvers.CourseUpdateResolver().schedule_bin( - _course_update_schedule_send, - site_id, - target_day_str, - day_offset, - bin_num, - org_list, - exclude_orgs=exclude_orgs, - override_recipient_email=override_recipient_email, - ) @task(ignore_result=True, routing_key=ROUTING_KEY) @@ -223,4 +201,16 @@ def _course_update_schedule_send(site_id, msg_str): return msg = Message.from_string(msg_str) + # A unique identifier for this batch of messages being sent. + set_custom_metric('send_uuid', msg.send_uuid) + # A unique identifier for this particular message. + set_custom_metric('uuid', msg.uuid) ace.send(msg) + + +class ScheduleCourseUpdate(ScheduleMessageBaseTask): + num_bins = resolvers.COURSE_UPDATE_NUM_BINS + enqueue_config_var = 'enqueue_course_update' + log_prefix = 'Course Update' + resolver = resolvers.CourseUpdateResolver + async_send_task = _course_update_schedule_send From 2dc4621db044fdfda73dc3c1d047897b504d275a Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 11:35:23 -0400 Subject: [PATCH 13/29] Move log_prefix up to be a class attribute --- openedx/core/djangoapps/schedules/resolvers.py | 14 +++----------- openedx/core/djangoapps/schedules/utils.py | 5 ++++- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index b339b2a666..69c3e818ef 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -154,11 +154,7 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. """ - - def __init__(self, *args, **kwargs): - super(ScheduleStartResolver, self).__init__(*args, **kwargs) - self.log_prefix = 'Scheduled Nudge' - + log_prefix = 'Scheduled Nudge' def schedule_bin( self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, @@ -242,9 +238,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. """ - def __init__(self, *args, **kwargs): - super(UpgradeReminderResolver, self).__init__(*args, **kwargs) - self.log_prefix = 'Upgrade Reminder' + log_prefix = 'Upgrade Reminder' def schedule_bin( self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, @@ -355,9 +349,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset`` and the course has updates. """ - def __init__(self, *args, **kwargs): - super(CourseUpdateResolver, self).__init__(*args, **kwargs) - self.log_prefix = 'Course Update' + log_prefix = 'Course Update' def schedule_bin( self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, diff --git a/openedx/core/djangoapps/schedules/utils.py b/openedx/core/djangoapps/schedules/utils.py index 5705d95d22..1364cebb2a 100644 --- a/openedx/core/djangoapps/schedules/utils.py +++ b/openedx/core/djangoapps/schedules/utils.py @@ -6,9 +6,12 @@ LOG = logging.getLogger(__name__) # TODO: consider using a LoggerAdapter instead of this mixin: # https://docs.python.org/2/library/logging.html#logging.LoggerAdapter class PrefixedDebugLoggerMixin(object): + log_prefix = None + def __init__(self, *args, **kwargs): super(PrefixedDebugLoggerMixin, self).__init__(*args, **kwargs) - self.log_prefix = self.__class__.__name__ + if self.log_prefix is None: + self.log_prefix = self.__class__.__name__ def log_debug(self, message, *args, **kwargs): LOG.debug(self.log_prefix + ': ' + message, *args, **kwargs) From ec3ebafbc125a1dddc414e55fdee91d246826026 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 11:35:50 -0400 Subject: [PATCH 14/29] Pass an instantiated Site object into schedule_bin --- openedx/core/djangoapps/schedules/resolvers.py | 15 ++++++--------- openedx/core/djangoapps/schedules/tasks.py | 2 +- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 69c3e818ef..69ef8aae42 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -157,13 +157,12 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): log_prefix = 'Scheduled Nudge' def schedule_bin( - self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + self, async_send_task, site, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): target_datetime = deserialize(target_day_str) # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = RecurringNudge(abs(day_offset)) - site = Site.objects.get(id=site_id) _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) @@ -185,7 +184,7 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): ) with function_trace('enqueue_send_task'): async_send_task.apply_async( - (site_id, str(msg)), retry=False) + (site.id, str(msg)), retry=False) def schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): @@ -241,13 +240,12 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): log_prefix = 'Upgrade Reminder' def schedule_bin( - self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + self, async_send_task, site, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): target_datetime = deserialize(target_day_str) # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = UpgradeReminder() - site = Site.objects.get(id=site_id) _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) @@ -270,7 +268,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): ) with function_trace('enqueue_send_task'): async_send_task.apply_async( - (site_id, str(msg)), retry=False) + (site.id, str(msg)), retry=False) def schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): @@ -352,13 +350,12 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): log_prefix = 'Course Update' def schedule_bin( - self, async_send_task, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + self, async_send_task, site, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): target_datetime = deserialize(target_day_str) # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = CourseUpdate() - site = Site.objects.get(id=site_id) _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) @@ -382,7 +379,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): ) with function_trace('enqueue_send_task'): async_send_task.apply_async( - (site_id, str(msg)), retry=False) + (site.id, str(msg)), retry=False) def schedules_for_bin(self, site, current_datetime, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False): diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index f426d57deb..13e2536ed8 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -136,7 +136,7 @@ class ScheduleMessageBaseTask(Task): ): return self.resolver().schedule_bin( self.async_send_task, - site_id, + Site.objects.get(id=site_id), target_day_str, day_offset, bin_num, From 27fd73ac37192df1862aecf14d8570ba7b074f3d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 11:39:42 -0400 Subject: [PATCH 15/29] Move datetime deserialization out of Resolver.schedule_bin --- .../core/djangoapps/schedules/resolvers.py | 21 +++++++------------ openedx/core/djangoapps/schedules/tasks.py | 2 +- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 69ef8aae42..4fa6b6864d 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -133,7 +133,7 @@ class RecurringNudge(ScheduleMessageType): self.name = "recurringnudge_day{}".format(day) -def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_offset): +def _annotate_for_monitoring(message_type, site, bin_num, target_datetime, day_offset): # This identifies the type of message being sent, for example: schedules.recurring_nudge3. set_custom_metric('message_name', '{0}.{1}'.format( message_type.app_label, message_type.name)) @@ -143,7 +143,7 @@ def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_of # workers for too long. This could help us identify particular bins that are problematic. set_custom_metric('bin', bin_num) # The date we are processing data for. - set_custom_metric('target_day', target_day_str) + set_custom_metric('target_day', serialize(target_datetime)) # The number of days relative to the current date to process data for. set_custom_metric('day_offset', day_offset) # A unique identifier for this batch of messages being sent. @@ -157,14 +157,13 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): log_prefix = 'Scheduled Nudge' def schedule_bin( - self, async_send_task, site, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + self, async_send_task, site, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_datetime = deserialize(target_day_str) # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = RecurringNudge(abs(day_offset)) - _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) + _annotate_for_monitoring(msg_type, site, bin_num, target_datetime, day_offset) for (user, language, context) in self.schedules_for_bin( site, @@ -240,15 +239,13 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): log_prefix = 'Upgrade Reminder' def schedule_bin( - self, async_send_task, site, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + self, async_send_task, site, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_datetime = deserialize(target_day_str) # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = UpgradeReminder() - _annotate_for_monitoring(msg_type, site, bin_num, - target_day_str, day_offset) + _annotate_for_monitoring(msg_type, site, bin_num, target_datetime, day_offset) for (user, language, context) in self.schedules_for_bin( site, @@ -350,15 +347,13 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): log_prefix = 'Course Update' def schedule_bin( - self, async_send_task, site, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, + self, async_send_task, site, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_datetime = deserialize(target_day_str) # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = CourseUpdate() - _annotate_for_monitoring(msg_type, site, bin_num, - target_day_str, day_offset) + _annotate_for_monitoring(msg_type, site, bin_num, target_datetime, day_offset) for (user, language, context) in self._course_update_schedules_for_bin( site, diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 13e2536ed8..15e7a78cfb 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -137,7 +137,7 @@ class ScheduleMessageBaseTask(Task): return self.resolver().schedule_bin( self.async_send_task, Site.objects.get(id=site_id), - target_day_str, + deserialize(target_day_str), day_offset, bin_num, org_list, From eff9c9c9ab33084e78ac46f69edf0308b31922b2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 13:13:26 -0400 Subject: [PATCH 16/29] Move common attributes into Resolver.__init__ --- .../core/djangoapps/schedules/resolvers.py | 141 ++++++++---------- openedx/core/djangoapps/schedules/tasks.py | 4 +- requirements/edx/base.txt | 1 + 3 files changed, 63 insertions(+), 83 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 4fa6b6864d..d78fb3c082 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -2,6 +2,7 @@ import datetime from itertools import groupby import logging +import attr from django.conf import settings from django.contrib.auth.models import User from django.contrib.sites.models import Site @@ -42,6 +43,8 @@ RECURRING_NUDGE_NUM_BINS = DEFAULT_NUM_BINS UPGRADE_REMINDER_NUM_BINS = DEFAULT_NUM_BINS COURSE_UPDATE_NUM_BINS = DEFAULT_NUM_BINS + +@attr.s class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): """ Starts num_bins number of async tasks, each of which sends emails to an equal group of learners. @@ -55,6 +58,15 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): num_bins -- the int number of bins to split the users into enqueue_config_var -- the string field name of the config variable on ScheduleConfig to check before enqueuing """ + async_send_task = attr.ib() + site = attr.ib() + target_datetime = attr.ib() + day_offset = attr.ib() + bin_num = attr.ib() + org_list = attr.ib() + exclude_orgs = attr.ib(default=False) + override_recipient_email = attr.ib(default=None) + def send(self, msg_type): pass @@ -156,45 +168,35 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Scheduled Nudge' - def schedule_bin( - self, async_send_task, site, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, - ): - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) - msg_type = RecurringNudge(abs(day_offset)) + def schedule_bin(self): + msg_type = RecurringNudge(abs(self.day_offset)) + _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - _annotate_for_monitoring(msg_type, site, bin_num, target_datetime, day_offset) - - for (user, language, context) in self.schedules_for_bin( - site, - current_datetime, - target_datetime, - bin_num, - org_list, - exclude_orgs - ): + for (user, language, context) in self.schedules_for_bin(): msg = msg_type.personalize( Recipient( user.username, - override_recipient_email or user.email, + self.override_recipient_email or user.email, ), language, context, ) with function_trace('enqueue_send_task'): - async_send_task.apply_async( - (site.id, str(msg)), retry=False) + self.async_send_task.apply_async( + (self.site.id, str(msg)), retry=False) + def schedules_for_bin(self): + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) - def schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, + target_datetime=self.target_datetime, + bin_num=self.bin_num, num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, + org_list=self.org_list, + exclude_orgs=self.exclude_orgs, ) LOG.debug('Recurring Nudge: Query = %r', schedules.query.sql_with_params()) @@ -204,12 +206,12 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] first_schedule = user_schedules[0] - template_context = get_base_template_context(site) + template_context = get_base_template_context(self.site) template_context.update({ 'student_name': user.profile.name, 'course_name': first_schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), + 'course_url': absolute_url(self.site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), # This is used by the bulk email optout policy 'course_ids': course_id_strs, @@ -238,45 +240,34 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Upgrade Reminder' - def schedule_bin( - self, async_send_task, site, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, - ): - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) + def schedule_bin(self): msg_type = UpgradeReminder() + _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - _annotate_for_monitoring(msg_type, site, bin_num, target_datetime, day_offset) - - for (user, language, context) in self.schedules_for_bin( - site, - current_datetime, - target_datetime, - bin_num, - org_list, - exclude_orgs - ): + for (user, language, context) in self.schedules_for_bin(): msg = msg_type.personalize( Recipient( user.username, - override_recipient_email or user.email, + self.override_recipient_email or user.email, ), language, context, ) with function_trace('enqueue_send_task'): - async_send_task.apply_async( - (site.id, str(msg)), retry=False) + self.async_send_task.apply_async( + (self.site.id, str(msg)), retry=False) - - def schedules_for_bin(self, site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): + def schedules_for_bin(self): + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='upgrade_deadline', current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, + target_datetime=self.target_datetime, + bin_num=self.bin_num, num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, + org_list=self.org_list, + exclude_orgs=self.exclude_orgs, ) for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): @@ -284,17 +275,17 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] first_schedule = user_schedules[0] - template_context = get_base_template_context(site) + template_context = get_base_template_context(self.site) template_context.update({ 'student_name': user.profile.name, 'course_links': [ { - 'url': absolute_url(site, reverse('course_root', args=[str(s.enrollment.course_id)])), + 'url': absolute_url(self.site, reverse('course_root', args=[str(s.enrollment.course_id)])), 'name': s.enrollment.course.display_name } for s in user_schedules ], 'first_course_name': first_schedule.enrollment.course.display_name, - 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), + 'cert_image': absolute_url(self.site, static('course_experience/images/verified-cert.png')), # This is used by the bulk email optout policy 'course_ids': course_id_strs, @@ -346,47 +337,35 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Course Update' - def schedule_bin( - self, async_send_task, site, target_datetime, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, - ): - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = target_datetime - datetime.timedelta(days=day_offset) + def schedule_bin(self): msg_type = CourseUpdate() + _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - _annotate_for_monitoring(msg_type, site, bin_num, target_datetime, day_offset) - - for (user, language, context) in self._course_update_schedules_for_bin( - site, - current_datetime, - target_datetime, - day_offset, - bin_num, - org_list, - exclude_orgs - ): + for (user, language, context) in self._course_update_schedules_for_bin(): msg = msg_type.personalize( Recipient( user.username, - override_recipient_email or user.email, + self.override_recipient_email or user.email, ), language, context, ) with function_trace('enqueue_send_task'): - async_send_task.apply_async( - (site.id, str(msg)), retry=False) + self.async_send_task.apply_async( + (self.site.id, str(msg)), retry=False) - def schedules_for_bin(self, site, current_datetime, target_datetime, day_offset, bin_num, org_list, - exclude_orgs=False): - week_num = abs(day_offset) / 7 + def schedules_for_bin(self): + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) + week_num = abs(self.day_offset) / 7 schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', current_datetime=current_datetime, - target_datetime=target_datetime, - bin_num=bin_num, + target_datetime=self.target_datetime, + bin_num=self.bin_num, num_bins=COURSE_UPDATE_NUM_BINS, - org_list=org_list, - exclude_orgs=exclude_orgs, + org_list=self.org_list, + exclude_orgs=self.exclude_orgs, order_by='enrollment__course', ) LOG.debug('Course Update: Query = %r', schedules.query.sql_with_params()) @@ -402,12 +381,12 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): user = enrollment.user course_id_str = str(enrollment.course_id) - template_context = get_base_template_context(site) + template_context = get_base_template_context(self.site) template_context.update({ 'student_name': user.profile.name, 'user_personal_address': user.profile.name if user.profile.name else user.username, 'course_name': schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), + 'course_url': absolute_url(self.site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), 'week_num': week_num, 'week_summary': week_summary, diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 15e7a78cfb..e3ceefb985 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -134,7 +134,7 @@ class ScheduleMessageBaseTask(Task): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - return self.resolver().schedule_bin( + return self.resolver( self.async_send_task, Site.objects.get(id=site_id), deserialize(target_day_str), @@ -143,7 +143,7 @@ class ScheduleMessageBaseTask(Task): org_list, exclude_orgs=exclude_orgs, override_recipient_email=override_recipient_email, - ) + ).schedule_bin() @task(ignore_result=True, routing_key=ROUTING_KEY) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index a9783ee2ad..29d95f915e 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -4,6 +4,7 @@ # * @edx/ospr - to check licensing # * @edx/devops - to check system requirements +attrs==17.2.0 beautifulsoup4==4.1.3 beautifulsoup==3.2.1 bleach==1.4 From 5dd172b3d099d7205b09e9a83c09b36ad51b9f32 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 13:22:16 -0400 Subject: [PATCH 17/29] Use consistent beginning_of_day function --- openedx/core/djangoapps/schedules/tasks.py | 2 +- openedx/core/djangoapps/schedules/tests/test_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index e3ceefb985..33958945db 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -65,7 +65,7 @@ class ScheduleMessageBaseTask(Task): @classmethod def enqueue(cls, site, current_date, day_offset, override_recipient_email=None): - current_date = current_date.replace(hour=0, minute=0, second=0) + current_date = resolvers._get_datetime_beginning_of_day(current_date) if not cls.is_enqueue_enabled(site): cls.log_debug( diff --git a/openedx/core/djangoapps/schedules/tests/test_tasks.py b/openedx/core/djangoapps/schedules/tests/test_tasks.py index 63517a4d01..ef91c1a0cf 100644 --- a/openedx/core/djangoapps/schedules/tests/test_tasks.py +++ b/openedx/core/djangoapps/schedules/tests/test_tasks.py @@ -58,7 +58,7 @@ class TestScheduleMessageBaseTask(CacheIsolationTestCase): current_date=current_date, day_offset=day_offset ) - target_date = current_date.replace(hour=0, minute=0, second=0) + \ + target_date = current_date.replace(hour=0, minute=0, second=0, microsecond=0) + \ datetime.timedelta(day_offset) print(patches['log_debug'].mock_calls) patches['log_debug'].assert_any_call( From f0fd40421be0f5284b45f9452d20841465c8c860 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 13:40:53 -0400 Subject: [PATCH 18/29] Use SiteConfigurationFactory a bit more idiomatically --- openedx/core/djangoapps/schedules/tests/test_tasks.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/schedules/tests/test_tasks.py b/openedx/core/djangoapps/schedules/tests/test_tasks.py index ef91c1a0cf..98dcfb7ff2 100644 --- a/openedx/core/djangoapps/schedules/tests/test_tasks.py +++ b/openedx/core/djangoapps/schedules/tests/test_tasks.py @@ -91,9 +91,7 @@ class TestScheduleMessageBaseTask(CacheIsolationTestCase): (['course1', 'course2'], [u'course1', u'course2']) ) def test_get_course_org_filter_exclude(self, course_org_filter, expected_org_list): - self.other_site = SiteFactory.create() - self.other_site_config = SiteConfigurationFactory.create( - site=self.other_site, + SiteConfigurationFactory.create( values={'course_org_filter': course_org_filter}, ) exclude_orgs, org_list = self.basetask.get_course_org_filter(self.site) From b9643e9d1d34ab7431d544207f2a509fe1e47da7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 14:56:22 -0400 Subject: [PATCH 19/29] Move MessageType instantiation out of Resolvers into Tasks --- openedx/core/djangoapps/schedules/resolvers.py | 9 +++------ openedx/core/djangoapps/schedules/tasks.py | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index d78fb3c082..440b558ca5 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -168,8 +168,7 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Scheduled Nudge' - def schedule_bin(self): - msg_type = RecurringNudge(abs(self.day_offset)) + def schedule_bin(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) for (user, language, context) in self.schedules_for_bin(): @@ -240,8 +239,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Upgrade Reminder' - def schedule_bin(self): - msg_type = UpgradeReminder() + def schedule_bin(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) for (user, language, context) in self.schedules_for_bin(): @@ -337,8 +335,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Course Update' - def schedule_bin(self): - msg_type = CourseUpdate() + def schedule_bin(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) for (user, language, context) in self._course_update_schedules_for_bin(): diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 33958945db..f51b961247 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -134,6 +134,7 @@ class ScheduleMessageBaseTask(Task): def run( self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): + msg_type = self.make_message_type(day_offset) return self.resolver( self.async_send_task, Site.objects.get(id=site_id), @@ -143,7 +144,10 @@ class ScheduleMessageBaseTask(Task): org_list, exclude_orgs=exclude_orgs, override_recipient_email=override_recipient_email, - ).schedule_bin() + ).schedule_bin(msg_type) + + def make_message_type(self, day_offset): + raise NotImplementedError() @task(ignore_result=True, routing_key=ROUTING_KEY) @@ -170,6 +174,9 @@ class ScheduleRecurringNudge(ScheduleMessageBaseTask): resolver = resolvers.ScheduleStartResolver async_send_task = _recurring_nudge_schedule_send + def make_message_type(self, day_offset): + return resolvers.RecurringNudge(abs(day_offset)) + @task(ignore_result=True, routing_key=ROUTING_KEY) def _upgrade_reminder_schedule_send(site_id, msg_str): @@ -192,6 +199,9 @@ class ScheduleUpgradeReminder(ScheduleMessageBaseTask): resolver = resolvers.UpgradeReminderResolver async_send_task = _upgrade_reminder_schedule_send + def make_message_type(self, day_offset): + return resolvers.UpgradeReminder() + @task(ignore_result=True, routing_key=ROUTING_KEY) @@ -214,3 +224,6 @@ class ScheduleCourseUpdate(ScheduleMessageBaseTask): log_prefix = 'Course Update' resolver = resolvers.CourseUpdateResolver async_send_task = _course_update_schedule_send + + def make_message_type(self, day_offset): + return resolvers.CourseUpdate() From 33a50cf8998d13f797ddfd813bbf788de101aa66 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 14:57:27 -0400 Subject: [PATCH 20/29] Rename schedule_bin to send to match standard Resolver terminology --- openedx/core/djangoapps/schedules/resolvers.py | 6 +++--- openedx/core/djangoapps/schedules/tasks.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 440b558ca5..4dc8763d2e 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -168,7 +168,7 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Scheduled Nudge' - def schedule_bin(self, msg_type): + def send(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) for (user, language, context) in self.schedules_for_bin(): @@ -239,7 +239,7 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Upgrade Reminder' - def schedule_bin(self, msg_type): + def send(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) for (user, language, context) in self.schedules_for_bin(): @@ -335,7 +335,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Course Update' - def schedule_bin(self, msg_type): + def send(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) for (user, language, context) in self._course_update_schedules_for_bin(): diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index f51b961247..627ad23c06 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -144,7 +144,7 @@ class ScheduleMessageBaseTask(Task): org_list, exclude_orgs=exclude_orgs, override_recipient_email=override_recipient_email, - ).schedule_bin(msg_type) + ).send(msg_type) def make_message_type(self, day_offset): raise NotImplementedError() From 234780f5c911852e8f05a424f57efa49a2f2c202 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 15:08:38 -0400 Subject: [PATCH 21/29] Move Resolver.send into baseclass --- .../core/djangoapps/schedules/resolvers.py | 62 ++++--------------- 1 file changed, 13 insertions(+), 49 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 4dc8763d2e..1e0ee67820 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -68,7 +68,19 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): override_recipient_email = attr.ib(default=None) def send(self, msg_type): - pass + _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) + + for (user, language, context) in self.schedules_for_bin(): + msg = msg_type.personalize( + Recipient( + user.username, + self.override_recipient_email or user.email, + ), + language, + context, + ) + with function_trace('enqueue_send_task'): + self.async_send_task.apply_async((self.site.id, str(msg)), retry=False) def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, @@ -168,22 +180,6 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Scheduled Nudge' - def send(self, msg_type): - _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - - for (user, language, context) in self.schedules_for_bin(): - msg = msg_type.personalize( - Recipient( - user.username, - self.override_recipient_email or user.email, - ), - language, - context, - ) - with function_trace('enqueue_send_task'): - self.async_send_task.apply_async( - (self.site.id, str(msg)), retry=False) - def schedules_for_bin(self): # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) @@ -239,22 +235,6 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Upgrade Reminder' - def send(self, msg_type): - _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - - for (user, language, context) in self.schedules_for_bin(): - msg = msg_type.personalize( - Recipient( - user.username, - self.override_recipient_email or user.email, - ), - language, - context, - ) - with function_trace('enqueue_send_task'): - self.async_send_task.apply_async( - (self.site.id, str(msg)), retry=False) - def schedules_for_bin(self): # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) @@ -335,22 +315,6 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ log_prefix = 'Course Update' - def send(self, msg_type): - _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - - for (user, language, context) in self._course_update_schedules_for_bin(): - msg = msg_type.personalize( - Recipient( - user.username, - self.override_recipient_email or user.email, - ), - language, - context, - ) - with function_trace('enqueue_send_task'): - self.async_send_task.apply_async( - (self.site.id, str(msg)), retry=False) - def schedules_for_bin(self): # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) From e81931da32ff2a760bd3f34bb92fb2f3d9f48370 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 19 Oct 2017 15:23:31 -0400 Subject: [PATCH 22/29] Move get_schedules_with_target_date_by_bin_and_orgs into Resolver baseclass --- .../core/djangoapps/schedules/resolvers.py | 166 ++++++++---------- 1 file changed, 75 insertions(+), 91 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 1e0ee67820..2a3aed6456 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -67,6 +67,13 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): exclude_orgs = attr.ib(default=False) override_recipient_email = attr.ib(default=None) + schedule_date_field = None + num_bins = DEFAULT_NUM_BINS + + def __attrs_post_init__(self): + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + self.current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) + def send(self, msg_type): _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) @@ -82,73 +89,73 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): with function_trace('enqueue_send_task'): self.async_send_task.apply_async((self.site.id, str(msg)), retry=False) -def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, - num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, - order_by='enrollment__user__id'): - """ - Returns Schedules with the target_date, related to Users whose id matches the bin_num, and filtered by org_list. + def get_schedules_with_target_date_by_bin_and_orgs( + self, order_by='enrollment__user__id' + ): + """ + Returns Schedules with the target_date, related to Users whose id matches the bin_num, and filtered by org_list. - Arguments: - schedule_date_field -- string field name to query on the User's Schedule model - current_datetime -- datetime that will be used as "right now" in the query - target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under - bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num - num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) - org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) - exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list - or strictly include (False) them (default: False) - order_by -- string for field to sort the resulting Schedules by - """ - target_day = _get_datetime_beginning_of_day(target_datetime) - schedule_day_equals_target_day_filter = { - 'courseenrollment__schedule__{}__gte'.format(schedule_date_field): target_day, - 'courseenrollment__schedule__{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), - } - users = User.objects.filter( - courseenrollment__is_active=True, - **schedule_day_equals_target_day_filter - ).annotate( - id_mod=F('id') % num_bins - ).filter( - id_mod=bin_num - ) + Arguments: + schedule_date_field -- string field name to query on the User's Schedule model + current_datetime -- datetime that will be used as "right now" in the query + target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under + bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num + num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) + org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) + exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list + or strictly include (False) them (default: False) + order_by -- string for field to sort the resulting Schedules by + """ + target_day = _get_datetime_beginning_of_day(self.target_datetime) + schedule_day_equals_target_day_filter = { + 'courseenrollment__schedule__{}__gte'.format(self.schedule_date_field): target_day, + 'courseenrollment__schedule__{}__lt'.format(self.schedule_date_field): target_day + datetime.timedelta(days=1), + } + users = User.objects.filter( + courseenrollment__is_active=True, + **schedule_day_equals_target_day_filter + ).annotate( + id_mod=F('id') % self.num_bins + ).filter( + id_mod=self.bin_num + ) - schedule_day_equals_target_day_filter = { - '{}__gte'.format(schedule_date_field): target_day, - '{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), - } - schedules = Schedule.objects.select_related( - 'enrollment__user__profile', - 'enrollment__course', - ).prefetch_related( - 'enrollment__course__modes' - ).filter( - Q(enrollment__course__end__isnull=True) | Q( - enrollment__course__end__gte=current_datetime), - enrollment__user__in=users, - enrollment__is_active=True, - **schedule_day_equals_target_day_filter - ).order_by(order_by) + schedule_day_equals_target_day_filter = { + '{}__gte'.format(self.schedule_date_field): target_day, + '{}__lt'.format(self.schedule_date_field): target_day + datetime.timedelta(days=1), + } + schedules = Schedule.objects.select_related( + 'enrollment__user__profile', + 'enrollment__course', + ).prefetch_related( + 'enrollment__course__modes' + ).filter( + Q(enrollment__course__end__isnull=True) | Q( + enrollment__course__end__gte=self.current_datetime), + enrollment__user__in=users, + enrollment__is_active=True, + **schedule_day_equals_target_day_filter + ).order_by(order_by) - if org_list is not None: - if exclude_orgs: - schedules = schedules.exclude(enrollment__course__org__in=org_list) - else: - schedules = schedules.filter(enrollment__course__org__in=org_list) + if self.org_list is not None: + if self.exclude_orgs: + schedules = schedules.exclude(enrollment__course__org__in=self.org_list) + else: + schedules = schedules.filter(enrollment__course__org__in=self.org_list) - if "read_replica" in settings.DATABASES: - schedules = schedules.using("read_replica") + if "read_replica" in settings.DATABASES: + schedules = schedules.using("read_replica") - LOG.debug('Query = %r', schedules.query.sql_with_params()) + LOG.debug('Query = %r', schedules.query.sql_with_params()) - with function_trace('schedule_query_set_evaluation'): - # This will run the query and cache all of the results in memory. - num_schedules = len(schedules) + with function_trace('schedule_query_set_evaluation'): + # This will run the query and cache all of the results in memory. + num_schedules = len(schedules) - # This should give us a sense of the volume of data being processed by each task. - set_custom_metric('num_schedules', num_schedules) + # This should give us a sense of the volume of data being processed by each task. + set_custom_metric('num_schedules', num_schedules) - return schedules + return schedules class RecurringNudge(ScheduleMessageType): @@ -179,20 +186,12 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. """ log_prefix = 'Scheduled Nudge' + schedule_date_field = 'start' + num_bins = RECURRING_NUDGE_NUM_BINS def schedules_for_bin(self): - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='start', - current_datetime=current_datetime, - target_datetime=self.target_datetime, - bin_num=self.bin_num, - num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=self.org_list, - exclude_orgs=self.exclude_orgs, - ) + schedules = self.get_schedules_with_target_date_by_bin_and_orgs() LOG.debug('Recurring Nudge: Query = %r', schedules.query.sql_with_params()) @@ -234,19 +233,11 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. """ log_prefix = 'Upgrade Reminder' + schedule_date_field = 'upgrade_deadline' + num_bins = UPGRADE_REMINDER_NUM_BINS def schedules_for_bin(self): - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='upgrade_deadline', - current_datetime=current_datetime, - target_datetime=self.target_datetime, - bin_num=self.bin_num, - num_bins=RECURRING_NUDGE_NUM_BINS, - org_list=self.org_list, - exclude_orgs=self.exclude_orgs, - ) + schedules = self.get_schedules_with_target_date_by_bin_and_orgs() for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): user_schedules = list(user_schedules) @@ -314,19 +305,12 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): course has updates. """ log_prefix = 'Course Update' + schedule_date_field = 'start' + num_bins = COURSE_UPDATE_NUM_BINS def schedules_for_bin(self): - # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here - current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) week_num = abs(self.day_offset) / 7 - schedules = get_schedules_with_target_date_by_bin_and_orgs( - schedule_date_field='start', - current_datetime=current_datetime, - target_datetime=self.target_datetime, - bin_num=self.bin_num, - num_bins=COURSE_UPDATE_NUM_BINS, - org_list=self.org_list, - exclude_orgs=self.exclude_orgs, + schedules = self.get_schedules_with_target_date_by_bin_and_orgs( order_by='enrollment__course', ) LOG.debug('Course Update: Query = %r', schedules.query.sql_with_params()) From c19a3368f0bdea4d96baa5041d42e04489f928bb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 20 Oct 2017 11:59:48 -0400 Subject: [PATCH 23/29] Move MessageType definitions into tasks.py --- .../core/djangoapps/schedules/resolvers.py | 14 ------------- openedx/core/djangoapps/schedules/tasks.py | 21 ++++++++++++++++--- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 2a3aed6456..7a89b8c2b1 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -22,7 +22,6 @@ from openedx.core.djangoapps.monitoring_utils import set_custom_metric, function from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig -from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin from openedx.core.djangoapps.schedules.template_context import ( absolute_url, @@ -158,12 +157,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): return schedules -class RecurringNudge(ScheduleMessageType): - def __init__(self, day, *args, **kwargs): - super(RecurringNudge, self).__init__(*args, **kwargs) - self.name = "recurringnudge_day{}".format(day) - - def _annotate_for_monitoring(message_type, site, bin_num, target_datetime, day_offset): # This identifies the type of message being sent, for example: schedules.recurring_nudge3. set_custom_metric('message_name', '{0}.{1}'.format( @@ -225,9 +218,6 @@ def _get_datetime_beginning_of_day(dt): return dt.replace(hour=0, minute=0, second=0, microsecond=0) -class UpgradeReminder(ScheduleMessageType): - pass - class UpgradeReminderResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose verified upgrade deadline is at ``self.current_date`` + ``day_offset``. @@ -295,10 +285,6 @@ def _get_link_to_purchase_verified_certificate(a_user, a_schedule): return verified_upgrade_deadline_link(a_user, enrollment.course) -class CourseUpdate(ScheduleMessageType): - pass - - class CourseUpdateResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset`` and the diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 627ad23c06..d9d3caf60a 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -20,6 +20,7 @@ from openedx.core.djangoapps.monitoring_utils import set_custom_metric from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig from openedx.core.djangoapps.schedules import resolvers +from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType from openedx.core.djangoapps.site_configuration.models import SiteConfiguration @@ -167,6 +168,12 @@ def _recurring_nudge_schedule_send(site_id, msg_str): ace.send(msg) +class RecurringNudge(ScheduleMessageType): + def __init__(self, day, *args, **kwargs): + super(RecurringNudge, self).__init__(*args, **kwargs) + self.name = "recurringnudge_day{}".format(day) + + class ScheduleRecurringNudge(ScheduleMessageBaseTask): num_bins = resolvers.RECURRING_NUDGE_NUM_BINS enqueue_config_var = 'enqueue_recurring_nudge' @@ -175,7 +182,7 @@ class ScheduleRecurringNudge(ScheduleMessageBaseTask): async_send_task = _recurring_nudge_schedule_send def make_message_type(self, day_offset): - return resolvers.RecurringNudge(abs(day_offset)) + return RecurringNudge(abs(day_offset)) @task(ignore_result=True, routing_key=ROUTING_KEY) @@ -192,6 +199,10 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) +class UpgradeReminder(ScheduleMessageType): + pass + + class ScheduleUpgradeReminder(ScheduleMessageBaseTask): num_bins = resolvers.UPGRADE_REMINDER_NUM_BINS enqueue_config_var = 'enqueue_upgrade_reminder' @@ -200,7 +211,7 @@ class ScheduleUpgradeReminder(ScheduleMessageBaseTask): async_send_task = _upgrade_reminder_schedule_send def make_message_type(self, day_offset): - return resolvers.UpgradeReminder() + return UpgradeReminder() @@ -218,6 +229,10 @@ def _course_update_schedule_send(site_id, msg_str): ace.send(msg) +class CourseUpdate(ScheduleMessageType): + pass + + class ScheduleCourseUpdate(ScheduleMessageBaseTask): num_bins = resolvers.COURSE_UPDATE_NUM_BINS enqueue_config_var = 'enqueue_course_update' @@ -226,4 +241,4 @@ class ScheduleCourseUpdate(ScheduleMessageBaseTask): async_send_task = _course_update_schedule_send def make_message_type(self, day_offset): - return resolvers.CourseUpdate() + return CourseUpdate() From 65a740e52f12c69a7bfade99c98db175c2f82aad Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 23 Oct 2017 23:26:12 -0400 Subject: [PATCH 24/29] Make SendEmailBaseCommand.send_emails raise NotImplementedError --- .../djangoapps/schedules/management/commands/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/__init__.py b/openedx/core/djangoapps/schedules/management/commands/__init__.py index 0aaeefd701..8c71e049d6 100644 --- a/openedx/core/djangoapps/schedules/management/commands/__init__.py +++ b/openedx/core/djangoapps/schedules/management/commands/__init__.py @@ -37,6 +37,9 @@ class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): override_recipient_email = options.get('override_recipient_email') self.send_emails(site, current_date, override_recipient_email) + def send_emails(self, *args, **kwargs): + raise NotImplementedError + def enqueue(self, day_offset, site, current_date, override_recipient_email=None): self.async_send_task.enqueue( site, @@ -44,6 +47,3 @@ class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): day_offset, override_recipient_email, ) - - def send_emails(self, *args, **kwargs): - pass # define in subclass From 9e00f9275e7fd55feadc837f547e3306416180db Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 23 Oct 2017 23:33:01 -0400 Subject: [PATCH 25/29] Move MessageType definitions into message_types.py --- .../{message_type.py => message_types.py} | 14 +++++++++++ openedx/core/djangoapps/schedules/tasks.py | 23 ++++--------------- 2 files changed, 18 insertions(+), 19 deletions(-) rename openedx/core/djangoapps/schedules/{message_type.py => message_types.py} (54%) diff --git a/openedx/core/djangoapps/schedules/message_type.py b/openedx/core/djangoapps/schedules/message_types.py similarity index 54% rename from openedx/core/djangoapps/schedules/message_type.py rename to openedx/core/djangoapps/schedules/message_types.py index fc3574232b..187d3149f4 100644 --- a/openedx/core/djangoapps/schedules/message_type.py +++ b/openedx/core/djangoapps/schedules/message_types.py @@ -9,3 +9,17 @@ class ScheduleMessageType(MessageType): def __init__(self, *args, **kwargs): super(ScheduleMessageType, self).__init__(*args, **kwargs) self.log_level = logging.DEBUG if DEBUG_MESSAGE_WAFFLE_FLAG.is_enabled() else None + + +class RecurringNudge(ScheduleMessageType): + def __init__(self, day, *args, **kwargs): + super(RecurringNudge, self).__init__(*args, **kwargs) + self.name = "recurringnudge_day{}".format(day) + + +class UpgradeReminder(ScheduleMessageType): + pass + + +class CourseUpdate(ScheduleMessageType): + pass diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index d9d3caf60a..576d76892c 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -17,10 +17,9 @@ from edx_ace.utils.date import deserialize, serialize from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.monitoring_utils import set_custom_metric - +from openedx.core.djangoapps.schedules import message_types from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig from openedx.core.djangoapps.schedules import resolvers -from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType from openedx.core.djangoapps.site_configuration.models import SiteConfiguration @@ -168,12 +167,6 @@ def _recurring_nudge_schedule_send(site_id, msg_str): ace.send(msg) -class RecurringNudge(ScheduleMessageType): - def __init__(self, day, *args, **kwargs): - super(RecurringNudge, self).__init__(*args, **kwargs) - self.name = "recurringnudge_day{}".format(day) - - class ScheduleRecurringNudge(ScheduleMessageBaseTask): num_bins = resolvers.RECURRING_NUDGE_NUM_BINS enqueue_config_var = 'enqueue_recurring_nudge' @@ -182,7 +175,7 @@ class ScheduleRecurringNudge(ScheduleMessageBaseTask): async_send_task = _recurring_nudge_schedule_send def make_message_type(self, day_offset): - return RecurringNudge(abs(day_offset)) + return message_types.RecurringNudge(abs(day_offset)) @task(ignore_result=True, routing_key=ROUTING_KEY) @@ -199,10 +192,6 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -class UpgradeReminder(ScheduleMessageType): - pass - - class ScheduleUpgradeReminder(ScheduleMessageBaseTask): num_bins = resolvers.UPGRADE_REMINDER_NUM_BINS enqueue_config_var = 'enqueue_upgrade_reminder' @@ -211,7 +200,7 @@ class ScheduleUpgradeReminder(ScheduleMessageBaseTask): async_send_task = _upgrade_reminder_schedule_send def make_message_type(self, day_offset): - return UpgradeReminder() + return message_types.UpgradeReminder() @@ -229,10 +218,6 @@ def _course_update_schedule_send(site_id, msg_str): ace.send(msg) -class CourseUpdate(ScheduleMessageType): - pass - - class ScheduleCourseUpdate(ScheduleMessageBaseTask): num_bins = resolvers.COURSE_UPDATE_NUM_BINS enqueue_config_var = 'enqueue_course_update' @@ -241,4 +226,4 @@ class ScheduleCourseUpdate(ScheduleMessageBaseTask): async_send_task = _course_update_schedule_send def make_message_type(self, day_offset): - return CourseUpdate() + return message_types.CourseUpdate() From dc3c79124bc49be515ccc798d0e85dd327737bec Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 23 Oct 2017 23:39:10 -0400 Subject: [PATCH 26/29] Clean up errant leftover imports --- openedx/core/djangoapps/schedules/resolvers.py | 13 ++++--------- openedx/core/djangoapps/schedules/tasks.py | 5 +---- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 7a89b8c2b1..113b8d2950 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -5,31 +5,26 @@ import logging import attr from django.conf import settings from django.contrib.auth.models import User -from django.contrib.sites.models import Site from django.contrib.staticfiles.templatetags.staticfiles import static from django.core.urlresolvers import reverse -from django.db.models import F, Min, Q +from django.db.models import F, Q from django.utils.formats import dateformat, get_format - from edx_ace.recipient_resolver import RecipientResolver from edx_ace.recipient import Recipient -from edx_ace.utils.date import serialize, deserialize +from edx_ace.utils.date import serialize from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid -from openedx.core.djangoapps.monitoring_utils import set_custom_metric, function_trace +from openedx.core.djangoapps.monitoring_utils import function_trace, set_custom_metric from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist -from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig +from openedx.core.djangoapps.schedules.models import Schedule from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin from openedx.core.djangoapps.schedules.template_context import ( absolute_url, - encode_url, - encode_urls_in_dict, get_base_template_context ) -from openedx.core.djangoapps.site_configuration.models import SiteConfiguration from request_cache.middleware import request_cached from xmodule.modulestore.django import modulestore diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 576d76892c..d76775c197 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -4,15 +4,12 @@ import logging from celery.task import task, Task from django.conf import settings from django.contrib.sites.models import Site -from django.contrib.staticfiles.templatetags.staticfiles import static from django.core.exceptions import ValidationError -from django.core.urlresolvers import reverse + from django.db.utils import DatabaseError -from django.utils.formats import dateformat, get_format from edx_ace import ace from edx_ace.message import Message -from edx_ace.recipient import Recipient from edx_ace.utils.date import deserialize, serialize from opaque_keys.edx.keys import CourseKey From 887191b1b3c6aa3e03f8885d06f56b9bc3df742f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 23 Oct 2017 23:43:02 -0400 Subject: [PATCH 27/29] Pull template context generation out of inside loop --- openedx/core/djangoapps/schedules/resolvers.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 113b8d2950..dcee44dd80 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -180,18 +180,15 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): def schedules_for_bin(self): schedules = self.get_schedules_with_target_date_by_bin_and_orgs() - - LOG.debug('Recurring Nudge: Query = %r', schedules.query.sql_with_params()) + template_context = get_base_template_context(self.site) for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): user_schedules = list(user_schedules) course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] first_schedule = user_schedules[0] - template_context = get_base_template_context(self.site) template_context.update({ 'student_name': user.profile.name, - 'course_name': first_schedule.enrollment.course.display_name, 'course_url': absolute_url(self.site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), From 1accff9b787f1f5e45bb426003ca048122a9db26 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 23 Oct 2017 23:51:41 -0400 Subject: [PATCH 28/29] DRY up more of tasks.py code --- .../core/djangoapps/schedules/resolvers.py | 38 +----- openedx/core/djangoapps/schedules/tasks.py | 125 +++++++++++------- 2 files changed, 83 insertions(+), 80 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index dcee44dd80..b424788d3b 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -13,7 +13,6 @@ from django.utils.formats import dateformat, get_format from edx_ace.recipient_resolver import RecipientResolver from edx_ace.recipient import Recipient -from edx_ace.utils.date import serialize from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid from openedx.core.djangoapps.monitoring_utils import function_trace, set_custom_metric @@ -69,8 +68,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): self.current_datetime = self.target_datetime - datetime.timedelta(days=self.day_offset) def send(self, msg_type): - _annotate_for_monitoring(msg_type, self.site, self.bin_num, self.target_datetime, self.day_offset) - for (user, language, context) in self.schedules_for_bin(): msg = msg_type.personalize( Recipient( @@ -152,23 +149,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): return schedules -def _annotate_for_monitoring(message_type, site, bin_num, target_datetime, day_offset): - # This identifies the type of message being sent, for example: schedules.recurring_nudge3. - set_custom_metric('message_name', '{0}.{1}'.format( - message_type.app_label, message_type.name)) - # The domain name of the site we are sending the message for. - set_custom_metric('site', site.domain) - # This is the "bin" of data being processed. We divide up the work into chunks so that we don't tie up celery - # workers for too long. This could help us identify particular bins that are problematic. - set_custom_metric('bin', bin_num) - # The date we are processing data for. - set_custom_metric('target_day', serialize(target_datetime)) - # The number of days relative to the current date to process data for. - set_custom_metric('day_offset', day_offset) - # A unique identifier for this batch of messages being sent. - set_custom_metric('send_uuid', message_type.uuid) - - class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``. @@ -251,8 +231,7 @@ def _add_upsell_button_information_to_template_context(user, schedule, template_ enrollment = schedule.enrollment course = enrollment.course - verified_upgrade_link = _get_link_to_purchase_verified_certificate( - user, schedule) + verified_upgrade_link = _get_verified_upgrade_link(user, schedule) has_verified_upgrade_link = verified_upgrade_link is not None if has_verified_upgrade_link: @@ -269,12 +248,10 @@ def _add_upsell_button_information_to_template_context(user, schedule, template_ template_context['show_upsell'] = has_verified_upgrade_link -def _get_link_to_purchase_verified_certificate(a_user, a_schedule): - enrollment = a_schedule.enrollment - if enrollment.dynamic_upgrade_deadline is None or not verified_upgrade_link_is_valid(enrollment): - return None - - return verified_upgrade_deadline_link(a_user, enrollment.course) +def _get_verified_upgrade_link(user, schedule): + enrollment = schedule.enrollment + if enrollment.dynamic_upgrade_deadline is not None and verified_upgrade_link_is_valid(enrollment): + return verified_upgrade_deadline_link(user, enrollment.course) class CourseUpdateResolver(BinnedSchedulesBaseResolver): @@ -291,13 +268,11 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): schedules = self.get_schedules_with_target_date_by_bin_and_orgs( order_by='enrollment__course', ) - LOG.debug('Course Update: Query = %r', schedules.query.sql_with_params()) for schedule in schedules: enrollment = schedule.enrollment try: - week_summary = get_course_week_summary( - enrollment.course_id, week_num) + week_summary = get_course_week_summary(enrollment.course_id, week_num) except CourseUpdateDoesNotExist: continue @@ -307,7 +282,6 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): template_context = get_base_template_context(self.site) template_context.update({ 'student_name': user.profile.name, - 'user_personal_address': user.profile.name if user.profile.name else user.username, 'course_name': schedule.enrollment.course.display_name, 'course_url': absolute_url(self.site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), 'week_num': week_num, diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index d76775c197..96c391f6c5 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -30,6 +30,11 @@ KNOWN_RETRY_ERRORS = ( # Errors we expect occasionally that could resolve on re ) +RECURRING_NUDGE_LOG_PREFIX = 'Recurring Nudge' +UPGRADE_REMINDER_LOG_PREFIX = 'Upgrade Reminder' +COURSE_UPDATE_LOG_PREFIX = 'Course Update' + + @task(bind=True, default_retry_delay=30, routing_key=ROUTING_KEY) def update_course_schedules(self, **kwargs): course_key = CourseKey.from_string(kwargs['course_id']) @@ -65,8 +70,7 @@ class ScheduleMessageBaseTask(Task): current_date = resolvers._get_datetime_beginning_of_day(current_date) if not cls.is_enqueue_enabled(site): - cls.log_debug( - 'Message queuing disabled for site %s', site.domain) + cls.log_debug('Message queuing disabled for site %s', site.domain) return exclude_orgs, org_list = cls.get_course_org_filter(site) @@ -132,9 +136,11 @@ class ScheduleMessageBaseTask(Task): self, site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): msg_type = self.make_message_type(day_offset) + site = Site.objects.get(id=site_id) + _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) return self.resolver( self.async_send_task, - Site.objects.get(id=site_id), + site, deserialize(target_day_str), day_offset, bin_num, @@ -144,30 +150,43 @@ class ScheduleMessageBaseTask(Task): ).send(msg_type) def make_message_type(self, day_offset): - raise NotImplementedError() + raise NotImplementedError @task(ignore_result=True, routing_key=ROUTING_KEY) def _recurring_nudge_schedule_send(site_id, msg_str): - site = Site.objects.get(pk=site_id) - if not ScheduleConfig.current(site).deliver_recurring_nudge: - LOG.debug( - 'Recurring Nudge: Message delivery disabled for site %s', site.domain) - return + _schedule_send( + msg_str, + site_id, + 'deliver_recurring_nudge', + RECURRING_NUDGE_LOG_PREFIX, + ) - msg = Message.from_string(msg_str) - # A unique identifier for this batch of messages being sent. - set_custom_metric('send_uuid', msg.send_uuid) - # A unique identifier for this particular message. - set_custom_metric('uuid', msg.uuid) - LOG.debug('Recurring Nudge: Sending message = %s', msg_str) - ace.send(msg) + +@task(ignore_result=True, routing_key=ROUTING_KEY) +def _upgrade_reminder_schedule_send(site_id, msg_str): + _schedule_send( + msg_str, + site_id, + 'deliver_upgrade_reminder', + UPGRADE_REMINDER_LOG_PREFIX, + ) + + +@task(ignore_result=True, routing_key=ROUTING_KEY) +def _course_update_schedule_send(site_id, msg_str): + _schedule_send( + msg_str, + site_id, + 'deliver_course_update', + COURSE_UPDATE_LOG_PREFIX, + ) class ScheduleRecurringNudge(ScheduleMessageBaseTask): num_bins = resolvers.RECURRING_NUDGE_NUM_BINS enqueue_config_var = 'enqueue_recurring_nudge' - log_prefix = 'Scheduled Nudge' + log_prefix = RECURRING_NUDGE_LOG_PREFIX resolver = resolvers.ScheduleStartResolver async_send_task = _recurring_nudge_schedule_send @@ -175,24 +194,10 @@ class ScheduleRecurringNudge(ScheduleMessageBaseTask): return message_types.RecurringNudge(abs(day_offset)) -@task(ignore_result=True, routing_key=ROUTING_KEY) -def _upgrade_reminder_schedule_send(site_id, msg_str): - site = Site.objects.get(pk=site_id) - if not ScheduleConfig.current(site).deliver_upgrade_reminder: - return - - msg = Message.from_string(msg_str) - # A unique identifier for this batch of messages being sent. - set_custom_metric('send_uuid', msg.send_uuid) - # A unique identifier for this particular message. - set_custom_metric('uuid', msg.uuid) - ace.send(msg) - - class ScheduleUpgradeReminder(ScheduleMessageBaseTask): num_bins = resolvers.UPGRADE_REMINDER_NUM_BINS enqueue_config_var = 'enqueue_upgrade_reminder' - log_prefix = 'Course Update' + log_prefix = UPGRADE_REMINDER_LOG_PREFIX resolver = resolvers.UpgradeReminderResolver async_send_task = _upgrade_reminder_schedule_send @@ -200,27 +205,51 @@ class ScheduleUpgradeReminder(ScheduleMessageBaseTask): return message_types.UpgradeReminder() - -@task(ignore_result=True, routing_key=ROUTING_KEY) -def _course_update_schedule_send(site_id, msg_str): - site = Site.objects.get(pk=site_id) - if not ScheduleConfig.current(site).deliver_course_update: - return - - msg = Message.from_string(msg_str) - # A unique identifier for this batch of messages being sent. - set_custom_metric('send_uuid', msg.send_uuid) - # A unique identifier for this particular message. - set_custom_metric('uuid', msg.uuid) - ace.send(msg) - - class ScheduleCourseUpdate(ScheduleMessageBaseTask): num_bins = resolvers.COURSE_UPDATE_NUM_BINS enqueue_config_var = 'enqueue_course_update' - log_prefix = 'Course Update' + log_prefix = COURSE_UPDATE_LOG_PREFIX resolver = resolvers.CourseUpdateResolver async_send_task = _course_update_schedule_send def make_message_type(self, day_offset): return message_types.CourseUpdate() + + +def _schedule_send(msg_str, site_id, delivery_config_var, log_prefix): + if _is_delivery_enabled(site_id, delivery_config_var, log_prefix): + msg = Message.from_string(msg_str) + _annonate_send_task_for_monitoring(msg) + LOG.debug('%s: Sending message = %s', log_prefix, msg_str) + ace.send(msg) + + +def _is_delivery_enabled(site_id, delivery_config_var, log_prefix): + site = Site.objects.get(pk=site_id) + if getattr(ScheduleConfig.current(site), delivery_config_var, False): + return True + else: + LOG.debug('%s: Message delivery disabled for site %s', log_prefix, site.domain) + + +def _annotate_for_monitoring(message_type, site, bin_num, target_day_str, day_offset): + # This identifies the type of message being sent, for example: schedules.recurring_nudge3. + set_custom_metric('message_name', '{0}.{1}'.format(message_type.app_label, message_type.name)) + # The domain name of the site we are sending the message for. + set_custom_metric('site', site.domain) + # This is the "bin" of data being processed. We divide up the work into chunks so that we don't tie up celery + # workers for too long. This could help us identify particular bins that are problematic. + set_custom_metric('bin', bin_num) + # The date we are processing data for. + set_custom_metric('target_day', target_day_str) + # The number of days relative to the current date to process data for. + set_custom_metric('day_offset', day_offset) + # A unique identifier for this batch of messages being sent. + set_custom_metric('send_uuid', message_type.uuid) + + +def _annonate_send_task_for_monitoring(msg): + # A unique identifier for this batch of messages being sent. + set_custom_metric('send_uuid', msg.send_uuid) + # A unique identifier for this particular message. + set_custom_metric('uuid', msg.uuid) From 58bff7ed8ca8ce6f8e733c2e261e501aef8accb5 Mon Sep 17 00:00:00 2001 From: Gabe Mulley Date: Tue, 24 Oct 2017 08:51:09 -0400 Subject: [PATCH 29/29] update docstrings, DRY up schedules_for_bin --- .../core/djangoapps/schedules/resolvers.py | 144 ++++++++++-------- 1 file changed, 79 insertions(+), 65 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index b424788d3b..3266d9b3ec 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -40,16 +40,29 @@ COURSE_UPDATE_NUM_BINS = DEFAULT_NUM_BINS @attr.s class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): """ - Starts num_bins number of async tasks, each of which sends emails to an equal group of learners. + Identifies learners to send messages to, pulls all needed context and sends a message to each learner. + + Note that for performance reasons, it actually enqueues a task to send the message instead of sending the message + directly. Arguments: + async_send_task -- celery task function that sends the message site -- Site object that filtered Schedules will be a part of - current_date -- datetime that will be used (with time zeroed-out) as the current date in the queries - async_send_task -- celery task function which this resolver will call out to + target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under + day_offset -- int number of days relative to the Schedule's schedule_date_field that we are targeting + bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num + org_list -- list of course_org names (strings) that the returned Schedules must or must not be in + (default: None) + exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in + org_list or strictly include (False) them (default: False) + override_recipient_email -- string email address that should receive all emails instead of the normal + recipient. (default: None) Static attributes: + schedule_date_field -- the name of the model field that represents the date that offsets should be computed + relative to. For example, if this resolver finds schedules that started 7 days ago + this variable should be set to "start". num_bins -- the int number of bins to split the users into - enqueue_config_var -- the string field name of the config variable on ScheduleConfig to check before enqueuing """ async_send_task = attr.ib() site = attr.ib() @@ -87,14 +100,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): Returns Schedules with the target_date, related to Users whose id matches the bin_num, and filtered by org_list. Arguments: - schedule_date_field -- string field name to query on the User's Schedule model - current_datetime -- datetime that will be used as "right now" in the query - target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under - bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num - num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) - org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) - exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list - or strictly include (False) them (default: False) order_by -- string for field to sort the resulting Schedules by """ target_day = _get_datetime_beginning_of_day(self.target_datetime) @@ -148,6 +153,44 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): return schedules + def schedules_for_bin(self): + schedules = self.get_schedules_with_target_date_by_bin_and_orgs() + template_context = get_base_template_context(self.site) + + for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): + user_schedules = list(user_schedules) + course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] + + # This is used by the bulk email optout policy + template_context['course_ids'] = course_id_strs + + first_schedule = user_schedules[0] + template_context.update(self.get_template_context(user, user_schedules)) + + # Information for including upsell messaging in template. + _add_upsell_button_information_to_template_context( + user, first_schedule, template_context) + + yield (user, first_schedule.enrollment.course.language, template_context) + + def get_template_context(self, user, user_schedules): + """ + Given a user and their schedules, build the context needed to render the template for this message. + + Arguments: + user -- the User who will be receiving the message + user_schedules -- a list of Schedule objects representing all of their schedules that should be covered by + this message. For example, when a user enrolls in multiple courses on the same day, we + don't want to send them multiple reminder emails. Instead this list would have multiple + elements, allowing us to send a single message for all of the courses. + + Returns: + dict: This dict must be JSON serializable (no datetime objects!). When rendering the message templates it + it will be used as the template context. Note that it will also include several default values that + injected into all template contexts. See `get_base_template_context` for more information. + """ + return {} + class ScheduleStartResolver(BinnedSchedulesBaseResolver): """ @@ -157,30 +200,14 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver): schedule_date_field = 'start' num_bins = RECURRING_NUDGE_NUM_BINS - def schedules_for_bin(self): - - schedules = self.get_schedules_with_target_date_by_bin_and_orgs() - template_context = get_base_template_context(self.site) - - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] - - first_schedule = user_schedules[0] - template_context.update({ - 'student_name': user.profile.name, - 'course_name': first_schedule.enrollment.course.display_name, - 'course_url': absolute_url(self.site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), - - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, - }) - - # Information for including upsell messaging in template. - _add_upsell_button_information_to_template_context( - user, first_schedule, template_context) - - yield (user, first_schedule.enrollment.course.language, template_context) + def get_template_context(self, user, user_schedules): + first_schedule = user_schedules[0] + return { + 'course_name': first_schedule.enrollment.course.display_name, + 'course_url': absolute_url( + self.site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)]) + ), + } def _get_datetime_beginning_of_day(dt): @@ -198,33 +225,18 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver): schedule_date_field = 'upgrade_deadline' num_bins = UPGRADE_REMINDER_NUM_BINS - def schedules_for_bin(self): - schedules = self.get_schedules_with_target_date_by_bin_and_orgs() - - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] - - first_schedule = user_schedules[0] - template_context = get_base_template_context(self.site) - template_context.update({ - 'student_name': user.profile.name, - 'course_links': [ - { - 'url': absolute_url(self.site, reverse('course_root', args=[str(s.enrollment.course_id)])), - 'name': s.enrollment.course.display_name - } for s in user_schedules - ], - 'first_course_name': first_schedule.enrollment.course.display_name, - 'cert_image': absolute_url(self.site, static('course_experience/images/verified-cert.png')), - - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, - }) - - _add_upsell_button_information_to_template_context(user, first_schedule, template_context) - - yield (user, first_schedule.enrollment.course.language, template_context) + def get_template_context(self, user, user_schedules): + first_schedule = user_schedules[0] + return { + 'course_links': [ + { + 'url': absolute_url(self.site, reverse('course_root', args=[str(s.enrollment.course_id)])), + 'name': s.enrollment.course.display_name + } for s in user_schedules + ], + 'first_course_name': first_schedule.enrollment.course.display_name, + 'cert_image': absolute_url(self.site, static('course_experience/images/verified-cert.png')), + } def _add_upsell_button_information_to_template_context(user, schedule, template_context): @@ -269,6 +281,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): order_by='enrollment__course', ) + template_context = get_base_template_context(self.site) for schedule in schedules: enrollment = schedule.enrollment try: @@ -279,11 +292,12 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver): user = enrollment.user course_id_str = str(enrollment.course_id) - template_context = get_base_template_context(self.site) template_context.update({ 'student_name': user.profile.name, 'course_name': schedule.enrollment.course.display_name, - 'course_url': absolute_url(self.site, reverse('course_root', args=[str(schedule.enrollment.course_id)])), + 'course_url': absolute_url( + self.site, reverse('course_root', args=[str(schedule.enrollment.course_id)]) + ), 'week_num': week_num, 'week_summary': week_summary,