From 9876f597e6bd489d6ae44710fe8e7d63acdfa7e9 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 4 Oct 2017 17:35:00 -0400 Subject: [PATCH] Refactor common task querying into a separate func Address some of Cale's PR comments Combine query functions into one. No debug logging Pull int variables out into static class variables Mixin needs to call super __init__ too Remove multi-course copy from upgrade reminder Address Cale's round 2 comments --- .../schedules/management/commands/__init__.py | 99 +------------- .../commands/send_recurring_nudge.py | 25 +--- .../commands/send_upgrade_reminder.py | 30 +---- .../management/commands/tests/test_base.py | 116 +--------------- .../tests/test_send_recurring_nudge.py | 18 +-- .../tests/test_send_upgrade_reminder.py | 18 +-- .../core/djangoapps/schedules/resolvers.py | 120 +++++++++++++++++ openedx/core/djangoapps/schedules/tasks.py | 126 ++++++++++-------- .../edx_ace/upgradereminder/email/body.html | 59 +++----- .../edx_ace/upgradereminder/email/body.txt | 12 -- .../upgradereminder/email/from_name.txt | 4 - .../edx_ace/upgradereminder/email/subject.txt | 4 - .../schedules/tests/test_resolvers.py | 110 +++++++++++++++ openedx/core/djangoapps/schedules/utils.py | 14 ++ 14 files changed, 367 insertions(+), 388 deletions(-) create mode 100644 openedx/core/djangoapps/schedules/resolvers.py create mode 100644 openedx/core/djangoapps/schedules/tests/test_resolvers.py create mode 100644 openedx/core/djangoapps/schedules/utils.py diff --git a/openedx/core/djangoapps/schedules/management/commands/__init__.py b/openedx/core/djangoapps/schedules/management/commands/__init__.py index d39d4f01eb..3be3b9f1a9 100644 --- a/openedx/core/djangoapps/schedules/management/commands/__init__.py +++ b/openedx/core/djangoapps/schedules/management/commands/__init__.py @@ -1,105 +1,14 @@ import datetime -import logging import pytz from django.contrib.sites.models import Site from django.core.management.base import BaseCommand -from edx_ace.recipient_resolver import RecipientResolver -from edx_ace.utils.date import serialize -from openedx.core.djangoapps.schedules.models import ScheduleConfig -from openedx.core.djangoapps.schedules.tasks import DEFAULT_NUM_BINS -from openedx.core.djangoapps.site_configuration.models import SiteConfiguration +from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin -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): - def __init__(self, *args, **kwargs): - self.log_prefix = self.__class__.__name__ - - def log_debug(self, message, *args, **kwargs): - LOG.debug(self.log_prefix + ': ' + message, *args, **kwargs) - - -class BinnedSchedulesBaseResolver(RecipientResolver, PrefixedDebugLoggerMixin): - """ - Starts num_bins number of async tasks, each of which sends emails to an equal group of learners. - """ - def __init__(self, site, current_date, *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 = None # define in subclasses - self.num_bins = DEFAULT_NUM_BINS - self.enqueue_config_var = None # define in subclasses - self.log_prefix = self.__class__.__name__ - - 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 - - -class SendEmailBaseCommand(BaseCommand, PrefixedDebugLoggerMixin): - def __init__(self, *args, **kwargs): - super(SendEmailBaseCommand, self).__init__(*args, **kwargs) - self.resolver_class = BinnedSchedulesBaseResolver - self.log_prefix = self.__class__.__name__ +class SendEmailBaseCommand(PrefixedDebugLoggerMixin, BaseCommand): + resolver_class = None # define in subclass def add_arguments(self, parser): parser.add_argument( @@ -130,4 +39,4 @@ class SendEmailBaseCommand(BaseCommand, PrefixedDebugLoggerMixin): return self.resolver_class(site, current_date) def send_emails(self, resolver, *args, **options): - resolver.send(0, options.get('override_recipient_email')) + pass # define in subclass 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 26902bbaab..5f3b316e60 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_recurring_nudge.py @@ -1,29 +1,12 @@ -from __future__ import print_function - -import logging - -from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand, BinnedSchedulesBaseResolver -from openedx.core.djangoapps.schedules.tasks import RECURRING_NUDGE_NUM_BINS, recurring_nudge_schedule_bin - -LOG = logging.getLogger(__name__) - - -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.async_send_task = recurring_nudge_schedule_bin - self.num_bins = RECURRING_NUDGE_NUM_BINS - self.log_prefix = 'Scheduled Nudge' - self.enqueue_config_var = 'enqueue_recurring_nudge' +from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand +from openedx.core.djangoapps.schedules.resolvers import ScheduleStartResolver class Command(SendEmailBaseCommand): + resolver_class = ScheduleStartResolver + def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) - self.resolver_class = ScheduleStartResolver self.log_prefix = 'Scheduled Nudge' def send_emails(self, resolver, *args, **options): 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 9d5956d013..c251c85275 100644 --- a/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/send_upgrade_reminder.py @@ -1,35 +1,13 @@ -from __future__ import print_function - -import logging - -from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand, BinnedSchedulesBaseResolver -from openedx.core.djangoapps.schedules.tasks import ( - UPGRADE_REMINDER_NUM_BINS, - upgrade_reminder_schedule_bin -) - - -LOG = logging.getLogger(__name__) - - -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.async_send_task = upgrade_reminder_schedule_bin - self.num_bins = UPGRADE_REMINDER_NUM_BINS - self.log_prefix = 'Upgrade Reminder' - self.enqueue_config_var = 'enqueue_upgrade_reminder' +from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand +from openedx.core.djangoapps.schedules.resolvers import UpgradeReminderResolver class Command(SendEmailBaseCommand): + resolver_class = UpgradeReminderResolver + def __init__(self, *args, **kwargs): super(Command, self).__init__(*args, **kwargs) - self.resolver_class = UpgradeReminderResolver self.log_prefix = 'Upgrade Reminder' def send_emails(self, resolver, *args, **options): - logging.basicConfig(level=logging.DEBUG) resolver.send(2, options.get('override_recipient_email')) 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 e2cb00a870..ce75828775 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py @@ -4,116 +4,13 @@ from unittest import skipUnless import ddt import pytz from django.conf import settings -from mock import patch, Mock +from mock import patch -from openedx.core.djangoapps.schedules.management.commands import ( - DEFAULT_NUM_BINS, - SendEmailBaseCommand, - BinnedSchedulesBaseResolver -) -from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory -from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory +from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand +from openedx.core.djangoapps.site_configuration.tests.factories import 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): - if site is None: - site = self.site - if current_date is None: - current_date = datetime.datetime.now() - resolver = BinnedSchedulesBaseResolver(self.site, current_date) - 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) - - @ddt.ddt @skip_unless_lms @skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS, @@ -123,7 +20,7 @@ class TestSendEmailBaseCommand(CacheIsolationTestCase): self.command = SendEmailBaseCommand() def test_init_resolver_class(self): - assert self.command.resolver_class == BinnedSchedulesBaseResolver + assert self.command.resolver_class is None def test_make_resolver(self): with patch.object(self.command, 'resolver_class') as resolver_class: @@ -134,11 +31,6 @@ class TestSendEmailBaseCommand(CacheIsolationTestCase): datetime.datetime(2017, 9, 29, tzinfo=pytz.UTC) ) - def test_send_emails(self): - resolver = Mock() - self.command.send_emails(resolver, override_recipient_email='foo@example.com') - resolver.send.assert_called_once_with(0, 'foo@example.com') - def test_handle(self): with patch.object(self.command, 'make_resolver') as make_resolver: make_resolver.return_value = 'resolver' 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 3b83a1d952..4dcec134c7 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 @@ -14,7 +14,7 @@ from mock import Mock, patch from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import CourseLocator -from openedx.core.djangoapps.schedules import tasks +from openedx.core.djangoapps.schedules import resolvers, tasks from openedx.core.djangoapps.schedules.management.commands import send_recurring_nudge as nudge from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory, ScheduleFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory @@ -40,7 +40,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): self.site_config = SiteConfigurationFactory.create(site=site) ScheduleConfigFactory.create(site=self.site_config.site) - @patch.object(nudge, 'ScheduleStartResolver') + @patch.object(nudge.Command, 'resolver_class') def test_handle(self, mock_resolver): test_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) nudge.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) @@ -50,7 +50,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): mock_resolver().send.assert_any_call(day, None) @patch.object(tasks, 'ace') - @patch.object(nudge, 'recurring_nudge_schedule_bin') + @patch.object(resolvers.ScheduleStartResolver, 'async_send_task') def test_resolver_send(self, mock_schedule_bin, mock_ace): current_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) nudge.ScheduleStartResolver(self.site_config.site, current_time).send(-3) @@ -80,8 +80,9 @@ class TestSendRecurringNudge(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(25): - for b in range(tasks.RECURRING_NUDGE_NUM_BINS): + for b in range(tasks.RECURRING_NUDGE_NUM_BINS): + # waffle flag takes an extra query before it is cached + with self.assertNumQueries(2 if b == 0 else 1): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=b, org_list=[schedules[0].enrollment.course.org], @@ -100,8 +101,9 @@ class TestSendRecurringNudge(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(25): - for b in range(tasks.RECURRING_NUDGE_NUM_BINS): + for b in range(tasks.RECURRING_NUDGE_NUM_BINS): + # waffle flag takes an extra query before it is cached + with self.assertNumQueries(2 if b == 0 else 1): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=b, org_list=[schedule.enrollment.course.org], @@ -124,7 +126,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(nudge, 'recurring_nudge_schedule_bin') + @patch.object(resolvers.ScheduleStartResolver, 'async_send_task') def test_enqueue_disabled(self, mock_schedule_bin, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_recurring_nudge=False) 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 329bcdd065..7c4507b144 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 @@ -14,7 +14,7 @@ from mock import Mock, patch from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import CourseLocator -from openedx.core.djangoapps.schedules import tasks +from openedx.core.djangoapps.schedules import resolvers, tasks from openedx.core.djangoapps.schedules.management.commands import send_upgrade_reminder as reminder from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory, ScheduleFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory @@ -40,7 +40,7 @@ class TestUpgradeReminder(CacheIsolationTestCase): self.site_config = SiteConfigurationFactory.create(site=site) ScheduleConfigFactory.create(site=self.site_config.site) - @patch.object(reminder, 'UpgradeReminderResolver') + @patch.object(reminder.Command, 'resolver_class') def test_handle(self, mock_resolver): test_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) reminder.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) @@ -49,7 +49,7 @@ class TestUpgradeReminder(CacheIsolationTestCase): mock_resolver().send.assert_any_call(2, None) @patch.object(tasks, 'ace') - @patch.object(reminder, 'upgrade_reminder_schedule_bin') + @patch.object(resolvers.UpgradeReminderResolver, 'async_send_task') def test_resolver_send(self, mock_schedule_bin, mock_ace): current_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) test_time = current_time + datetime.timedelta(days=2) @@ -81,8 +81,9 @@ class TestUpgradeReminder(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(25): - for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): + for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): + # waffle flag takes an extra query before it is cached + with self.assertNumQueries(2 if b == 0 else 1): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=b, org_list=[schedules[0].enrollment.course.org], @@ -101,8 +102,9 @@ class TestUpgradeReminder(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(25): - for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): + for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): + # waffle flag takes an extra query before it is cached + with self.assertNumQueries(2 if b == 0 else 1): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=b, org_list=[schedule.enrollment.course.org], @@ -125,7 +127,7 @@ class TestUpgradeReminder(CacheIsolationTestCase): self.assertFalse(mock_ace.send.called) @patch.object(tasks, 'ace') - @patch.object(reminder, 'upgrade_reminder_schedule_bin') + @patch.object(resolvers.UpgradeReminderResolver, 'async_send_task') def test_enqueue_disabled(self, mock_schedule_bin, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_upgrade_reminder=False) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py new file mode 100644 index 0000000000..bc86e1c5d8 --- /dev/null +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -0,0 +1,120 @@ +import datetime + +from edx_ace.recipient_resolver import RecipientResolver +from edx_ace.utils.date import serialize + +from openedx.core.djangoapps.schedules.models import ScheduleConfig +from openedx.core.djangoapps.schedules.tasks import ( + DEFAULT_NUM_BINS, + RECURRING_NUDGE_NUM_BINS, + UPGRADE_REMINDER_NUM_BINS, + recurring_nudge_schedule_bin, + upgrade_reminder_schedule_bin +) +from openedx.core.djangoapps.schedules.utils import PrefixedDebugLoggerMixin +from openedx.core.djangoapps.site_configuration.models import SiteConfiguration + + +class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): + """ + Starts num_bins number of async tasks, each of which sends emails to an equal group of learners. + + 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 + + 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): + super(BinnedSchedulesBaseResolver, self).__init__(*args, **kwargs) + self.site = site + self.current_date = current_date.replace(hour=0, minute=0, second=0) + + 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 + + +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' + + def __init__(self, *args, **kwargs): + super(ScheduleStartResolver, self).__init__(*args, **kwargs) + self.log_prefix = 'Scheduled Nudge' + + +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' + + def __init__(self, *args, **kwargs): + super(UpgradeReminderResolver, self).__init__(*args, **kwargs) + self.log_prefix = 'Upgrade Reminder' diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index a9cdcfcd51..2e8dcff8f9 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -181,36 +181,16 @@ def recurring_nudge_schedule_bin( def _recurring_nudge_schedules_for_bin(target_day, bin_num, org_list, exclude_orgs=False): beginning_of_day = target_day.replace(hour=0, minute=0, second=0) - users = User.objects.filter( - courseenrollment__schedule__start__gte=beginning_of_day, - courseenrollment__schedule__start__lt=beginning_of_day + datetime.timedelta(days=1), - courseenrollment__is_active=True, - ).annotate( - first_schedule=Min('courseenrollment__schedule__start') - ).annotate( - id_mod=F('id') % RECURRING_NUDGE_NUM_BINS - ).filter( - id_mod=bin_num + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='start', + target_date=beginning_of_day, + bin_num=bin_num, + num_bins=RECURRING_NUDGE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, ) - schedules = Schedule.objects.select_related( - 'enrollment__user__profile', - 'enrollment__course', - ).filter( - enrollment__user__in=users, - start__gte=beginning_of_day, - start__lt=beginning_of_day + datetime.timedelta(days=1), - enrollment__is_active=True, - ).order_by('enrollment__user__id') - - 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('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) @@ -265,36 +245,17 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): def _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_orgs=False): beginning_of_day = target_day.replace(hour=0, minute=0, second=0) - users = User.objects.filter( - courseenrollment__schedule__upgrade_deadline__gte=beginning_of_day, - courseenrollment__schedule__upgrade_deadline__lt=beginning_of_day + datetime.timedelta(days=1), - courseenrollment__is_active=True, - ).annotate( - first_schedule=Min('courseenrollment__schedule__upgrade_deadline') - ).annotate( - id_mod=F('id') % UPGRADE_REMINDER_NUM_BINS - ).filter( - id_mod=bin_num + + schedules = get_schedules_with_target_date_by_bin_and_orgs( + schedule_date_field='upgrade_deadline', + target_date=beginning_of_day, + bin_num=bin_num, + num_bins=RECURRING_NUDGE_NUM_BINS, + org_list=org_list, + exclude_orgs=exclude_orgs, ) - schedules = Schedule.objects.select_related( - 'enrollment__user__profile', - 'enrollment__course', - ).filter( - enrollment__user__in=users, - upgrade_deadline__gte=beginning_of_day, - upgrade_deadline__lt=beginning_of_day + datetime.timedelta(days=1), - enrollment__is_active=True, - ).order_by('enrollment__user__id') - - 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('Upgrade Reminder: Query = %r', schedules.query.sql_with_params()) for schedule in schedules: enrollment = schedule.enrollment @@ -327,3 +288,56 @@ def _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_o }) yield (user, first_schedule.enrollment.course.language, template_context) + + +def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_date, bin_num, num_bins=DEFAULT_NUM_BINS, + org_list=None, exclude_orgs=False): + """ + 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 + target_date -- datetime day (with zeroed-out time) 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) + """ + schedule_date_equals_target_date_filter = { + 'courseenrollment__schedule__{}__gte'.format(schedule_date_field): target_date, + 'courseenrollment__schedule__{}__lt'.format(schedule_date_field): target_date + datetime.timedelta(days=1), + } + users = User.objects.filter( + courseenrollment__is_active=True, + **schedule_date_equals_target_date_filter + ).annotate( + id_mod=F('id') % num_bins + ).filter( + id_mod=bin_num + ) + + schedule_date_equals_target_date_filter = { + '{}__gte'.format(schedule_date_field): target_date, + '{}__lt'.format(schedule_date_field): target_date + datetime.timedelta(days=1), + } + schedules = Schedule.objects.select_related( + 'enrollment__user__profile', + 'enrollment__course', + ).filter( + enrollment__user__in=users, + enrollment__is_active=True, + **schedule_date_equals_target_date_filter + ).order_by('enrollment__user__id') + + 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") + + return schedules diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html index 1746524cf3..6d02528764 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html @@ -2,22 +2,12 @@ {% load i18n %} {% block preview_text %} - {% if course_ids|length > 1 %} - {% blocktrans trimmed %} - We hope you are enjoying learning with us so far in {{ course_name }}, and other courses on - {{ platform_name }}! A verified certificate will allow you to highlight your new knowledge and - skills. It's official, and easily shareable. + {% blocktrans trimmed %} + We hope you are enjoying learning with us so far in {{ course_name }}! A verified certificate + will allow you to highlight your new knowledge and skills. It's official, and easily shareable. - Upgrade by {{ user_schedule_upgrade_deadline_time }}. - {% endblocktrans %} - {% else %} - {% blocktrans trimmed %} - We hope you are enjoying learning with us so far in {{ course_name }}! A verified certificate - will allow you to highlight your new knowledge and skills. It's official, and easily shareable. - - Upgrade by {{ user_schedule_upgrade_deadline_time }}. - {% endblocktrans %} - {% endif %} + Upgrade by {{ user_schedule_upgrade_deadline_time }}. + {% endblocktrans %} {% endblock %} {% block content %} @@ -26,33 +16,18 @@

{% trans "Upgrade now" %}

- {% if course_ids|length > 1 %} -

- {% blocktrans trimmed %} - We hope you are enjoying learning with us so far in {{ course_name }}, and - other courses on {{ platform_name }}! A verified certificate will allow you to highlight your - new knowledge and skills. It's official, and easily shareable. - {% endblocktrans %} -

-

- {% blocktrans trimmed %} - Upgrade by {{ user_schedule_upgrade_deadline_time }}. - {% endblocktrans %} -

- {% else %} -

- {% blocktrans trimmed %} - We hope you are enjoying learning with us so far in {{ course_name }}! A - verified certificate will allow you to highlight your new knowledge and skills. It's official, - and easily shareable. - {% endblocktrans %} -

-

- {% blocktrans trimmed %} - Upgrade by {{ user_schedule_upgrade_deadline_time }}. - {% endblocktrans %} -

- {% endif %} +

+ {% blocktrans trimmed %} + We hope you are enjoying learning with us so far in {{ course_name }}! A + verified certificate will allow you to highlight your new knowledge and skills. It's official, + and easily shareable. + {% endblocktrans %} +

+

+ {% blocktrans trimmed %} + Upgrade by {{ user_schedule_upgrade_deadline_time }}. + {% endblocktrans %} +

diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.txt index 0efa861d4b..3bbf29c78e 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.txt @@ -4,17 +4,6 @@ Dear {{ user_personal_address }}, {% endblocktrans %} -{% if course_ids|length > 1 %} -{% blocktrans trimmed %} - We hope you are enjoying learning with us so far in {{ course_name }}, and other courses on - {{ platform_name }}! A verified certificate will allow you to highlight your new knowledge and - skills. It's official, and easily shareable. - - Upgrade by {{ user_schedule_upgrade_deadline_time }}. -{% endblocktrans %} - -{% trans "Upgrade now at" %} <{{ dashboard_url }}> -{% else %} {% blocktrans trimmed %} We hope you are enjoying learning with us so far in {{ course_name }}! A verified certificate will allow you to highlight your new knowledge and skills. It's official, and easily shareable. @@ -23,4 +12,3 @@ Dear {{ user_personal_address }}, {% endblocktrans %} {% trans "Upgrade now at" %} <{{ course_url }}> -{% endif %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/from_name.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/from_name.txt index d25f5d3e4a..08e86bddd0 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/from_name.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/from_name.txt @@ -1,5 +1 @@ -{% if course_ids|length > 1 %} -{{ platform_name }} -{% else %} {{ course_name }} -{% endif %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/subject.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/subject.txt index b207b935fe..7daea3543a 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/subject.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/subject.txt @@ -1,7 +1,3 @@ {% load i18n %} -{% if course_ids|length > 1 %} -{% blocktrans %}Upgrade to earn a verified certificate on {{ platform_name }}{% endblocktrans %} -{% else %} {% blocktrans %}Upgrade to earn a verified certificate in {{ course_name }}{% endblocktrans %} -{% endif %} diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py new file mode 100644 index 0000000000..0854c52b56 --- /dev/null +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -0,0 +1,110 @@ +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.tasks 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): + if site is None: + site = self.site + if current_date is None: + current_date = datetime.datetime.now() + resolver = BinnedSchedulesBaseResolver(self.site, current_date) + 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/utils.py b/openedx/core/djangoapps/schedules/utils.py new file mode 100644 index 0000000000..5705d95d22 --- /dev/null +++ b/openedx/core/djangoapps/schedules/utils.py @@ -0,0 +1,14 @@ +import logging + +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): + def __init__(self, *args, **kwargs): + super(PrefixedDebugLoggerMixin, self).__init__(*args, **kwargs) + self.log_prefix = self.__class__.__name__ + + def log_debug(self, message, *args, **kwargs): + LOG.debug(self.log_prefix + ': ' + message, *args, **kwargs)