Only send one email even if there are multiple enrollments by the same user on the same day

lms/djangoapps/bulk_email/policies.py

@@ -9,12 +9,12 @@ from bulk_email.models import Optout
 class CourseEmailOptout(Policy):
 
     def check(self, message):
-        course_id = message.context.get('course_id')
-        if not course_id:
+        course_ids = message.context.get('course_ids')
+        if not course_ids:
             return PolicyResult(deny=frozenset())
 
-        course_key = CourseKey.from_string(course_id)
-        if Optout.objects.filter(user__username=message.recipient.username, course_id=course_key).exists():
+        course_keys = [CourseKey.from_string(course_id) for course_id in course_ids]
+        if Optout.objects.filter(user__username=message.recipient.username, course_id__in=course_keys).count() == len(course_keys):
             return PolicyResult(deny={ChannelType.EMAIL})
 
         return PolicyResult(deny=frozenset())

lms/djangoapps/bulk_email/tests/test_course_optout.py

@@ -166,7 +166,7 @@ class TestACEOptoutCourseEmails(ModuleStoreTestCase):
                 email_address=self.student.email,
             ),
             context={
-                'course_id': str(self.course.id)
+                'course_ids': [str(self.course.id)]
             },
         )
 

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

@@ -28,3 +28,7 @@ class CourseOverviewFactory(DjangoModelFactory):
     @factory.lazy_attribute
     def id(self):
         return CourseLocator(self.org, 'toy', '2012_Fall')
+
+    @factory.lazy_attribute
+    def display_name(self):
+        return "{} Course".format(self.id)

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

@@ -1,27 +1,32 @@
 import datetime
-from mock import patch, Mock
+import itertools
 from unittest import skipUnless
-import pytz
 
+import attr
 import ddt
+import pytz
 from django.conf import settings
-
+from django.test import override_settings
+from edx_ace.channel import ChannelType
+from edx_ace.test_utils import StubPolicy, patch_channels, patch_policies
 from edx_ace.utils.date import serialize
-
+from mock import Mock, patch
 from opaque_keys.edx.keys import CourseKey
+from opaque_keys.edx.locator import CourseLocator
+
 from openedx.core.djangoapps.schedules import tasks
 from openedx.core.djangoapps.schedules.management.commands import send_recurring_nudge as nudge
-from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory
+from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory, ScheduleFactory
+from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory
 from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms
-from openedx.core.djangoapps.schedules.tests.factories import ScheduleFactory, ScheduleConfigFactory
-from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory
+from student.tests.factories import UserFactory
 
 
 @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")
+@skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS,
+            "Can't test schedules if the app isn't installed")
 class TestSendRecurringNudge(CacheIsolationTestCase):
-
     # pylint: disable=protected-access
 
     def setUp(self):
@@ -71,7 +76,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase):
         test_time_str = serialize(datetime.datetime(2017, 8, 1, 18, tzinfo=pytz.UTC))
         with self.assertNumQueries(1):
             tasks.recurring_nudge_schedule_hour(
-                self.site_config.site, 3, test_time_str, [schedules[0].enrollment.course.org],
+                self.site_config.site.id, 3, test_time_str, [schedules[0].enrollment.course.org],
             )
         self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count)
         self.assertFalse(mock_ace.send.called)
@@ -88,7 +93,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase):
         test_time_str = serialize(datetime.datetime(2017, 8, 1, 20, tzinfo=pytz.UTC))
         with self.assertNumQueries(1):
             tasks.recurring_nudge_schedule_hour(
-                self.site_config.site, 3, test_time_str, [schedule.enrollment.course.org],
+                self.site_config.site.id, 3, test_time_str, [schedule.enrollment.course.org],
             )
 
         # There is no database constraint that enforces that enrollment.course_id points
@@ -136,27 +141,15 @@ class TestSendRecurringNudge(CacheIsolationTestCase):
         for config in (limited_config, unlimited_config):
             ScheduleConfigFactory.create(site=config.site)
 
