From fa7b9a132a8d94764e42b3af501cfc6badb3e0ed Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 23 Oct 2017 21:31:20 -0400 Subject: [PATCH 1/2] Move the org_list and exclude code into the resolvers --- .../course_overviews/tests/factories.py | 3 - .../management/commands/tests/test_base.py | 3 +- .../tests/test_send_recurring_nudge.py | 97 ++++++++++--------- .../tests/test_send_upgrade_reminder.py | 90 +++++++++-------- .../core/djangoapps/schedules/resolvers.py | 36 ++++++- openedx/core/djangoapps/schedules/tasks.py | 44 +-------- .../schedules/tests/test_resolvers.py | 57 +++++++++++ .../djangoapps/schedules/tests/test_tasks.py | 30 +----- 8 files changed, 191 insertions(+), 169 deletions(-) create mode 100644 openedx/core/djangoapps/schedules/tests/test_resolvers.py diff --git a/openedx/core/djangoapps/content/course_overviews/tests/factories.py b/openedx/core/djangoapps/content/course_overviews/tests/factories.py index f412fda062..4b3a9bd370 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/factories.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/factories.py @@ -1,7 +1,6 @@ import json import factory -from django.utils.timezone import get_current_timezone from factory.django import DjangoModelFactory from ..models import CourseOverview @@ -15,8 +14,6 @@ class CourseOverviewFactory(DjangoModelFactory): version = CourseOverview.VERSION pre_requisite_courses = [] - start = factory.Faker('past_datetime', tzinfo=get_current_timezone()) - end = factory.Faker('future_datetime', tzinfo=get_current_timezone()) org = 'edX' @factory.lazy_attribute 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 3cb6ebc59f..3a53dbbd8a 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_base.py @@ -7,7 +7,7 @@ from django.conf import settings from mock import patch from openedx.core.djangoapps.schedules.management.commands import SendEmailBaseCommand -from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory +from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory, SiteConfigurationFactory from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms @@ -19,6 +19,7 @@ class TestSendEmailBaseCommand(CacheIsolationTestCase): def setUp(self): self.command = SendEmailBaseCommand() self.site = SiteFactory() + self.site_config = SiteConfigurationFactory.create(site=self.site) def test_handle(self): with patch.object(self.command, 'send_emails') as send_emails: 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 eb40d4dbbc..eba4348053 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 @@ -35,6 +35,9 @@ NUM_QUERIES_NO_MATCHING_SCHEDULES = 2 # 3) Query all course modes for all courses in returned schedules NUM_QUERIES_WITH_MATCHES = NUM_QUERIES_NO_MATCHING_SCHEDULES + 1 +# 4) Load the non-matching site configurations +NUM_QUERIES_NO_ORG_LIST = 1 + NUM_COURSE_MODES_QUERIES = 1 @@ -79,11 +82,11 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): 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), + (self.site_config.site.id, serialize(test_day), -3, 0, 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), + (self.site_config.site.id, serialize(test_day), -3, resolvers.RECURRING_NUDGE_NUM_BINS - 1, None), retry=False, ) self.assertFalse(mock_ace.send.called) @@ -104,16 +107,16 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): test_datetime = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) for b in range(resolvers.RECURRING_NUDGE_NUM_BINS): - expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES + expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES + NUM_QUERIES_NO_ORG_LIST if b in bins_in_use: # to fetch course modes for valid schedules expected_queries += NUM_COURSE_MODES_QUERIES with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): - 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], - ) + + tasks.ScheduleRecurringNudge.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, + )) self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count) self.assertFalse(mock_ace.send.called) @@ -129,11 +132,10 @@ 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(resolvers.RECURRING_NUDGE_NUM_BINS): - with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): - 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], - ) + with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES + NUM_QUERIES_NO_ORG_LIST, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleRecurringNudge.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b + )) # There is no database constraint that enforces that enrollment.course_id points # to a valid CourseOverview object. However, in that case, schedules isn't going @@ -147,20 +149,23 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): def test_send_after_course_end(self, mock_schedule_send): user1 = UserFactory.create(id=resolvers.RECURRING_NUDGE_NUM_BINS) + schedule_start = datetime.datetime(2017, 8, 3, 20, 34, 30, tzinfo=pytz.UTC) + day_command_is_run = schedule_start + datetime.timedelta(days=3) schedule = ScheduleFactory.create( - start=datetime.datetime(2017, 8, 3, 20, 34, 30, tzinfo=pytz.UTC), + start=schedule_start, enrollment__user=user1, ) - schedule.enrollment.course = CourseOverviewFactory() - schedule.enrollment.course.end = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=1) + + schedule.enrollment.course.start = schedule_start - datetime.timedelta(days=30) + schedule.enrollment.course.end = day_command_is_run - datetime.timedelta(days=1) + schedule.enrollment.course.save() test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - 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], - ) + tasks.ScheduleRecurringNudge.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0, + )) self.assertFalse(mock_schedule_send.apply_async.called) @@ -189,19 +194,17 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): @patch.object(tasks, 'ace') @patch.object(tasks.ScheduleRecurringNudge, 'async_send_task') @ddt.data( - ((['filtered_org'], False, 1)), - ((['filtered_org'], True, 2)) + ((['filtered_org'], [], 1)), + (([], ['filtered_org'], 2)) ) @ddt.unpack - def test_site_config(self, org_list, exclude_orgs, expected_message_count, mock_schedule_send, mock_ace): + def test_site_config(self, this_org_list, other_org_list, expected_message_count, mock_schedule_send, mock_ace): filtered_org = 'filtered_org' unfiltered_org = 'unfiltered_org' - site1 = SiteFactory.create(domain='foo1.bar', name='foo1.bar') - limited_config = SiteConfigurationFactory.create(values={'course_org_filter': [filtered_org]}, site=site1) - site2 = SiteFactory.create(domain='foo2.bar', name='foo2.bar') - unlimited_config = SiteConfigurationFactory.create(values={'course_org_filter': []}, site=site2) + this_config = SiteConfigurationFactory.create(values={'course_org_filter': this_org_list}) + other_config = SiteConfigurationFactory.create(values={'course_org_filter': other_org_list}) - for config in (limited_config, unlimited_config): + for config in (this_config, other_config): ScheduleConfigFactory.create(site=config.site) user1 = UserFactory.create(id=resolvers.RECURRING_NUDGE_NUM_BINS) @@ -225,11 +228,15 @@ 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.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, - ) + + expected_queries = NUM_QUERIES_WITH_MATCHES + if not this_org_list: + expected_queries += NUM_QUERIES_NO_ORG_LIST + + with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleRecurringNudge.apply(kwargs=dict( + site_id=this_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0 + )) self.assertEqual(mock_schedule_send.apply_async.call_count, expected_message_count) self.assertFalse(mock_ace.send.called) @@ -249,12 +256,11 @@ 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.delay( - self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_NO_ORG_LIST, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleRecurringNudge.apply(kwargs=dict( + site_id=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], - ) + )) self.assertEqual(mock_schedule_send.apply_async.call_count, 1) self.assertFalse(mock_ace.send.called) @@ -288,11 +294,11 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): 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): - 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], - ) + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_NO_ORG_LIST, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleRecurringNudge.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, + bin_num=self._calculate_bin_for_user(user), + )) self.assertEqual(len(sent_messages), 1) @@ -423,13 +429,12 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) - bin_task.delay( - self.site_config.site.id, + bin_task.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=bin_task_params[0], day_offset=bin_task_params[1], bin_num=self._calculate_bin_for_user(bin_task_params[2]), - org_list=[bin_task_params[3]] - ) + )) return sent_messages 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 775fa4b9a2..95a9bc3f5c 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 @@ -36,6 +36,7 @@ SCHEDULES_QUERY = 1 COURSE_MODES_QUERY = 1 GLOBAL_DEADLINE_SWITCH_QUERY = 1 COMMERCE_CONFIG_QUERY = 1 +NUM_QUERIES_NO_ORG_LIST = 1 NUM_QUERIES_NO_MATCHING_SCHEDULES = SITE_QUERY + SCHEDULES_QUERY @@ -114,11 +115,11 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): 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), + (self.site_config.site.id, serialize(test_day), 2, 0, 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), + (self.site_config.site.id, serialize(test_day), 2, resolvers.UPGRADE_REMINDER_NUM_BINS - 1, None), retry=False, ) self.assertFalse(mock_ace.send.called) @@ -135,13 +136,10 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): ) for i in range(schedule_count) ] - bins_in_use = frozenset((s.enrollment.user.id % resolvers.UPGRADE_REMINDER_NUM_BINS) for s in schedules) + bins_in_use = frozenset((self._calculate_bin_for_user(s.enrollment.user)) for s in schedules) is_first_match = True - course_switch_queries = len(set(s.enrollment.course.id for s in schedules)) - - test_datetime = datetime.datetime(upgrade_deadline.year, upgrade_deadline.month, upgrade_deadline.day, 18, - tzinfo=pytz.UTC) + test_datetime = upgrade_deadline test_datetime_str = serialize(test_datetime) for b in range(resolvers.UPGRADE_REMINDER_NUM_BINS): @@ -159,11 +157,12 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): else: expected_queries = NUM_QUERIES_WITH_MATCHES + expected_queries += NUM_QUERIES_NO_ORG_LIST + with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): - 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], - ) + tasks.ScheduleUpgradeReminder.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, + )) self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count) self.assertFalse(mock_ace.send.called) @@ -180,11 +179,11 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) 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.delay( - self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, - org_list=[schedule.enrollment.course.org], - ) + + with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES + NUM_QUERIES_NO_ORG_LIST, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleUpgradeReminder.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, + )) # There is no database constraint that enforces that enrollment.course_id points # to a valid CourseOverview object. However, in that case, schedules isn't going @@ -219,19 +218,17 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): @patch.object(tasks, 'ace') @patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') @ddt.data( - ((['filtered_org'], False, 1)), - ((['filtered_org'], True, 2)) + ((['filtered_org'], [], 1)), + (([], ['filtered_org'], 2)) ) @ddt.unpack - def test_site_config(self, org_list, exclude_orgs, expected_message_count, mock_schedule_send, mock_ace): + def test_site_config(self, this_org_list, other_org_list, expected_message_count, mock_schedule_send, mock_ace): filtered_org = 'filtered_org' unfiltered_org = 'unfiltered_org' - site1 = SiteFactory.create(domain='foo1.bar', name='foo1.bar') - limited_config = SiteConfigurationFactory.create(values={'course_org_filter': [filtered_org]}, site=site1) - site2 = SiteFactory.create(domain='foo2.bar', name='foo2.bar') - unlimited_config = SiteConfigurationFactory.create(values={'course_org_filter': []}, site=site2) + this_config = SiteConfigurationFactory.create(values={'course_org_filter': this_org_list}) + other_config = SiteConfigurationFactory.create(values={'course_org_filter': other_org_list}) - for config in (limited_config, unlimited_config): + for config in (this_config, other_config): ScheduleConfigFactory.create(site=config.site) user1 = UserFactory.create(id=resolvers.UPGRADE_REMINDER_NUM_BINS) @@ -260,11 +257,14 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): test_datetime_str = serialize(test_datetime) course_switch_queries = 1 - with self.assertNumQueries(NUM_QUERIES_FIRST_MATCH + course_switch_queries, table_blacklist=WAFFLE_TABLES): - 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, - ) + expected_queries = NUM_QUERIES_FIRST_MATCH + course_switch_queries + if not this_org_list: + expected_queries += NUM_QUERIES_NO_ORG_LIST + + with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleUpgradeReminder.apply(kwargs=dict( + site_id=this_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0 + )) self.assertEqual(mock_schedule_send.apply_async.call_count, expected_message_count) self.assertFalse(mock_ace.send.called) @@ -287,13 +287,12 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_datetime_str = serialize(test_datetime) - - with self.assertNumQueries(NUM_QUERIES_FIRST_MATCH + num_courses, table_blacklist=WAFFLE_TABLES): - 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], - ) + expected_query_count = NUM_QUERIES_FIRST_MATCH + num_courses + NUM_QUERIES_NO_ORG_LIST + with self.assertNumQueries(expected_query_count, table_blacklist=WAFFLE_TABLES): + tasks.ScheduleUpgradeReminder.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, + bin_num=self._calculate_bin_for_user(user), + )) self.assertEqual(mock_schedule_send.apply_async.call_count, 1) self.assertFalse(mock_ace.send.called) @@ -340,13 +339,13 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): 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 - num_expected_queries = NUM_QUERIES_FIRST_MATCH + num_courses + num_expected_queries = NUM_QUERIES_FIRST_MATCH + NUM_QUERIES_NO_ORG_LIST + num_courses + with self.assertNumQueries(num_expected_queries, table_blacklist=WAFFLE_TABLES): - tasks.ScheduleUpgradeReminder.delay( - self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, + tasks.ScheduleUpgradeReminder.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=self._calculate_bin_for_user(user), - org_list=[schedules[0].enrollment.course.org], - ) + )) self.assertEqual(len(sent_messages), 1) @@ -416,16 +415,15 @@ class TestUpgradeReminder(SharedModuleStoreTestCase): test_datetime = future_datetime test_datetime_str = serialize(test_datetime) - bin_num = self._calculate_bin_for_user(user) sent_messages = [] 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[1]) - tasks.ScheduleUpgradeReminder.delay( - self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=bin_num, - org_list=[schedules[0].enrollment.course.org], - ) + tasks.ScheduleUpgradeReminder.apply(kwargs=dict( + site_id=self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, + bin_num=self._calculate_bin_for_user(user), + )) messages = [Message.from_string(m) for m in sent_messages] self.assertEqual(len(messages), 1) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 340dfbd9b7..4ad324c77a 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -24,6 +24,7 @@ from openedx.core.djangoapps.schedules.template_context import ( absolute_url, 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 @@ -69,8 +70,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): 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) schedule_date_field = None @@ -79,6 +78,7 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): 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) + self.exclude_orgs, self.org_list = self.get_course_org_filter() def send(self, msg_type): for (user, language, context) in self.schedules_for_bin(): @@ -153,6 +153,38 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): return schedules + 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 = self.site.configuration + 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 + + return exclude_orgs, org_list + def schedules_for_bin(self): schedules = self.get_schedules_with_target_date_by_bin_and_orgs() template_context = get_base_template_context(self.site) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 96c391f6c5..1c732db888 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -17,7 +17,6 @@ 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.site_configuration.models import SiteConfiguration LOG = logging.getLogger(__name__) @@ -73,8 +72,6 @@ class ScheduleMessageBaseTask(Task): 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): @@ -83,8 +80,6 @@ class ScheduleMessageBaseTask(Task): serialize(target_date), day_offset, bin, - org_list, - exclude_orgs, override_recipient_email, ) cls.log_debug('Launching task with args = %r', task_args) @@ -99,44 +94,11 @@ class ScheduleMessageBaseTask(Task): 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 - - 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, + self, site_id, target_day_str, day_offset, bin_num, override_recipient_email=None, ): msg_type = self.make_message_type(day_offset) - site = Site.objects.get(id=site_id) + site = Site.objects.select_related('configuration').get(id=site_id) _annotate_for_monitoring(msg_type, site, bin_num, target_day_str, day_offset) return self.resolver( self.async_send_task, @@ -144,8 +106,6 @@ class ScheduleMessageBaseTask(Task): deserialize(target_day_str), day_offset, bin_num, - org_list, - exclude_orgs=exclude_orgs, override_recipient_email=override_recipient_email, ).send(msg_type) 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..3144a45b02 --- /dev/null +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -0,0 +1,57 @@ +import datetime +from unittest import skipUnless + +import ddt +from django.conf import settings +from mock import patch, DEFAULT, Mock + +from openedx.core.djangoapps.schedules.resolvers import BinnedSchedulesBaseResolver +from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory +from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory, SiteConfigurationFactory +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(site=self.site) + self.schedule_config = ScheduleConfigFactory.create(site=self.site) + self.resolver = BinnedSchedulesBaseResolver( + async_send_task=Mock(name='async_send_task'), + site=self.site, + target_datetime=datetime.datetime.now(), + day_offset=3, + bin_num=2, + ) + + @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.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): + SiteConfigurationFactory.create( + values={'course_org_filter': course_org_filter}, + ) + exclude_orgs, org_list = self.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 index 98dcfb7ff2..bebd81689d 100644 --- a/openedx/core/djangoapps/schedules/tests/test_tasks.py +++ b/openedx/core/djangoapps/schedules/tests/test_tasks.py @@ -8,7 +8,7 @@ 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.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms @@ -21,7 +21,6 @@ class TestScheduleMessageBaseTask(CacheIsolationTestCase): 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 @@ -49,7 +48,6 @@ class TestScheduleMessageBaseTask(CacheIsolationTestCase): 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: @@ -71,29 +69,3 @@ class TestScheduleMessageBaseTask(CacheIsolationTestCase): 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): - SiteConfigurationFactory.create( - 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 07bb3ea564e736124cd6299791a3656375c1a8aa Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 24 Oct 2017 15:42:59 -0400 Subject: [PATCH 2/2] Make the exclude orgs a bit more natural by returning a filtered query --- .../core/djangoapps/schedules/resolvers.py | 20 +++------- .../schedules/tests/test_resolvers.py | 40 ++++++++++++------- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 4ad324c77a..be8a938df1 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -78,7 +78,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): 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) - self.exclude_orgs, self.org_list = self.get_course_org_filter() def send(self, msg_type): for (user, language, context) in self.schedules_for_bin(): @@ -133,11 +132,7 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): **schedule_day_equals_target_day_filter ).order_by(order_by) - 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) + schedules = self.filter_by_org(schedules) if "read_replica" in settings.DATABASES: schedules = schedules.using("read_replica") @@ -153,7 +148,7 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): return schedules - def get_course_org_filter(self): + def filter_by_org(self, schedules): """ Given the configuration of sites, get the list of orgs that should be included or excluded from this send. @@ -165,7 +160,6 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): try: site_config = self.site.configuration 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(): @@ -175,15 +169,13 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver): not_orgs.add(other) else: not_orgs.update(other) - org_list = list(not_orgs) - exclude_orgs = True + return schedules.exclude(enrollment__course__org__in=not_orgs) elif not isinstance(org_list, list): - org_list = [org_list] + return schedules.filter(enrollment__course__org=org_list) except SiteConfiguration.DoesNotExist: - org_list = None - exclude_orgs = False + return schedules - return exclude_orgs, org_list + return schedules.filter(enrollment__course__org__in=org_list) def schedules_for_bin(self): schedules = self.get_schedules_with_target_date_by_bin_and_orgs() diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py index 3144a45b02..e64844c375 100644 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -30,28 +30,40 @@ class TestBinnedSchedulesBaseResolver(CacheIsolationTestCase): bin_num=2, ) - @ddt.unpack @ddt.data( - ('course1', ['course1']), - (['course1', 'course2'], ['course1', 'course2']) + 'course1' ) - def test_get_course_org_filter_include(self, course_org_filter, expected_org_list): + def test_get_course_org_filter_equal(self, course_org_filter): self.site_config.values['course_org_filter'] = course_org_filter self.site_config.save() - exclude_orgs, org_list = self.resolver.get_course_org_filter() - assert not exclude_orgs - assert org_list == expected_org_list + mock_query = Mock() + result = self.resolver.filter_by_org(mock_query) + self.assertEqual(result, mock_query.filter.return_value) + mock_query.filter.assert_called_once_with(enrollment__course__org=course_org_filter) @ddt.unpack @ddt.data( - (None, []), - ('course1', [u'course1']), - (['course1', 'course2'], [u'course1', u'course2']) + (['course1', 'course2'], ['course1', 'course2']) ) - def test_get_course_org_filter_exclude(self, course_org_filter, expected_org_list): + def test_get_course_org_filter_include__in(self, course_org_filter, expected_org_list): + self.site_config.values['course_org_filter'] = course_org_filter + self.site_config.save() + mock_query = Mock() + result = self.resolver.filter_by_org(mock_query) + self.assertEqual(result, mock_query.filter.return_value) + mock_query.filter.assert_called_once_with(enrollment__course__org__in=expected_org_list) + + @ddt.unpack + @ddt.data( + (None, set([])), + ('course1', set([u'course1'])), + (['course1', 'course2'], set([u'course1', u'course2'])) + ) + def test_get_course_org_filter_exclude__in(self, course_org_filter, expected_org_list): SiteConfigurationFactory.create( values={'course_org_filter': course_org_filter}, ) - exclude_orgs, org_list = self.resolver.get_course_org_filter() - assert exclude_orgs - self.assertItemsEqual(org_list, expected_org_list) + mock_query = Mock() + result = self.resolver.filter_by_org(mock_query) + mock_query.exclude.assert_called_once_with(enrollment__course__org__in=expected_org_list) + self.assertEqual(result, mock_query.exclude.return_value)