Merge pull request #16327 from edx/mulby/upgrade-reminders-fix

Only send upgrade reminders to learners who are eligible to upgrade

openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py

 import datetime
-import itertools
 from copy import deepcopy
+import logging
 from unittest import skipUnless
 
 import attr
 import ddt
 import pytz
 from django.conf import settings
+from edx_ace import Message
+from freezegun import freeze_time
 from edx_ace.channel import ChannelType
 from edx_ace.test_utils import StubPolicy, patch_channels, patch_policies
 from edx_ace.utils.date import serialize
@@ -17,42 +19,71 @@ from opaque_keys.edx.locator import CourseLocator
 from course_modes.models import CourseMode
 from course_modes.tests.factories import CourseModeFactory
 from courseware.models import DynamicUpgradeDeadlineConfiguration
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 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
 from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES
-from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms, FilteredQueryCountMixin
+from openedx.core.djangolib.testing.utils import skip_unless_lms
 from student.tests.factories import UserFactory
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
 
 
-# 1) Load the current django site
-# 2) Query the schedules to find all of the template context information
-NUM_QUERIES_NO_MATCHING_SCHEDULES = 2
+SITE_QUERY = 1
+SCHEDULES_QUERY = 1
+COURSE_MODES_QUERY = 1
+GLOBAL_DEADLINE_SWITCH_QUERY = 1
+COMMERCE_CONFIG_QUERY = 1
 
-# 3) Query all course modes for all courses in returned schedules
-NUM_QUERIES_WITH_MATCHES = NUM_QUERIES_NO_MATCHING_SCHEDULES + 1
+NUM_QUERIES_NO_MATCHING_SCHEDULES = SITE_QUERY + SCHEDULES_QUERY
 
-# 1) Global dynamic deadline switch
-# 2) Course dynamic deadline switch
-# 2) E-commerce configuration
-NUM_QUERIES_WITH_DEADLINE = 3
+NUM_QUERIES_WITH_MATCHES = (
+    NUM_QUERIES_NO_MATCHING_SCHEDULES +
+    COURSE_MODES_QUERY
+)
 
-NUM_COURSE_MODES_QUERIES = 1
+NUM_QUERIES_FIRST_MATCH = (
+    NUM_QUERIES_WITH_MATCHES
+    + GLOBAL_DEADLINE_SWITCH_QUERY
+    + COMMERCE_CONFIG_QUERY
+)
+
+LOG = logging.getLogger(__name__)
 
 
 @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 TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
-    # pylint: disable=protected-access
+@freeze_time('2017-08-01 00:00:00', tz_offset=0, tick=True)
+class TestUpgradeReminder(SharedModuleStoreTestCase):
 
     ENABLED_CACHES = ['default']
 
+    @classmethod
+    def setUpClass(cls):
+        super(TestUpgradeReminder, cls).setUpClass()
+
+        cls.course = CourseFactory.create(
+            org='edX',
+            number='test',
+            display_name='Test Course',
+            self_paced=True,
+            start=datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30),
+        )
+        cls.course_overview = CourseOverview.get_from_id(cls.course.id)
+
     def setUp(self):
         super(TestUpgradeReminder, self).setUp()
 
+        CourseModeFactory(
+            course_id=self.course.id,
+            mode_slug=CourseMode.VERIFIED,
+            expiration_datetime=datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=30),
+        )
+
         ScheduleFactory.create(upgrade_deadline=datetime.datetime(2017, 8, 1, 15, 44, 30, tzinfo=pytz.UTC))
         ScheduleFactory.create(upgrade_deadline=datetime.datetime(2017, 8, 1, 17, 34, 30, tzinfo=pytz.UTC))
         ScheduleFactory.create(upgrade_deadline=datetime.datetime(2017, 8, 2, 15, 34, 30, tzinfo=pytz.UTC))
@@ -96,22 +127,37 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
     @patch.object(tasks, 'ace')
     @patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task')
     def test_schedule_bin(self, schedule_count, mock_schedule_send, mock_ace):
