From 5bfb322f1ed6a1b49517cc2f673c73ccd4f7d932 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 11 Oct 2017 15:12:27 -0400 Subject: [PATCH 1/4] Don't send RET emails after course end --- .../course_overviews/tests/factories.py | 4 +++- .../tests/test_send_recurring_nudge.py | 22 +++++++++++++++++++ openedx/core/djangoapps/schedules/tasks.py | 8 ++++--- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content/course_overviews/tests/factories.py b/openedx/core/djangoapps/content/course_overviews/tests/factories.py index c9878e1481..f412fda062 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/factories.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/factories.py @@ -1,6 +1,7 @@ import json import factory +from django.utils.timezone import get_current_timezone from factory.django import DjangoModelFactory from ..models import CourseOverview @@ -14,7 +15,8 @@ class CourseOverviewFactory(DjangoModelFactory): version = CourseOverview.VERSION pre_requisite_courses = [] - start = factory.Faker('past_datetime') + 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_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 5674a9e3ea..049f519f5f 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 @@ -18,6 +18,7 @@ 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.tests.factories import CourseOverviewFactory 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 @@ -140,6 +141,27 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): # is null. self.assertEqual(mock_schedule_send.apply_async.call_count, 0) + @patch.object(tasks, '_recurring_nudge_schedule_send') + def test_send_after_course_end(self, mock_schedule_send): + user1 = UserFactory.create(id=tasks.RECURRING_NUDGE_NUM_BINS) + + schedule = ScheduleFactory.create( + start=datetime.datetime(2017, 8, 3, 20, 34, 30, tzinfo=pytz.UTC), + enrollment__user=user1, + ) + schedule.enrollment.course = CourseOverviewFactory() + schedule.enrollment.course.end = datetime.datetime.now() - datetime.timedelta(days=1) + + test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) + test_time_str = serialize(test_time) + + tasks.recurring_nudge_schedule_bin.apply_async( + self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=0, + org_list=[schedule.enrollment.course.org], + ) + + self.assertFalse(mock_schedule_send.apply_async.called) + @patch.object(tasks, 'ace') def test_delivery_disabled(self, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, deliver_recurring_nudge=False) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 054c7a175d..018d2b2d8d 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -9,7 +9,7 @@ from django.contrib.sites.models import Site from django.contrib.staticfiles.templatetags.staticfiles import static from django.core.exceptions import ValidationError from django.core.urlresolvers import reverse -from django.db.models import F, Min +from django.db.models import F, Min, Q from django.db.utils import DatabaseError from django.utils.formats import dateformat, get_format import pytz @@ -112,7 +112,7 @@ def recurring_nudge_schedule_bin( def _recurring_nudge_schedules_for_bin(site, target_day, bin_num, org_list, exclude_orgs=False): - beginning_of_day = target_day.replace(hour=0, minute=0, second=0) + beginning_of_day = target_day.replace(hour=0, minute=0, second=0, microsecond=0) schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', target_date=beginning_of_day, @@ -186,7 +186,7 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): def _upgrade_reminder_schedules_for_bin(site, target_day, bin_num, org_list, exclude_orgs=False): - beginning_of_day = target_day.replace(hour=0, minute=0, second=0) + beginning_of_day = target_day.replace(hour=0, minute=0, second=0, microsecond=0) schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='upgrade_deadline', @@ -332,6 +332,7 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_d exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list or strictly include (False) them (default: False) """ + today = datetime.datetime.now().replace(hour=0, minute=0, second=0, microsecond=0) 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), @@ -355,6 +356,7 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_d ).prefetch_related( 'enrollment__course__modes' ).filter( + Q(enrollment__course__end__isnull=True) | Q(enrollment__course__end__gte=today), enrollment__user__in=users, enrollment__is_active=True, **schedule_date_equals_target_date_filter From 3c250e5649f2bde197dddfb6234a18b0d8963b87 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Thu, 12 Oct 2017 15:07:04 -0400 Subject: [PATCH 2/4] Address Gabe's comments --- .../tests/test_send_recurring_nudge.py | 2 +- openedx/core/djangoapps/schedules/tasks.py | 58 +++++++++++++------ 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 049f519f5f..fddf199aa4 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 @@ -150,7 +150,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): enrollment__user=user1, ) schedule.enrollment.course = CourseOverviewFactory() - schedule.enrollment.course.end = datetime.datetime.now() - datetime.timedelta(days=1) + schedule.enrollment.course.end = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=1) test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_time_str = serialize(test_time) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 018d2b2d8d..c746fe1247 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -90,12 +90,15 @@ def _recurring_nudge_schedule_send(site_id, msg_str): def recurring_nudge_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_day = deserialize(target_day_str) + target_date = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_date instead of reproducing it here + current_date = target_date - datetime.timedelta(days=day_offset) msg_type = RecurringNudge(abs(day_offset)) for (user, language, context) in _recurring_nudge_schedules_for_bin( Site.objects.get(id=site_id), - target_day, + current_date, + target_date, bin_num, org_list, exclude_orgs @@ -111,11 +114,11 @@ def recurring_nudge_schedule_bin( _recurring_nudge_schedule_send.apply_async((site_id, str(msg)), retry=False) -def _recurring_nudge_schedules_for_bin(site, target_day, bin_num, org_list, exclude_orgs=False): - beginning_of_day = target_day.replace(hour=0, minute=0, second=0, microsecond=0) +def _recurring_nudge_schedules_for_bin(site, current_date, target_date, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', - target_date=beginning_of_day, + current_date=current_date, + target_date=target_date, bin_num=bin_num, num_bins=RECURRING_NUDGE_NUM_BINS, org_list=org_list, @@ -154,12 +157,15 @@ class UpgradeReminder(ScheduleMessageType): def upgrade_reminder_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_day = deserialize(target_day_str) + target_date = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_date instead of reproducing it here + current_date = target_date - datetime.timedelta(days=day_offset) msg_type = UpgradeReminder() for (user, language, context) in _upgrade_reminder_schedules_for_bin( Site.objects.get(id=site_id), - target_day, + current_date, + target_date, bin_num, org_list, exclude_orgs @@ -185,12 +191,11 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -def _upgrade_reminder_schedules_for_bin(site, target_day, bin_num, org_list, exclude_orgs=False): - beginning_of_day = target_day.replace(hour=0, minute=0, second=0, microsecond=0) - +def _upgrade_reminder_schedules_for_bin(site, current_date, target_date, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='upgrade_deadline', - target_date=beginning_of_day, + current_date=current_date, + target_date=target_date, bin_num=bin_num, num_bins=RECURRING_NUDGE_NUM_BINS, org_list=org_list, @@ -227,6 +232,7 @@ def _upgrade_reminder_schedules_for_bin(site, target_day, bin_num, org_list, exc yield (user, first_schedule.enrollment.course.language, template_context) +<<<<<<< HEAD class CourseUpdate(ScheduleMessageType): pass @@ -319,23 +325,30 @@ def get_course_week_summary(course_id, week_num): 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, order_by='enrollment__user__id'): +||||||| parent of c6f507c4b7... Address Gabe's comments +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): +======= +def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_date, target_date, bin_num, + num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False): +>>>>>>> c6f507c4b7... Address Gabe's comments """ 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 + current_date -- datetime that will be used as "right now" in the query + target_date -- datetime that the User's Schedule's schedule_date_field value should fall under bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list or strictly include (False) them (default: False) """ - today = datetime.datetime.now().replace(hour=0, minute=0, second=0, microsecond=0) + target_day = _get_datetime_beginning_of_day(target_date) 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), + 'courseenrollment__schedule__{}__gte'.format(schedule_date_field): target_day, + 'courseenrollment__schedule__{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), } users = User.objects.filter( courseenrollment__is_active=True, @@ -347,8 +360,8 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_d ) schedule_date_equals_target_date_filter = { - '{}__gte'.format(schedule_date_field): target_date, - '{}__lt'.format(schedule_date_field): target_date + datetime.timedelta(days=1), + '{}__gte'.format(schedule_date_field): target_day, + '{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), } schedules = Schedule.objects.select_related( 'enrollment__user__profile', @@ -356,7 +369,7 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_d ).prefetch_related( 'enrollment__course__modes' ).filter( - Q(enrollment__course__end__isnull=True) | Q(enrollment__course__end__gte=today), + Q(enrollment__course__end__isnull=True) | Q(enrollment__course__end__gte=current_date), enrollment__user__in=users, enrollment__is_active=True, **schedule_date_equals_target_date_filter @@ -401,3 +414,10 @@ def _get_link_to_purchase_verified_certificate(a_user, a_schedule): return None return verified_upgrade_deadline_link(a_user, enrollment.course) + + +def _get_datetime_beginning_of_day(dt): + """ + Truncates hours, minutes, seconds, and microseconds to zero on given datetime. + """ + return dt.replace(hour=0, minute=0, second=0, microsecond=0) From bf95f2371caa0d47da7faf5566fab8df4af68bcc Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 13 Oct 2017 13:24:24 -0400 Subject: [PATCH 3/4] Standardize on var names: *_datetime and *_day --- .../tests/test_send_recurring_nudge.py | 54 ++++++------- .../tests/test_send_upgrade_reminder.py | 54 ++++++------- openedx/core/djangoapps/schedules/tasks.py | 76 +++++++++---------- 3 files changed, 91 insertions(+), 93 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index fddf199aa4..85c9c8477f 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 @@ -62,9 +62,9 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): @patch.object(nudge.Command, 'resolver_class') def test_handle(self, mock_resolver): - test_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) + test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) nudge.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) - mock_resolver.assert_called_with(self.site_config.site, test_time) + mock_resolver.assert_called_with(self.site_config.site, test_day) for day in (-3, -10): mock_resolver().send.assert_any_call(day, None) @@ -72,16 +72,16 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): @patch.object(tasks, 'ace') @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) - test_time = current_time + datetime.timedelta(days=-3) + current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) + nudge.ScheduleStartResolver(self.site_config.site, current_day).send(-3) + test_day = current_day + datetime.timedelta(days=-3) self.assertFalse(mock_schedule_bin.called) mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_time), -3, 0, [], True, None), + (self.site_config.site.id, serialize(test_day), -3, 0, [], True, None), retry=False, ) mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_time), -3, tasks.RECURRING_NUDGE_NUM_BINS - 1, [], True, None), + (self.site_config.site.id, serialize(test_day), -3, tasks.RECURRING_NUDGE_NUM_BINS - 1, [], True, None), retry=False, ) self.assertFalse(mock_ace.send.called) @@ -99,8 +99,8 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): bins_in_use = frozenset((s.enrollment.user.id % tasks.RECURRING_NUDGE_NUM_BINS) for s in schedules) - test_time = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) for b in range(tasks.RECURRING_NUDGE_NUM_BINS): expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES if b in bins_in_use: @@ -109,7 +109,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=b, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count) @@ -124,12 +124,12 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): schedule.enrollment.course_id = CourseKey.from_string('edX/toy/Not_2012_Fall') schedule.enrollment.save() - test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) for b in range(tasks.RECURRING_NUDGE_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=b, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=b, org_list=[schedule.enrollment.course.org], ) @@ -152,11 +152,11 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): schedule.enrollment.course = CourseOverviewFactory() schedule.enrollment.course.end = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=1) - test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) tasks.recurring_nudge_schedule_bin.apply_async( - self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=0, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0, org_list=[schedule.enrollment.course.org], ) @@ -175,8 +175,8 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): def test_enqueue_disabled(self, mock_schedule_bin, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_recurring_nudge=False) - current_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - nudge.ScheduleStartResolver(self.site_config.site, current_time).send(3) + current_datetime = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) + nudge.ScheduleStartResolver(self.site_config.site, current_datetime).send(3) self.assertFalse(mock_schedule_bin.called) self.assertFalse(mock_schedule_bin.apply_async.called) self.assertFalse(mock_ace.send.called) @@ -218,11 +218,11 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): enrollment__user=user2, ) - test_time = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( - limited_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=0, + limited_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, ) @@ -242,11 +242,11 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): for course_num in (1, 2, 3) ] - test_time = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=-3, bin_num=user.id % tasks.RECURRING_NUDGE_NUM_BINS, org_list=[schedules[0].enrollment.course.org], ) @@ -267,8 +267,8 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): for course_num in range(message_count) ] - test_time = datetime.datetime(2017, 8, 3, 19, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 19, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) patch_policies(self, [StubPolicy([ChannelType.PUSH])]) mock_channel = Mock( @@ -285,7 +285,7 @@ class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=day, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], ) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 9003aa45fd..3a6f6e61cc 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 @@ -62,27 +62,27 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): @patch.object(reminder.Command, 'resolver_class') def test_handle(self, mock_resolver): - test_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) + test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) reminder.Command().handle(date='2017-08-01', site_domain_name=self.site_config.site.domain) - mock_resolver.assert_called_with(self.site_config.site, test_time) + mock_resolver.assert_called_with(self.site_config.site, test_day) mock_resolver().send.assert_any_call(2, None) @patch.object(tasks, 'ace') @patch.object(resolvers.UpgradeReminderResolver, 'async_send_task') def test_resolver_send(self, mock_schedule_bin, mock_ace): - current_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - test_time = current_time + datetime.timedelta(days=2) + current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) + test_day = current_day + datetime.timedelta(days=2) ScheduleFactory.create(upgrade_deadline=datetime.datetime(2017, 8, 3, 15, 34, 30, tzinfo=pytz.UTC)) - reminder.UpgradeReminderResolver(self.site_config.site, current_time).send(2) + reminder.UpgradeReminderResolver(self.site_config.site, current_day).send(2) self.assertFalse(mock_schedule_bin.called) mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_time), 2, 0, [], True, None), + (self.site_config.site.id, serialize(test_day), 2, 0, [], True, None), retry=False, ) mock_schedule_bin.apply_async.assert_any_call( - (self.site_config.site.id, serialize(test_time), 2, tasks.UPGRADE_REMINDER_NUM_BINS - 1, [], True, None), + (self.site_config.site.id, serialize(test_day), 2, tasks.UPGRADE_REMINDER_NUM_BINS - 1, [], True, None), retry=False, ) self.assertFalse(mock_ace.send.called) @@ -100,8 +100,8 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): bins_in_use = frozenset((s.enrollment.user.id % tasks.UPGRADE_REMINDER_NUM_BINS) for s in schedules) - test_time = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES if b in bins_in_use: @@ -110,7 +110,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=b, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) @@ -126,12 +126,12 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): schedule.enrollment.course_id = CourseKey.from_string('edX/toy/Not_2012_Fall') schedule.enrollment.save() - test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=b, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=b, org_list=[schedule.enrollment.course.org], ) @@ -156,8 +156,8 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): def test_enqueue_disabled(self, mock_schedule_bin, mock_ace): ScheduleConfigFactory.create(site=self.site_config.site, enqueue_upgrade_reminder=False) - current_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) - reminder.UpgradeReminderResolver(self.site_config.site, current_time).send(3) + current_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) + reminder.UpgradeReminderResolver(self.site_config.site, current_day).send(3) self.assertFalse(mock_schedule_bin.called) self.assertFalse(mock_schedule_bin.apply_async.called) self.assertFalse(mock_ace.send.called) @@ -199,11 +199,11 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): enrollment__user=user2, ) - test_time = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( - limited_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=0, + limited_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, ) @@ -223,11 +223,11 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): for course_num in (1, 2, 3) ] - test_time = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) - test_time_str = serialize(test_time) + test_datetime = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) + test_datetime_str = serialize(test_datetime) with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=2, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=2, bin_num=user.id % tasks.UPGRADE_REMINDER_NUM_BINS, org_list=[schedules[0].enrollment.course.org], ) @@ -239,12 +239,12 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): def test_templates(self, message_count, day): DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) now = datetime.datetime.now(pytz.UTC) - future_date = now + datetime.timedelta(days=21) + future_datetime = now + datetime.timedelta(days=21) user = UserFactory.create() schedules = [ ScheduleFactory.create( - upgrade_deadline=future_date, + upgrade_deadline=future_datetime, enrollment__user=user, enrollment__course__id=CourseLocator('edX', 'toy', 'Course{}'.format(course_num)) ) @@ -258,11 +258,11 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): CourseModeFactory( course_id=schedule.enrollment.course.id, mode_slug=CourseMode.VERIFIED, - expiration_datetime=future_date + expiration_datetime=future_datetime ) - test_time = future_date - test_time_str = serialize(test_time) + test_datetime = future_datetime + test_datetime_str = serialize(test_datetime) patch_policies(self, [StubPolicy([ChannelType.PUSH])]) mock_channel = Mock( @@ -284,7 +284,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): num_expected_queries = NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_WITH_DEADLINE + message_count with self.assertNumQueries(num_expected_queries, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( - self.site_config.site.id, target_day_str=test_time_str, day_offset=day, + self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, bin_num=user.id % tasks.UPGRADE_REMINDER_NUM_BINS, org_list=[schedules[0].enrollment.course.org], ) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index c746fe1247..f42b60e3d4 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -90,15 +90,15 @@ def _recurring_nudge_schedule_send(site_id, msg_str): def recurring_nudge_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_date = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_date instead of reproducing it here - current_date = target_date - datetime.timedelta(days=day_offset) + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = RecurringNudge(abs(day_offset)) for (user, language, context) in _recurring_nudge_schedules_for_bin( Site.objects.get(id=site_id), - current_date, - target_date, + current_datetime, + target_datetime, bin_num, org_list, exclude_orgs @@ -114,11 +114,11 @@ def recurring_nudge_schedule_bin( _recurring_nudge_schedule_send.apply_async((site_id, str(msg)), retry=False) -def _recurring_nudge_schedules_for_bin(site, current_date, target_date, bin_num, org_list, exclude_orgs=False): +def _recurring_nudge_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', - current_date=current_date, - target_date=target_date, + current_datetime=current_datetime, + target_datetime=target_datetime, bin_num=bin_num, num_bins=RECURRING_NUDGE_NUM_BINS, org_list=org_list, @@ -157,15 +157,15 @@ class UpgradeReminder(ScheduleMessageType): def upgrade_reminder_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_date = deserialize(target_day_str) - # TODO: in the next refactor of this task, pass in current_date instead of reproducing it here - current_date = target_date - datetime.timedelta(days=day_offset) + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = UpgradeReminder() for (user, language, context) in _upgrade_reminder_schedules_for_bin( Site.objects.get(id=site_id), - current_date, - target_date, + current_datetime, + target_datetime, bin_num, org_list, exclude_orgs @@ -191,11 +191,11 @@ def _upgrade_reminder_schedule_send(site_id, msg_str): ace.send(msg) -def _upgrade_reminder_schedules_for_bin(site, current_date, target_date, bin_num, org_list, exclude_orgs=False): +def _upgrade_reminder_schedules_for_bin(site, current_datetime, target_datetime, bin_num, org_list, exclude_orgs=False): schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='upgrade_deadline', - current_date=current_date, - target_date=target_date, + current_datetime=current_datetime, + target_datetime=target_datetime, bin_num=bin_num, num_bins=RECURRING_NUDGE_NUM_BINS, org_list=org_list, @@ -232,7 +232,6 @@ def _upgrade_reminder_schedules_for_bin(site, current_date, target_date, bin_num yield (user, first_schedule.enrollment.course.language, template_context) -<<<<<<< HEAD class CourseUpdate(ScheduleMessageType): pass @@ -241,12 +240,15 @@ class CourseUpdate(ScheduleMessageType): def course_update_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, ): - target_day = deserialize(target_day_str) + target_datetime = deserialize(target_day_str) + # TODO: in the next refactor of this task, pass in current_datetime instead of reproducing it here + current_datetime = target_datetime - datetime.timedelta(days=day_offset) msg_type = CourseUpdate() for (user, language, context) in _course_update_schedules_for_bin( Site.objects.get(id=site_id), - target_day, + current_datetime, + target_datetime, day_offset, bin_num, org_list, @@ -273,12 +275,13 @@ def _course_update_schedule_send(site_id, msg_str): ace.send(msg) -def _course_update_schedules_for_bin(site, target_day, day_offset, bin_num, org_list, exclude_orgs=False): +def _course_update_schedules_for_bin(site, current_datetime, target_datetime, day_offset, bin_num, org_list, + exclude_orgs=False): week_num = abs(day_offset) / 7 - beginning_of_day = target_day.replace(hour=0, minute=0, second=0) schedules = get_schedules_with_target_date_by_bin_and_orgs( schedule_date_field='start', - target_date=beginning_of_day, + current_datetime=current_datetime, + target_datetime=target_datetime, bin_num=bin_num, num_bins=COURSE_UPDATE_NUM_BINS, org_list=org_list, @@ -323,43 +326,38 @@ def get_course_week_summary(course_id, week_num): raise CourseUpdateDoesNotExist() -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, order_by='enrollment__user__id'): -||||||| parent of c6f507c4b7... Address Gabe's comments -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): -======= -def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_date, target_date, bin_num, - num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False): ->>>>>>> c6f507c4b7... Address Gabe's comments +def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_datetime, target_datetime, bin_num, + num_bins=DEFAULT_NUM_BINS, org_list=None, exclude_orgs=False, + order_by='enrollment__user__id'): """ Returns Schedules with the target_date, related to Users whose id matches the bin_num, and filtered by org_list. Arguments: schedule_date_field -- string field name to query on the User's Schedule model - current_date -- datetime that will be used as "right now" in the query - target_date -- datetime that the User's Schedule's schedule_date_field value should fall under + current_datetime -- datetime that will be used as "right now" in the query + target_datetime -- datetime that the User's Schedule's schedule_date_field value should fall under bin_num -- int for selecting the bin of Users whose id % num_bins == bin_num num_bin -- int specifying the number of bins to separate the Users into (default: DEFAULT_NUM_BINS) org_list -- list of course_org names (strings) that the returned Schedules must or must not be in (default: None) exclude_orgs -- boolean indicating whether the returned Schedules should exclude (True) the course_orgs in org_list or strictly include (False) them (default: False) + order_by -- string for field to sort the resulting Schedules by """ - target_day = _get_datetime_beginning_of_day(target_date) - schedule_date_equals_target_date_filter = { + target_day = _get_datetime_beginning_of_day(target_datetime) + schedule_day_equals_target_day_filter = { 'courseenrollment__schedule__{}__gte'.format(schedule_date_field): target_day, 'courseenrollment__schedule__{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), } users = User.objects.filter( courseenrollment__is_active=True, - **schedule_date_equals_target_date_filter + **schedule_day_equals_target_day_filter ).annotate( id_mod=F('id') % num_bins ).filter( id_mod=bin_num ) - schedule_date_equals_target_date_filter = { + schedule_day_equals_target_day_filter = { '{}__gte'.format(schedule_date_field): target_day, '{}__lt'.format(schedule_date_field): target_day + datetime.timedelta(days=1), } @@ -369,10 +367,10 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, current_ ).prefetch_related( 'enrollment__course__modes' ).filter( - Q(enrollment__course__end__isnull=True) | Q(enrollment__course__end__gte=current_date), + Q(enrollment__course__end__isnull=True) | Q(enrollment__course__end__gte=current_datetime), enrollment__user__in=users, enrollment__is_active=True, - **schedule_date_equals_target_date_filter + **schedule_day_equals_target_day_filter ).order_by(order_by) if org_list is not None: From 1c8fcf218a212f39e7956788593cba2d03a63a84 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 16 Oct 2017 16:15:10 -0400 Subject: [PATCH 4/4] Fix tests --- .../tests/test_send_upgrade_reminder.py | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) 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 3a6f6e61cc..13a214acab 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 @@ -60,6 +60,8 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): self.site_config = SiteConfigurationFactory.create(site=site) ScheduleConfigFactory.create(site=self.site_config.site) + DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) + @patch.object(reminder.Command, 'resolver_class') def test_handle(self, mock_resolver): test_day = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) @@ -237,7 +239,6 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): @ddt.data(*itertools.product((1, 10, 100), (2, 10))) @ddt.unpack def test_templates(self, message_count, day): - DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) now = datetime.datetime.now(pytz.UTC) future_datetime = now + datetime.timedelta(days=21) @@ -253,6 +254,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): 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( @@ -273,9 +275,7 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): sent_messages = [] - templates_override = deepcopy(settings.TEMPLATES) - templates_override[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]" - with self.settings(TEMPLATES=templates_override): + with self.settings(TEMPLATES=self._get_template_overrides()): with patch.object(tasks, '_upgrade_reminder_schedule_send') as mock_schedule_send: mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) @@ -285,16 +285,27 @@ class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): with self.assertNumQueries(num_expected_queries, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_datetime_str, day_offset=day, - bin_num=user.id % tasks.UPGRADE_REMINDER_NUM_BINS, + bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], ) self.assertEqual(len(sent_messages), message_count) - for args in sent_messages: - tasks._upgrade_reminder_schedule_send(*args) + # Load the site (which we query per message sent) + # Check the schedule config + with self.assertNumQueries(1 + message_count): + for args in sent_messages: + tasks._upgrade_reminder_schedule_send(*args) self.assertEqual(mock_channel.deliver.call_count, message_count) for (_name, (_msg, email), _kwargs) in mock_channel.deliver.mock_calls: for template in attr.astuple(email): self.assertNotIn("TEMPLATE WARNING", template) + + def _get_template_overrides(self): + templates_override = deepcopy(settings.TEMPLATES) + templates_override[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]" + return templates_override + + def _calculate_bin_for_user(self, user): + return user.id % tasks.RECURRING_NUDGE_NUM_BINS