Add tests for upgrade_reminder

Fix test_templates

By passing the date pre-formatted to ace.

Address PR comments

Fix linting

Add TODO comment about using LoggerAdapter

More lint fixes

Fix upgrade_reminder task from silently failing

openedx/core/djangoapps/schedules/management/commands/__init__.py

@@ -15,6 +15,8 @@ from openedx.core.djangoapps.site_configuration.models import SiteConfiguration
 LOG = logging.getLogger(__name__)
 
 
+# TODO: consider using a LoggerAdapter instead of this mixin:
+# https://docs.python.org/2/library/logging.html#logging.LoggerAdapter
 class PrefixedDebugLoggerMixin(object):
     def __init__(self, *args, **kwargs):
         self.log_prefix = self.__class__.__name__

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

@@ -11,7 +11,7 @@ from openedx.core.djangoapps.schedules.management.commands import (
     SendEmailBaseCommand,
     BinnedSchedulesBaseResolver
 )
-from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory, ScheduleFactory
+from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory
 from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory
 from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms
 
@@ -96,7 +96,6 @@ class TestBinnedSchedulesBaseResolver(CacheIsolationTestCase):
         assert not exclude_orgs
         assert org_list == expected_org_list
 
-    # factory_boy doesn't make sense at all
     @ddt.unpack
     @ddt.data(
         (None, []),

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

@@ -61,7 +61,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase):
             retry=False,
         )
         mock_schedule_bin.apply_async.assert_any_call(
-            (self.site_config.site.id, serialize(test_time), -3, 23, [], True, None),
+            (self.site_config.site.id, serialize(test_time), -3, tasks.RECURRING_NUDGE_NUM_BINS - 1, [], True, None),
             retry=False,
         )
         self.assertFalse(mock_ace.send.called)

openedx/core/djangoapps/schedules/tasks.py

@@ -8,8 +8,9 @@ from django.contrib.auth.models import User
 from django.contrib.sites.models import Site
 from django.core.exceptions import ValidationError
 from django.core.urlresolvers import reverse
-from django.db.models import F, Min, Prefetch
+from django.db.models import F, Min
 from django.db.utils import DatabaseError
+from django.utils.formats import dateformat, get_format
 
 from edx_ace import ace
 from edx_ace.message import Message
@@ -17,7 +18,6 @@ from edx_ace.recipient import Recipient
 from edx_ace.utils.date import deserialize
 from opaque_keys.edx.keys import CourseKey
 
-from course_modes.models import CourseMode
 from edxmako.shortcuts import marketing_link
 from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType
 from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig
@@ -27,7 +27,6 @@ from openedx.core.djangoapps.schedules.template_context import (
     encode_urls_in_dict,
     get_base_template_context
 )
-from openedx.core.djangoapps.user_api.models import UserPreference
 
 
 LOG = logging.getLogger(__name__)
@@ -232,9 +231,7 @@ def _recurring_nudge_schedules_for_bin(target_day, bin_num, org_list, exclude_or
 
 
 class UpgradeReminder(ScheduleMessageType):
-    def __init__(self, day, *args, **kwargs):
-        super(UpgradeReminder, self).__init__(*args, **kwargs)
-        self.name = "upgradereminder".format(day)
+    pass
 
 
 @task(ignore_result=True, routing_key=ROUTING_KEY)
@@ -242,7 +239,7 @@ 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)
-    msg_type = UpgradeReminder(abs(day_offset))
+    msg_type = UpgradeReminder()
 
     for (user, language, context) in _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_orgs):
         msg = msg_type.personalize(
@@ -267,30 +264,35 @@ def _upgrade_reminder_schedule_send(site_id, msg_str):
 
 
 def _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_orgs=False):