-        filtered_sched = ScheduleFactory.create(
+        ScheduleFactory.create(
             start=datetime.datetime(2017, 8, 2, 17, 44, 30, tzinfo=pytz.UTC),
             enrollment__course__org=filtered_org,
         )
-        unfiltered_scheds = [
+        for _ in range(2):
             ScheduleFactory.create(
                 start=datetime.datetime(2017, 8, 2, 17, 44, 30, tzinfo=pytz.UTC),
                 enrollment__course__org=unfiltered_org,
             )
-            for _ in range(2)
-        ]
-
-        print(filtered_sched.enrollment)
-        print(filtered_sched.enrollment.course)
-        print(filtered_sched.enrollment.course.org)
-        print(unfiltered_scheds[0].enrollment)
-        print(unfiltered_scheds[0].enrollment.course)
-        print(unfiltered_scheds[0].enrollment.course.org)
-        print(unfiltered_scheds[1].enrollment)
-        print(unfiltered_scheds[1].enrollment.course)
-        print(unfiltered_scheds[1].enrollment.course.org)
 
         test_time_str = serialize(datetime.datetime(2017, 8, 2, 17, tzinfo=pytz.UTC))
         with self.assertNumQueries(1):
@@ -164,6 +157,77 @@ class TestSendRecurringNudge(CacheIsolationTestCase):
                 limited_config.site.id, 3, test_time_str, org_list=org_list, exclude_orgs=exclude_orgs,
             )
 
-        print(mock_schedule_send.mock_calls)
         self.assertEqual(mock_schedule_send.apply_async.call_count, expected_message_count)
         self.assertFalse(mock_ace.send.called)
+
+    @ddt.data(
+        (19, 1),
+        (20, 0),
+        (21, 0)
+    )
+    @ddt.unpack
+    @patch.object(tasks, 'ace')
+    @patch.object(tasks, '_recurring_nudge_schedule_send')
+    def test_multiple_enrollments(self, test_hour, messages_sent, mock_schedule_send, mock_ace):
+        user = UserFactory.create()
+        schedules = [
+            ScheduleFactory.create(
+                start=datetime.datetime(2017, 8, 1, hour, 44, 30, tzinfo=pytz.UTC),
+                enrollment__user=user,
+                enrollment__course__id=CourseLocator('edX', 'toy', 'Hour{}'.format(hour))
+            )
+            for hour in (19, 20, 21)
+        ]
+
+        test_time_str = serialize(datetime.datetime(2017, 8, 1, test_hour, tzinfo=pytz.UTC))
+        with self.assertNumQueries(1):
+            tasks.recurring_nudge_schedule_hour(
+                self.site_config.site.id, 3, test_time_str, [schedules[0].enrollment.course.org],
+            )
+        self.assertEqual(mock_schedule_send.apply_async.call_count, messages_sent)
+        self.assertFalse(mock_ace.send.called)
+
+    @ddt.data(*itertools.product((1, 10, 100), (3, 10)))
+    @ddt.unpack
+    @override_settings()
+    def test_templates(self, message_count, day):
+
+        settings.TEMPLATES[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]"
+        user = UserFactory.create()
+        schedules = [
+            ScheduleFactory.create(
+                start=datetime.datetime(2017, 8, 1, 19, 44, 30, tzinfo=pytz.UTC),
+                enrollment__user=user,
+                enrollment__course__id=CourseLocator('edX', 'toy', 'Hour{}'.format(idx))
+            )
+            for idx in range(message_count)
+        ]
+
+        test_time_str = serialize(datetime.datetime(2017, 8, 1, 19, tzinfo=pytz.UTC))
+
+        patch_policies(self, [StubPolicy([ChannelType.PUSH])])
+        mock_channel = Mock(
+            name='test_channel',
+            channel_type=ChannelType.EMAIL
+        )
+        patch_channels(self, [mock_channel])
+
+        sent_messages = []
+
+        with patch.object(tasks, '_recurring_nudge_schedule_send') as mock_schedule_send:
+            mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args)
+
+            with self.assertNumQueries(1):
+                tasks.recurring_nudge_schedule_hour(
+                    self.site_config.site.id, day, test_time_str, [schedules[0].enrollment.course.org],
+                )
+
+        self.assertEqual(len(sent_messages), 1)
+
+        for args in sent_messages:
+            tasks._recurring_nudge_schedule_send(*args)
+
+        self.assertEqual(mock_channel.deliver.call_count, 1)
+        for (_name, (_msg, email), _kwargs) in mock_channel.deliver.mock_calls:
+            for template in attr.astuple(email):
+                self.assertNotIn("TEMPLATE WARNING", template)

openedx/core/djangoapps/schedules/tasks.py

@@ -1,25 +1,25 @@
 import datetime