+        upgrade_deadline = datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=2)
         schedules = [
             ScheduleFactory.create(
-                upgrade_deadline=datetime.datetime(2017, 8, 3, 18, 44, 30, tzinfo=pytz.UTC),
-                enrollment__course__id=CourseLocator('edX', 'toy', 'Bin')
+                upgrade_deadline=upgrade_deadline,
+                enrollment__course=self.course_overview,
             ) for i in range(schedule_count)
         ]
 
         bins_in_use = frozenset((s.enrollment.user.id % resolvers.UPGRADE_REMINDER_NUM_BINS) 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(2017, 8, 3, 18, tzinfo=pytz.UTC)
+        test_datetime = datetime.datetime(upgrade_deadline.year, upgrade_deadline.month, upgrade_deadline.day, 18,
+                                          tzinfo=pytz.UTC)
         test_datetime_str = serialize(test_datetime)
+
         for b in range(resolvers.UPGRADE_REMINDER_NUM_BINS):
+            LOG.debug('Running bin %d', b)
             expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES
             if b in bins_in_use:
-                # to fetch course modes for valid schedules
-                expected_queries += NUM_COURSE_MODES_QUERIES
+                if is_first_match:
+                    expected_queries = (
+                        # Since this is the first match, we need to cache all of the config models, so we run a query
+                        # for each of those...
+                        NUM_QUERIES_FIRST_MATCH
+                        + course_switch_queries
+                    )
+                    is_first_match = False
+                else:
+                    expected_queries = NUM_QUERIES_WITH_MATCHES
 
             with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES):
                 tasks.ScheduleUpgradeReminder.delay(
@@ -194,22 +240,27 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
         ScheduleFactory.create(
             upgrade_deadline=datetime.datetime(2017, 8, 3, 17, 44, 30, tzinfo=pytz.UTC),
             enrollment__course__org=filtered_org,
+            enrollment__course__self_paced=True,
             enrollment__user=user1,
         )
         ScheduleFactory.create(
             upgrade_deadline=datetime.datetime(2017, 8, 3, 17, 44, 30, tzinfo=pytz.UTC),
             enrollment__course__org=unfiltered_org,
+            enrollment__course__self_paced=True,
             enrollment__user=user1,
         )
         ScheduleFactory.create(
             upgrade_deadline=datetime.datetime(2017, 8, 3, 17, 44, 30, tzinfo=pytz.UTC),
             enrollment__course__org=unfiltered_org,
+            enrollment__course__self_paced=True,
             enrollment__user=user2,
         )
 
         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):
+
+        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,
@@ -226,14 +277,18 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
             ScheduleFactory.create(
                 upgrade_deadline=datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC),
                 enrollment__user=user,
+                enrollment__course__self_paced=True,
                 enrollment__course__id=CourseLocator('edX', 'toy', 'Course{}'.format(course_num))
             )
             for course_num in (1, 2, 3)
         ]
 
+        num_courses = len(set(s.enrollment.course.id for s in schedules))
+
         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):
+
+        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,
@@ -242,9 +297,8 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
         self.assertEqual(mock_schedule_send.apply_async.call_count, 1)
         self.assertFalse(mock_ace.send.called)
 
-    @ddt.data(*itertools.product((1, 10, 100), (2, 10)))
-    @ddt.unpack
-    def test_templates(self, message_count, day):
+    @ddt.data(1, 10, 100)
+    def test_templates(self, message_count):
         now = datetime.datetime.now(pytz.UTC)
         future_datetime = now + datetime.timedelta(days=21)
 
@@ -253,22 +307,22 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
             ScheduleFactory.create(
                 upgrade_deadline=future_datetime,
                 enrollment__user=user,
+                enrollment__course__self_paced=True,
+                enrollment__course__end=future_datetime + datetime.timedelta(days=30),
                 enrollment__course__id=CourseLocator('edX', 'toy', 'Course{}'.format(course_num))
             )
             for course_num in range(message_count)
         ]
 
         for schedule in schedules:
