diff --git a/lms/djangoapps/bulk_email/policies.py b/lms/djangoapps/bulk_email/policies.py index a21f9b0068..74259ba7a0 100644 --- a/lms/djangoapps/bulk_email/policies.py +++ b/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()) diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py index 31d0e7b75e..3707365017 100644 --- a/lms/djangoapps/bulk_email/tests/test_course_optout.py +++ b/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)] }, ) diff --git a/openedx/core/djangoapps/content/course_overviews/tests/factories.py b/openedx/core/djangoapps/content/course_overviews/tests/factories.py index 7d8acc497c..c9878e1481 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/factories.py +++ b/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) 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 4d381a5f97..30b5aff256 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 @@ -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) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index e2a409e45e..166e86a30f 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/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): diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/body.html b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/body.html index 6619b83f48..9f35e91fe4 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/body.html +++ b/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 @@

{% trans "Keep up the momentum!" %}

- {% 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 %} + {% 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 {{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 %}

- \ No newline at end of file +{% 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 %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/from_name.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/from_name.txt index dc05f94dca..f07331f68a 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/from_name.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day10/email/from_name.txt @@ -1 +1,5 @@ -{{ course_name }} \ No newline at end of file +{% if courses|length > 1 %} +{{ platform_name }} +{% else %} +{{ course_name }} +{% endif %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html index 370d2dbc60..0ff5da03b8 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html +++ b/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 @@

{% trans "Keep learning today" %}.

- {% 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 %} + {% 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 %}

- - {% trans "Keep learning" %} + {% trans "Start learning now" %}

-{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt index ff31043471..d8f94e1268 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt +++ b/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}}> \ No newline at end of file +{% 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 %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/from_name.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/from_name.txt index dc05f94dca..f07331f68a 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/from_name.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/from_name.txt @@ -1 +1,5 @@ -{{ course_name }} \ No newline at end of file +{% if courses|length > 1 %} +{{ platform_name }} +{% else %} +{{ course_name }} +{% endif %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/subject.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/subject.txt index 1fe793f97b..d60afe9e54 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/subject.txt +++ b/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 %} \ No newline at end of file +{% if courses|length > 1 %} +{% blocktrans %}Keep learning on {{ platform_name }}!{% endblocktrans %} +{% else %} +{% blocktrans %}Keep learning in {{course_name}} !{% endblocktrans %} +{% endif %} diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 862f43e31b..713b203dab 100644 --- a/requirements/edx/base.txt +++ b/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 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index dc8d207890..a81d068a32 100644 --- a/requirements/edx/github.txt +++ b/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