-from subprocess import check_output, CalledProcessError
+from itertools import groupby
+from logging import getLogger
 from urlparse import urlparse
 
 from celery.task import task
 from django.conf import settings
+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 Min
 from django.db.utils import DatabaseError
 from django.utils.http import urlquote
-
-from logging import getLogger
-
 from edx_ace import ace
-from edx_ace.message import MessageType, Message
+from edx_ace.message import Message, MessageType
 from edx_ace.recipient import Recipient
 from edx_ace.utils.date import deserialize
-from edxmako.shortcuts import marketing_link
 from opaque_keys.edx.keys import CourseKey
-from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig
 
+from edxmako.shortcuts import marketing_link
+from openedx.core.djangoapps.schedules.models import Schedule, ScheduleConfig
 
 log = getLogger(__name__)
 
@@ -84,14 +84,33 @@ def _recurring_nudge_schedule_send(site_id, msg_str):
 
 
 def _recurring_nudge_schedules_for_hour(target_hour, org_list, exclude_orgs=False):
+    beginning_of_day = target_hour.replace(hour=0, minute=0, second=0)
+    users = User.objects.filter(
+        courseenrollment__schedule__start__gte=beginning_of_day,
+        courseenrollment__schedule__start__lt=beginning_of_day + datetime.timedelta(days=1),
+        courseenrollment__is_active=True,
+    ).annotate(
+        first_schedule=Min('courseenrollment__schedule__start')
+    ).filter(
+        first_schedule__gte=target_hour,
+        first_schedule__lt=target_hour + datetime.timedelta(minutes=60)
+    )
+
+    if org_list is not None:
+        if exclude_orgs:
+            users = users.exclude(courseenrollment__course__org__in=org_list)
+        else:
+            users = users.filter(courseenrollment__course__org__in=org_list)
+
     schedules = Schedule.objects.select_related(
         'enrollment__user__profile',
         'enrollment__course',
     ).filter(
-        start__gte=target_hour,
-        start__lt=target_hour + datetime.timedelta(minutes=60),
+        enrollment__user__in=users,
+        start__gte=beginning_of_day,
+        start__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:
@@ -102,20 +121,24 @@ def _recurring_nudge_schedules_for_hour(target_hour, org_list, exclude_orgs=Fals
     if "read_replica" in settings.DATABASES:
         schedules = schedules.using("read_replica")
 
-    for schedule in schedules:
-        enrollment = schedule.enrollment
-        user = enrollment.user
+    dashboard_relative_url = reverse('dashboard')
 
-        course_id_str = str(enrollment.course_id)
-        course = enrollment.course
+    for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user):
+        user_schedules = list(user_schedules)
+        course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules]
 
-        course_root_relative_url = reverse('course_root', args=[course_id_str])
-        dashboard_relative_url = reverse('dashboard')
+        def absolute_url(relative_path):
+            return u'{}{}'.format(settings.LMS_ROOT_URL, urlquote(relative_path))
 