-            schedule.enrollment.course.self_paced = True
-            schedule.enrollment.course.end = future_datetime + datetime.timedelta(days=30)
-            schedule.enrollment.course.save()
-
             CourseModeFactory(
                 course_id=schedule.enrollment.course.id,
                 mode_slug=CourseMode.VERIFIED,
                 expiration_datetime=future_datetime
             )
 
+        num_courses = len(set(s.enrollment.course.id for s in schedules))
+
         test_datetime = future_datetime
         test_datetime_str = serialize(test_datetime)
 
@@ -285,12 +339,11 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
             with patch.object(tasks.ScheduleUpgradeReminder, 'async_send_task') as mock_schedule_send:
                 mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args)
 
-                # we execute one query per course to see if it's opted out of dynamic upgrade deadlines, however,
-                # since we create a new course for each schedule in this test, we expect there to be one per message
-                num_expected_queries = NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_WITH_DEADLINE
+                # 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
                 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=day,
+                        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],
                     )
@@ -316,4 +369,68 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase):
         return templates_override
 
     def _calculate_bin_for_user(self, user):
-        return user.id % resolvers.RECURRING_NUDGE_NUM_BINS
+        return user.id % resolvers.UPGRADE_REMINDER_NUM_BINS
+
+    @patch.object(tasks, '_upgrade_reminder_schedule_send')
+    def test_dont_send_to_verified_learner(self, mock_schedule_send):
+        upgrade_deadline = datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=2)
+        ScheduleFactory.create(
+            upgrade_deadline=upgrade_deadline,
+            enrollment__user=UserFactory.create(id=resolvers.UPGRADE_REMINDER_NUM_BINS),
+            enrollment__course=self.course_overview,
+            enrollment__mode=CourseMode.VERIFIED,
+        )
+        test_datetime_str = serialize(datetime.datetime.now(pytz.UTC))
+
+        tasks.ScheduleUpgradeReminder.delay(
+            self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=0,
+            org_list=[self.course.org],
+        )
+
+        self.assertFalse(mock_schedule_send.called)
+        self.assertFalse(mock_schedule_send.apply_async.called)
+
+    def test_filter_out_verified_schedules(self):
+        now = datetime.datetime.now(pytz.UTC)
+        future_datetime = now + datetime.timedelta(days=21)
+
+        user = UserFactory.create()
+        schedules = [
+            ScheduleFactory.create(
+                upgrade_deadline=future_datetime,
+                enrollment__user=user,
+                enrollment__course__self_paced=True,
+                enrollment__course__end=future_datetime + datetime.timedelta(days=30),
+                enrollment__course__id=CourseLocator('edX', 'toy', 'Course{}'.format(i)),
+                enrollment__mode=CourseMode.VERIFIED if i in (0, 3) else CourseMode.AUDIT,
+            )
+            for i in range(5)
+        ]
+
+        for schedule in schedules:
+            CourseModeFactory(
+                course_id=schedule.enrollment.course.id,
+                mode_slug=CourseMode.VERIFIED,
+                expiration_datetime=future_datetime
+            )
+
+        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],
+            )
+
+            messages = [Message.from_string(m) for m in sent_messages]
+            self.assertEqual(len(messages), 1)
+            message = messages[0]
+            self.assertItemsEqual(
+                message.context['course_ids'],
+                [str(schedules[i].enrollment.course.id) for i in (1, 2, 4)]
+            )

openedx/core/djangoapps/schedules/resolvers.py

@@ -165,11 +165,10 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver):
             template_context['course_ids'] = course_id_strs
 
             first_schedule = user_schedules[0]
-            template_context.update(self.get_template_context(user, user_schedules))
-
-            # Information for including upsell messaging in template.
-            _add_upsell_button_information_to_template_context(
-                user, first_schedule, template_context)
+            try:
+                template_context.update(self.get_template_context(user, user_schedules))
+            except InvalidContextError:
+                continue
 
             yield (user, first_schedule.enrollment.course.language, template_context)
 
@@ -188,10 +187,19 @@ class BinnedSchedulesBaseResolver(PrefixedDebugLoggerMixin, RecipientResolver):
             dict: This dict must be JSON serializable (no datetime objects!). When rendering the message templates it
                   it will be used as the template context. Note that it will also include several default values that
                   injected into all template contexts. See `get_base_template_context` for more information.
