Merge pull request #16334 from edx/cale/move-org-query-to-resolver

Cale/move org query to resolver

openedx/core/djangoapps/content/course_overviews/tests/factories.py

 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

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:

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)

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
@@ -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,6 +148,35 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver):
 
         return schedules
 
+    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.
+
+        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')
+            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)
+                return schedules.exclude(enrollment__course__org__in=not_orgs)
+            elif not isinstance(org_list, list):
+                return schedules.filter(enrollment__course__org=org_list)
+        except SiteConfiguration.DoesNotExist:
+            return schedules
+
+        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()
         template_context = get_base_template_context(self.site)

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)
 

openedx/core/djangoapps/schedules/tests/test_resolvers.py

+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.data(
+        'course1'
+    )
+    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()
+        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(
+        (['course1', 'course2'], ['course1', 'course2'])
+    )
+    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},
+        )
+        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)

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)