+        first_schedule = user_schedules[0]
         template_context = {
             'student_name': user.profile.name,
-            'course_name': course.display_name,
-            'course_url': absolute_url(course_root_relative_url),
+
+            'course_name': first_schedule.enrollment.course.display_name,
+            'course_url': absolute_url(reverse('course_root', args=[str(first_schedule.enrollment.course_id)])),
+
+            # This is used by the bulk email optout policy
+            'course_ids': course_id_strs,
 
             # Platform information
             'homepage_url': encode_url(marketing_link('ROOT')),
@@ -125,12 +148,8 @@ def _recurring_nudge_schedules_for_hour(target_hour, org_list, exclude_orgs=Fals
             'contact_mailing_address': settings.CONTACT_MAILING_ADDRESS,
             'social_media_urls': encode_urls_in_dict(getattr(settings, 'SOCIAL_MEDIA_FOOTER_URLS', {})),
             'mobile_store_urls': encode_urls_in_dict(getattr(settings, 'MOBILE_STORE_URLS', {})),
-
-            # This is used by the bulk email optout policy
-            'course_id': course_id_str,
         }
-
-        yield (user, course.language, template_context)
+        yield (user, first_schedule.enrollment.course.language, template_context)
 
 
 def encode_url(url):

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

@@ -2,10 +2,17 @@
 {% load i18n %}
 
 {% block preview_text %}
-    {% blocktrans trimmed %}
-        Keep up the momentum! Many edX learners in {{course_name}} are completing more problems every week, and
-        participating in the discussion forums. What do you want to do to keep learning?
-    {% endblocktrans %}
+    {% if courses|length > 1 %}
+        {% blocktrans trimmed %}
+            Many {{ platform_name }} learners are completing more problems every week, and
+            participating in the discussion forums. What do you want to do to keep learning?
+        {% endblocktrans %}
+    {% else %}
+        {% blocktrans trimmed %}
+            Many {{ platform_name }} learners in {{course_name}} are completing more problems every week, and
+            participating in the discussion forums. What do you want to do to keep learning?
+        {% endblocktrans %}
+    {% endif %}
 {% endblock %}
 
 {% block content %}
@@ -15,14 +22,27 @@
             <h1>{% trans "Keep up the momentum!" %}</h1>
 
             <p>
-                {% blocktrans trimmed %}
-                    Many edX learners in <strong>{{course_name}}</strong> are completing more problems every week, and
-                    participating in the discussion forums. What do you want to do to keep learning?
-                {% endblocktrans %}
+                {% if courses|length > 1 %}
+                    {% blocktrans trimmed %}
+                        Many edX learners are completing more problems every week, and
+                        participating in the discussion forums. What do you want to do to keep learning?
+                    {% endblocktrans %}
+                {% else %}
+                    {% blocktrans trimmed %}
+                        Many edX learners in <strong>{{course_name}}</strong> are completing more problems every week, and
+                        participating in the discussion forums. What do you want to do to keep learning?
+                    {% endblocktrans %}
+                {% endif %}
             </p>
             <p>
                 <!-- email client support for style sheets is pretty spotty, so we have to inline all of these styles -->
-                <a href="{{ course_url }}" style="
+                <a
+                {% if courses|length > 1 %}
+                    href="{{ dashboard_url }}"
+                {% else %}
+                    href="{{ course_url }}"
+                {% endif %}
+                style="
                     color: #ffffff;
                     text-decoration: none;
                     border-radius: 4px;
@@ -42,4 +62,4 @@
         </td>
     </tr>
 </table>
-{% endblock %}
+{% endblock %}

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

@@ -1,7 +1,14 @@
 {% load i18n %}
-{% blocktrans trimmed %}
-    Keep up the momentum! Many edX learners in {{course_name}} are completing more problems every week, and
-    participating in the discussion forums. What do you want to do to keep learning?
-{% endblocktrans %}
-
-{% trans "Keep learning" %} <{{course_url}}>
+{% if courses|length > 1 %}
+    {% blocktrans trimmed %}
+        Many edX learners are completing more problems every week, and
+        participating in the discussion forums. What do you want to do to keep learning?
+    {% endblocktrans %}
+    {% trans "Keep learning" %} <{{dashboard_url}}>
+{% else %}
+    {% blocktrans trimmed %}
+        Many edX learners in {{course_name}} are completing more problems every week, and
+        participating in the discussion forums. What do you want to do to keep learning?
+    {% endblocktrans %}
+    {% trans "Keep learning" %} <{{course_url}}>
+{% endif %}

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

@@ -1 +1,5 @@
-{{ course_name }}
+{% if courses|length > 1 %}
+{{ platform_name }}
+{% else %}
+{{ course_name }}
+{% endif %}

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

@@ -2,10 +2,17 @@
 {% load i18n %}
 
 {% block preview_text %}
-    {% blocktrans trimmed %}
-        Keep learning today. Remember when you enrolled in {{course_name}} on edX.org? We do, and we’re glad
-        to have you! Come see what everyone is learning.
-    {% endblocktrans %}
+    {% if courses|length > 1 %}
+        {% blocktrans trimmed %}
+            Remember when you enrolled in {{ course_name }}, and other courses on edX.org? We do, and we’re glad
+            to have you! Come see what everyone is learning.
+        {% endblocktrans %}
+    {% else %}
+        {% blocktrans trimmed %}
+            Remember when you enrolled in {{ course_name }} on edX.org? We do, and we’re glad
+            to have you! Come see what everyone is learning.
+        {% endblocktrans %}
+    {% endif %}
 {% endblock %}
 
 {% block content %}
@@ -15,15 +22,28 @@
             <h1>{% trans "Keep learning today" %}.</h1>
 
             <p>
-                {% blocktrans trimmed %}
-                    Remember when you enrolled in <strong>{{ course_name }}</strong> on edX.org? We do, and we’re glad
-                    to have you! Come see what everyone is learning.
-                {% endblocktrans %}
+                {% if courses|length > 1 %}
+                    {% blocktrans trimmed %}
+                        Remember when you enrolled in <strong>{{ course_name }}</strong>, and other courses on edX.org? We do, and we’re glad
+                        to have you! Come see what everyone is learning.
+                    {% endblocktrans %}
+                {% else %}
+                    {% blocktrans trimmed %}
+                        Remember when you enrolled in <strong>{{ course_name }}</strong> on edX.org? We do, and we’re glad
+                        to have you! Come see what everyone is learning.
+                    {% endblocktrans %}
+                {% endif %}
             </p>
 
             <p>
                 <!-- email client support for style sheets is pretty spotty, so we have to inline all of these styles -->
-                <a href="{{ course_url }}" style="
+                <a
+                {% if courses|length > 1 %}
+                    href="{{ dashboard_url }}"
+                {% else %}
+                    href="{{ course_url }}"
+                {% endif %}
+                style="
                     color: #ffffff;
                     text-decoration: none;
                     border-radius: 4px;
@@ -37,10 +57,10 @@
                     display: inline-block;
                 ">
                     <!-- old email clients require the use of the font tag :( -->
-                    <font color="#ffffff"><b>{% trans "Keep learning" %}</b></font>
+                    <font color="#ffffff"><b>{% trans "Start learning now" %}</b></font>
                 </a>
             </p>
         </td>
     </tr>
 </table>
-{% endblock %}
+{% endblock %}

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

@@ -1,7 +1,17 @@
 {% load i18n %}
+
+{% if courses|length > 1 %}
 {% blocktrans trimmed %}
-    Keep learning today. Remember when you enrolled in {{course_name}} on edX.org? We do, and we’re glad
+    Remember when you enrolled in {{ course_name }}, and other courses on edX.org? We do, and we’re glad
     to have you! Come see what everyone is learning.
 {% endblocktrans %}
 
-{% trans "Keep learning" %} <{{course_url}}>
+{% trans "Start learning now" %} <{{ dashboard_url }}>
+{% else %}
+{% blocktrans trimmed %}
+    Remember when you enrolled in {{ course_name }} on edX.org? We do, and we’re glad
+    to have you! Come see what everyone is learning.
+{% endblocktrans %}
+
+{% trans "Start learning now" %} <{{ course_url }}>
+{% endif %}

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

@@ -1 +1,5 @@
-{{ course_name }}
+{% if courses|length > 1 %}
+{{ platform_name }}
+{% else %}
+{{ course_name }}
+{% endif %}

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

@@ -1,3 +1,7 @@
 {% load i18n %}
 
-{% blocktrans %}Keep learning in {{course_name}}!{% endblocktrans %}
+{% if courses|length > 1 %}
+{% blocktrans %}Keep learning on {{ platform_name }}!{% endblocktrans %}
+{% else %}
+{% blocktrans %}Keep learning in {{course_name}} !{% endblocktrans %}
+{% endif %}

requirements/edx/base.txt

@@ -38,6 +38,7 @@ django==1.8.18
 django-waffle==0.12.0
 djangorestframework-jwt==1.11.0
 enum34==1.1.6
+edx-ace==0.1.2
 edx-ccx-keys==0.2.1
 edx-celeryutils==0.2.6
 edx-drf-extensions==1.2.3

requirements/edx/github.txt

@@ -97,7 +97,6 @@ git+https://github.com/edx/xblock-utils.git@v1.0.5#egg=xblock-utils==1.0.5
 git+https://github.com/edx/edx-user-state-client.git@1.0.1#egg=edx-user-state-client==1.0.1
 git+https://github.com/edx/xblock-lti-consumer.git@v1.1.5#egg=lti_consumer-xblock==1.1.5
 git+https://github.com/edx/edx-proctoring.git@1.2.0#egg=edx-proctoring==1.2.0
-git+https://github.com/edx/edx-ace.git@v0.1.1#egg=edx-ace==0.1.1
 
 # Third Party XBlocks
 git+https://github.com/open-craft/xblock-poll@7ba819b968fe8faddb78bb22e1fe7637005eb414#egg=xblock-poll==1.2.7