-    schedules = Schedule.objects.select_related(
-        'enrollment__user__profile',
-        'enrollment__course',
-    ).prefetch_related(
-        Prefetch(
-            'enrollment__course__modes',
-            queryset=CourseMode.objects.filter(mode_slug=CourseMode.VERIFIED),
-            to_attr='verified_modes'
-        ),
-        Prefetch(
-            'enrollment__user__preferences',
-            queryset=UserPreference.objects.filter(key='time_zone'),
-            to_attr='tzprefs'
-        ),
+    beginning_of_day = target_day.replace(hour=0, minute=0, second=0)
+    users = User.objects.filter(
+        courseenrollment__schedule__upgrade_deadline__gte=beginning_of_day,
+        courseenrollment__schedule__upgrade_deadline__lt=beginning_of_day + datetime.timedelta(days=1),
+        courseenrollment__is_active=True,
     ).annotate(
-        id_mod=F('enrollment__user__id') % UPGRADE_REMINDER_NUM_BINS
+        first_schedule=Min('courseenrollment__schedule__upgrade_deadline')
+    ).annotate(
+        id_mod=F('id') % UPGRADE_REMINDER_NUM_BINS
     ).filter(
         id_mod=bin_num
-    ).filter(
-        upgrade_deadline__year=target_day.year,
-        upgrade_deadline__month=target_day.month,
-        upgrade_deadline__day=target_day.day,
     )
 
+    schedules = Schedule.objects.select_related(
+        'enrollment__user__profile',
+        'enrollment__course',
+    ).filter(
+        enrollment__user__in=users,
+        upgrade_deadline__gte=beginning_of_day,
+        upgrade_deadline__lt=beginning_of_day + datetime.timedelta(days=1),
+        enrollment__is_active=True,
+    ).order_by('enrollment__user__id')
+
+    if org_list is not None:
+        if exclude_orgs:
+            schedules = schedules.exclude(enrollment__course__org__in=org_list)
+        else:
+            schedules = schedules.filter(enrollment__course__org__in=org_list)
+
     if "read_replica" in settings.DATABASES:
         schedules = schedules.using("read_replica")
 
@@ -299,7 +301,6 @@ def _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_o
         user = enrollment.user
 
         course_id_str = str(enrollment.course_id)
-        course = enrollment.course
 
         # TODO: group by schedule and user like recurring nudge
         course_id_strs = [course_id_str]
@@ -309,7 +310,14 @@ def _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_o
         template_context.update({
             'student_name': user.profile.name,
             'user_personal_address': user.profile.name if user.profile.name else user.username,
-            'user_schedule_upgrade_deadline_time': schedule.upgrade_deadline,
+            'user_schedule_upgrade_deadline_time': dateformat.format(
+                schedule.upgrade_deadline,
+                get_format(
+                    'DATE_FORMAT',
+                    lang=first_schedule.enrollment.course.language,
+                    use_l10n=True
+                )
+            ),
 
             'course_name': first_schedule.enrollment.course.display_name,
             'course_url': absolute_url(reverse('course_root', args=[str(first_schedule.enrollment.course_id)])),
@@ -318,4 +326,4 @@ def _upgrade_reminder_schedules_for_bin(target_day, bin_num, org_list, exclude_o
             'course_ids': course_id_strs,
         })
 
-        yield (user, course.language, template_context)
+        yield (user, first_schedule.enrollment.course.language, template_context)

openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html

@@ -2,15 +2,15 @@
 {% load i18n %}
 
 {% block preview_text %}
-    {% if courses|length > 1 %}
+    {% if course_ids|length > 1 %}
         {% blocktrans trimmed %}
             We hope you are enjoying {{ course_name }}, and other courses on edX.org.
-            Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }} to get a shareable certificate!
+            Upgrade by {{ user_schedule_upgrade_deadline_time }} to get a shareable certificate!
         {% endblocktrans %}
     {% else %}
         {% blocktrans trimmed %}
             We hope you are enjoying {{ course_name }}.
-            Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }} to get a shareable certificate!
+            Upgrade by {{ user_schedule_upgrade_deadline_time }} to get a shareable certificate!
         {% endblocktrans %}
     {% endif %}
 {% endblock %}
@@ -22,16 +22,15 @@
             <h1>{% trans "Upgrade now" %}</h1>
 
             <p>
-                {% if courses|length > 1 %}
+                {% if course_ids|length > 1 %}
                     {% blocktrans trimmed %}
                         We hope you are enjoying <strong>{{ course_name }}</strong>, and other courses on edX.org.
-                        {{ user_schedule_upgrade_deadline_time|date }}
-                        Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }} to get a shareable certificate!
+                        Upgrade by {{ user_schedule_upgrade_deadline_time }} to get a shareable certificate!
                     {% endblocktrans %}
                 {% else %}
                     {% blocktrans trimmed %}
                         We hope you are enjoying <strong>{{ course_name }}</strong>.
-                        Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }} to get a shareable certificate!
+                        Upgrade by {{ user_schedule_upgrade_deadline_time }} to get a shareable certificate!
                     {% endblocktrans %}
                 {% endif %}
             </p>
@@ -39,7 +38,7 @@
             <p>
                 <!-- email client support for style sheets is pretty spotty, so we have to inline all of these styles -->
                 <a
-                {% if courses|length > 1 %}
+                {% if course_ids|length > 1 %}
                     href="{{ dashboard_url }}"
                 {% else %}
                     href="{{ course_url }}"

openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.txt

@@ -4,17 +4,17 @@
 Dear {{ user_personal_address }},
 {% endblocktrans %}
 
-{% if courses|length > 1 %}
+{% if course_ids|length > 1 %}
 {% blocktrans trimmed %}
     We hope you are enjoying {{ course_name }}, and other courses on edX.org.
-    Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }} to get a shareable certificate!
+    Upgrade by {{ user_schedule_upgrade_deadline_time }} to get a shareable certificate!
 {% endblocktrans %}
 
 {% trans "Upgrade now at" %} <{{ dashboard_url }}>
 {% else %}
 {% blocktrans trimmed %}
     We hope you are enjoying {{ course_name }}.
-    Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }} to get a shareable certificate!
+    Upgrade by {{ user_schedule_upgrade_deadline_time }} to get a shareable certificate!
 {% endblocktrans %}
 
 {% trans "Upgrade now at" %} <{{ course_url }}>

openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/from_name.txt

-{% if courses|length > 1 %}
+{% if course_ids|length > 1 %}
 {{ platform_name }}
 {% else %}
 {{ course_name }}

openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/subject.txt

 {% load i18n %}
 
-{% if courses|length > 1 %}
+{% if course_ids|length > 1 %}
 {% blocktrans %}Only two days left to upgrade on {{ platform_name }}!{% endblocktrans %}
 {% else %}
 {% blocktrans %}Only two days left to upgrade in {{course_name}} !{% endblocktrans %}

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

@@ -23,3 +23,5 @@ class ScheduleConfigFactory(factory.DjangoModelFactory):
     create_schedules = True
     enqueue_recurring_nudge = True
     deliver_recurring_nudge = True
+    enqueue_upgrade_reminder = True
+    deliver_upgrade_reminder = True