+
+        Raises:
+            InvalidContextError: If this user and set of schedules are not valid for this type of message. Raising this
+            exception will prevent this user from receiving the message, but allow other messages to be sent to other
+            users.
         """
         return {}
 
 
+class InvalidContextError(Exception):
+    pass
+
+
 class ScheduleStartResolver(BinnedSchedulesBaseResolver):
     """
     Send a message to all users whose schedule started at ``self.current_date`` + ``day_offset``.
@@ -202,13 +210,18 @@ class ScheduleStartResolver(BinnedSchedulesBaseResolver):
 
     def get_template_context(self, user, user_schedules):
         first_schedule = user_schedules[0]
-        return {
+        context = {
             'course_name': first_schedule.enrollment.course.display_name,
             'course_url': absolute_url(
                 self.site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])
             ),
         }
 
+        # Information for including upsell messaging in template.
+        context.update(_get_upsell_information_for_schedule(user, first_schedule))
+
+        return context
+
 
 def _get_datetime_beginning_of_day(dt):
     """
@@ -226,20 +239,41 @@ class UpgradeReminderResolver(BinnedSchedulesBaseResolver):
     num_bins = UPGRADE_REMINDER_NUM_BINS
 
     def get_template_context(self, user, user_schedules):
-        first_schedule = user_schedules[0]
-        return {
-            'course_links': [
-                {
-                    'url': absolute_url(self.site, reverse('course_root', args=[str(s.enrollment.course_id)])),
-                    'name': s.enrollment.course.display_name
-                } for s in user_schedules
-            ],
+        course_id_strs = []
+        course_links = []
+        first_valid_upsell_context = None
+        first_schedule = None
+        for schedule in user_schedules:
+            upsell_context = _get_upsell_information_for_schedule(user, schedule)
+            if not upsell_context['show_upsell']:
+                continue
+
+            if first_valid_upsell_context is None:
+                first_schedule = schedule
+                first_valid_upsell_context = upsell_context
+            course_id_str = str(schedule.enrollment.course_id)
+            course_id_strs.append(course_id_str)
+            course_links.append({
+                'url': absolute_url(self.site, reverse('course_root', args=[course_id_str])),
+                'name': schedule.enrollment.course.display_name
+            })
+
+        if first_schedule is None:
+            self.log_debug('No courses eligible for upgrade for user.')
+            raise InvalidContextError()
+
+        context = {
+            'course_links': course_links,
             'first_course_name': first_schedule.enrollment.course.display_name,
             'cert_image': absolute_url(self.site, static('course_experience/images/verified-cert.png')),
+            'course_ids': course_id_strs,
         }
+        context.update(first_valid_upsell_context)
+        return context
 
 
-def _add_upsell_button_information_to_template_context(user, schedule, template_context):
+def _get_upsell_information_for_schedule(user, schedule):
+    template_context = {}
     enrollment = schedule.enrollment
     course = enrollment.course
 
@@ -258,6 +292,7 @@ def _add_upsell_button_information_to_template_context(user, schedule, template_
         )
 
     template_context['show_upsell'] = has_verified_upgrade_link
+    return template_context
 
 
 def _get_verified_upgrade_link(user, schedule):
@@ -293,10 +328,9 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver):
             course_id_str = str(enrollment.course_id)
 
             template_context.update({
-                'student_name': user.profile.name,
                 'course_name': schedule.enrollment.course.display_name,
                 'course_url': absolute_url(
-                    self.site, reverse('course_root', args=[str(schedule.enrollment.course_id)])
+                    self.site, reverse('course_root', args=[course_id_str])
                 ),
                 'week_num': week_num,
                 'week_summary': week_summary,
@@ -304,6 +338,7 @@ class CourseUpdateResolver(BinnedSchedulesBaseResolver):
                 # This is used by the bulk email optout policy
                 'course_ids': [course_id_str],
             })
+            template_context.update(_get_upsell_information_for_schedule(user, schedule))
 
             yield (user, schedule.enrollment.course.language, template_context)