diff --git a/common/djangoapps/enrollment/api.py b/common/djangoapps/enrollment/api.py index d97e31111f..e35539479c 100644 --- a/common/djangoapps/enrollment/api.py +++ b/common/djangoapps/enrollment/api.py @@ -192,7 +192,7 @@ def add_enrollment(user_id, course_id, mode=None, is_active=True): """ if mode is None: mode = _default_course_mode(course_id) - _validate_course_mode(course_id, mode, is_active=is_active) + validate_course_mode(course_id, mode, is_active=is_active) return _data_api().create_course_enrollment(user_id, course_id, mode, is_active) @@ -252,7 +252,7 @@ def update_enrollment(user_id, course_id, mode=None, is_active=None, enrollment_ mode=mode, )) if mode is not None: - _validate_course_mode(course_id, mode, is_active=is_active, include_expired=include_expired) + validate_course_mode(course_id, mode, is_active=is_active, include_expired=include_expired) enrollment = _data_api().update_course_enrollment(user_id, course_id, mode=mode, is_active=is_active) if enrollment is None: msg = u"Course Enrollment not found for user {user} in course {course}".format(user=user_id, course=course_id) @@ -409,7 +409,7 @@ def _default_course_mode(course_id): return CourseMode.DEFAULT_MODE_SLUG -def _validate_course_mode(course_id, mode, is_active=None, include_expired=False): +def validate_course_mode(course_id, mode, is_active=None, include_expired=False): """Checks to see if the specified course mode is valid for the course. If the requested course mode is not available for the course, raise an error with corresponding diff --git a/lms/djangoapps/bulk_email/migrations/0006_course_mode_targets.py b/lms/djangoapps/bulk_email/migrations/0006_course_mode_targets.py new file mode 100644 index 0000000000..ec294d95c3 --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0006_course_mode_targets.py @@ -0,0 +1,28 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_modes', '0007_coursemode_bulk_sku'), + ('bulk_email', '0005_move_target_data'), + ] + + operations = [ + migrations.CreateModel( + name='CourseModeTarget', + fields=[ + ('target_ptr', models.OneToOneField(parent_link=True, auto_created=True, primary_key=True, serialize=False, to='bulk_email.Target')), + ('track', models.ForeignKey(to='course_modes.CourseMode')), + ], + bases=('bulk_email.target',), + ), + migrations.AlterField( + model_name='target', + name='target_type', + field=models.CharField(max_length=64, choices=[(b'myself', b'Myself'), (b'staff', b'Staff and instructors'), (b'learners', b'All students'), (b'cohort', b'Specific cohort'), (b'track', b'Specific course mode')]), + ), + ] diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 34c8b53397..09b6c46a6b 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/lms/djangoapps/bulk_email/models.py @@ -13,6 +13,9 @@ from openedx.core.lib.html_to_text import html_to_text from openedx.core.lib.mail_utils import wrap_message from config_models.models import ConfigurationModel +from course_modes.models import CourseMode +from enrollment.api import validate_course_mode +from enrollment.errors import CourseModeNotFoundError from student.roles import CourseStaffRole, CourseInstructorRole from openedx.core.djangoapps.xmodule_django.models import CourseKeyField @@ -45,9 +48,10 @@ SEND_TO_MYSELF = 'myself' SEND_TO_STAFF = 'staff' SEND_TO_LEARNERS = 'learners' SEND_TO_COHORT = 'cohort' +SEND_TO_TRACK = 'track' EMAIL_TARGET_CHOICES = zip( - [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS, SEND_TO_COHORT], - ['Myself', 'Staff and instructors', 'All students', 'Specific cohort'] + [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS, SEND_TO_COHORT, SEND_TO_TRACK], + ['Myself', 'Staff and instructors', 'All students', 'Specific cohort', 'Specific course mode'] ) EMAIL_TARGETS = {target[0] for target in EMAIL_TARGET_CHOICES} @@ -78,6 +82,8 @@ class Target(models.Model): """ if self.target_type == SEND_TO_COHORT: return self.cohorttarget.short_display() # pylint: disable=no-member + elif self.target_type == SEND_TO_TRACK: + return self.coursemodetarget.short_display() # pylint: disable=no-member else: return self.target_type @@ -87,6 +93,8 @@ class Target(models.Model): """ if self.target_type == SEND_TO_COHORT: return self.cohorttarget.long_display() # pylint: disable=no-member + elif self.target_type == SEND_TO_TRACK: + return self.coursemodetarget.long_display() # pylint: disable=no-member else: return self.get_target_type_display() @@ -115,6 +123,12 @@ class Target(models.Model): return use_read_replica_if_available(enrollment_qset.exclude(id__in=staff_instructor_qset)) elif self.target_type == SEND_TO_COHORT: return self.cohorttarget.cohort.users.filter(id__in=enrollment_qset) # pylint: disable=no-member + elif self.target_type == SEND_TO_TRACK: + return use_read_replica_if_available( + enrollment_qset.filter( + courseenrollment__mode=self.coursemodetarget.track.mode_slug + ) + ) else: raise ValueError("Unrecognized target type {}".format(self.target_type)) @@ -132,6 +146,9 @@ class CohortTarget(Target): kwargs['target_type'] = SEND_TO_COHORT super(CohortTarget, self).__init__(*args, **kwargs) + def __unicode__(self): + return self.short_display() + def short_display(self): return "{}-{}".format(self.target_type, self.cohort.name) @@ -159,6 +176,53 @@ class CohortTarget(Target): return cohort +class CourseModeTarget(Target): + """ + Subclass of Target, specifically for course modes. + """ + track = models.ForeignKey('course_modes.CourseMode') + + class Meta: + app_label = "bulk_email" + + def __init__(self, *args, **kwargs): + kwargs['target_type'] = SEND_TO_TRACK + super(CourseModeTarget, self).__init__(*args, **kwargs) + + def __unicode__(self): + return self.short_display() + + def short_display(self): + return "{}-{}".format(self.target_type, self.track.mode_slug) # pylint: disable=no-member + + def long_display(self): + all_modes = CourseMode.objects.filter( + course_id=self.track.course_id, + mode_slug=self.track.mode_slug, # pylint: disable=no-member + ) + return "Course mode: {}, Currencies: {}".format( + self.track.mode_display_name, # pylint: disable=no-member + ", ".join([mode.currency for mode in all_modes]) + ) + + @classmethod + def ensure_valid_mode(cls, mode_slug, course_id): + """ + Ensures mode_slug is a valid mode for course_id. Will raise an error if invalid. + """ + if mode_slug is None: + raise ValueError("Cannot create a CourseModeTarget without specifying a mode_slug.") + try: + validate_course_mode(unicode(course_id), mode_slug) + except CourseModeNotFoundError: + raise ValueError( + "Track {track} does not exist in course {course_id}".format( + track=mode_slug, + course_id=course_id + ) + ) + + class CourseEmail(Email): """ Stores information for an email to a course. @@ -179,7 +243,7 @@ class CourseEmail(Email): @classmethod def create( cls, course_id, sender, targets, subject, html_message, - text_message=None, template_name=None, from_addr=None, cohort_name=None): + text_message=None, template_name=None, from_addr=None): """ Create an instance of CourseEmail. """ @@ -189,7 +253,7 @@ class CourseEmail(Email): new_targets = [] for target in targets: - # split target, to handle cohort:cohort_name + # split target, to handle cohort:cohort_name and track:mode_slug target_split = target.split(':', 1) # Ensure our desired target exists if target_split[0] not in EMAIL_TARGETS: @@ -200,6 +264,14 @@ class CourseEmail(Email): # target_split[1] will contain the cohort name cohort = CohortTarget.ensure_valid_cohort(target_split[1], course_id) new_target, _ = CohortTarget.objects.get_or_create(target_type=target_split[0], cohort=cohort) + elif target_split[0] == SEND_TO_TRACK: + # target_split[1] contains the desired mode slug + CourseModeTarget.ensure_valid_mode(target_split[1], course_id) + + # There could exist multiple CourseModes that match this query, due to differing currency types. + # The currencies do not affect user lookup though, so we can just use the first result. + mode = CourseMode.objects.filter(course_id=course_id, mode_slug=target_split[1])[0] + new_target, _ = CourseModeTarget.objects.get_or_create(target_type=target_split[0], track=mode) else: new_target, _ = Target.objects.get_or_create(target_type=target_split[0]) new_targets.append(new_target) diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index d79604ad79..1834eedcb4 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -20,7 +20,9 @@ from bulk_email.models import Optout, BulkEmailFlag from bulk_email.tasks import _get_source_address, _get_course_email_context from openedx.core.djangoapps.course_groups.models import CourseCohort from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort +from course_modes.models import CourseMode from courseware.tests.factories import StaffFactory, InstructorFactory +from enrollment.api import update_enrollment from lms.djangoapps.instructor_task.subtasks import update_subtask_status from student.roles import CourseStaffRole from student.models import CourseEnrollment @@ -239,6 +241,27 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) self.assertEquals(len(mail.outbox), len(self.students) - 1) self.assertNotIn(self.students[-1].email, [e.to[0] for e in mail.outbox]) + def test_send_to_track(self): + """ + Make sure email sent to a registration track goes there. + """ + CourseMode.objects.create(mode_slug='test', course_id=self.course.id) + for student in self.students: + update_enrollment(student, unicode(self.course.id), 'test') + test_email = { + 'action': 'Send email', + 'send_to': '["track:test"]', + 'subject': 'test subject for test track', + 'message': 'test message for test track', + } + response = self.client.post(self.send_mail_url, test_email) + self.assertEquals(json.loads(response.content), self.success_content) + + self.assertItemsEqual( + [e.to[0] for e in mail.outbox], + [s.email for s in self.students] + ) + def test_send_to_all(self): """ Make sure email send to all goes there. diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index 6a3a46f7b8..2f7b9bd4e9 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -5,6 +5,7 @@ Unit tests for handling email sending errors from itertools import cycle from celery.states import SUCCESS, RETRY # pylint: disable=no-name-in-module, import-error +import ddt from django.conf import settings from django.core.management import call_command from django.core.urlresolvers import reverse @@ -36,6 +37,7 @@ class EmailTestException(Exception): pass +@ddt.ddt @attr(shard=1) @patch('bulk_email.models.html_to_text', Mock(return_value='Mocking CourseEmail.text_message', autospec=True)) class TestEmailErrors(ModuleStoreTestCase): @@ -213,15 +215,16 @@ class TestEmailErrors(ModuleStoreTestCase): "dummy body goes here" ) - def test_nonexistent_cohort(self): + @ddt.data('track', 'cohort') + def test_nonexistent_grouping(self, target_type): """ - Tests exception when the cohort doesn't exist + Tests exception when the cohort or course mode doesn't exist """ - with self.assertRaisesRegexp(ValueError, 'Cohort IDONTEXIST does not exist *'): + with self.assertRaisesRegexp(ValueError, '.* IDONTEXIST does not exist .*'): email = CourseEmail.create( # pylint: disable=unused-variable self.course.id, self.instructor, - ["cohort:IDONTEXIST"], + ["{}:IDONTEXIST".format(target_type)], "re: subject", "dummy body goes here" ) diff --git a/lms/djangoapps/bulk_email/tests/test_models.py b/lms/djangoapps/bulk_email/tests/test_models.py index b0b588f112..9fce4ebf4d 100644 --- a/lms/djangoapps/bulk_email/tests/test_models.py +++ b/lms/djangoapps/bulk_email/tests/test_models.py @@ -12,11 +12,13 @@ from nose.plugins.attrib import attr from bulk_email.models import ( CourseEmail, SEND_TO_COHORT, + SEND_TO_TRACK, SEND_TO_STAFF, CourseEmailTemplate, CourseAuthorization, BulkEmailFlag ) +from course_modes.models import CourseMode from openedx.core.djangoapps.course_groups.models import CourseCohort from opaque_keys.edx.keys import CourseKey @@ -67,6 +69,22 @@ class CourseEmailTest(TestCase): with self.assertRaises(ValueError): CourseEmail.create(course_id, sender, to_option, subject, html_message) + def test_track_target(self): + course_id = CourseKey.from_string('abc/123/doremi') + sender = UserFactory.create() + to_option = 'track:test' + subject = "dummy subject" + html_message = "dummy message" + CourseMode.objects.create(mode_slug='test', mode_display_name='Test', course_id=course_id) + with patch('bulk_email.models.validate_course_mode'): + # we don't have a real course, so validation will fail. Mock it out! + email = CourseEmail.create(course_id, sender, [to_option], subject, html_message) + self.assertEqual(len(email.targets.all()), 1) + target = email.targets.all()[0] + self.assertEqual(target.target_type, SEND_TO_TRACK) + self.assertEqual(target.short_display(), 'track-test') + self.assertEqual(target.long_display(), 'Course mode: Test, Currencies: usd') + def test_cohort_target(self): course_id = CourseKey.from_string('abc/123/doremi') sender = UserFactory.create() diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 75eeae3895..de87be7c1b 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -633,6 +633,10 @@ def _section_send_email(course, access): cohorts = [] if is_course_cohorted(course_key): cohorts = get_course_cohorts(course) + course_modes = [] + from verified_track_content.models import VerifiedTrackCohortedCourse + if not VerifiedTrackCohortedCourse.is_verified_track_cohort_enabled(course_key): + course_modes = CourseMode.modes_for_course(course_key) email_editor = fragment.content section_data = { 'section_key': 'send_email', @@ -641,6 +645,7 @@ def _section_send_email(course, access): 'send_email': reverse('send_email', kwargs={'course_id': unicode(course_key)}), 'editor': email_editor, 'cohorts': cohorts, + 'course_modes': course_modes, 'default_cohort_name': DEFAULT_COHORT_NAME, 'list_instructor_tasks_url': reverse( 'list_instructor_tasks', kwargs={'course_id': unicode(course_key)} diff --git a/lms/static/js/instructor_dashboard/send_email.js b/lms/static/js/instructor_dashboard/send_email.js index d20a54b42c..16cc1c1919 100644 --- a/lms/static/js/instructor_dashboard/send_email.js +++ b/lms/static/js/instructor_dashboard/send_email.js @@ -41,6 +41,7 @@ this.$emailEditor = XBlock.initializeBlock($('.xblock-studio_view')); this.$send_to = this.$container.find("input[name='send_to']"); this.$cohort_targets = this.$send_to.filter('[value^="cohort:"]'); + this.$course_mode_targets = this.$send_to.filter('[value^="track:"]'); this.$subject = this.$container.find("input[name='subject']"); this.$btn_send = this.$container.find("input[name='send']"); this.$task_response = this.$container.find('.request-response'); @@ -85,9 +86,12 @@ return gettext('Everyone who has staff privileges in this course'); } else if (value === 'learners') { return gettext('All learners who are enrolled in this course'); - } else { + } else if (value.startsWith('cohort')) { return gettext('All learners in the {cohort_name} cohort') .replace('{cohort_name}', value.slice(value.indexOf(':') + 1)); + } else if (value.startsWith('track')) { + return gettext('All learners in the {track_name} track') + .replace('{track_name}', value.slice(value.indexOf(':') + 1)); } }; successMessage = gettext('Your email message was successfully queued for sending. In courses with a large number of learners, email messages to learners might take up to an hour to be sent.'); // eslint-disable-line max-len @@ -179,17 +183,21 @@ }); this.$send_to.change(function() { var targets; + var inputDisable = function() { + this.checked = false; + this.disabled = true; + return true; + }; + var inputEnable = function() { + this.disabled = false; + return true; + }; if ($('input#target_learners:checked').length) { - sendemail.$cohort_targets.each(function() { - this.checked = false; - this.disabled = true; - return true; - }); + sendemail.$cohort_targets.each(inputDisable); + sendemail.$course_mode_targets.each(inputDisable); } else { - sendemail.$cohort_targets.each(function() { - this.disabled = false; - return true; - }); + sendemail.$cohort_targets.each(inputEnable); + sendemail.$cohort_targets.each(inputEnable); } targets = []; $('input[name="send_to"]:checked+label').each(function() { diff --git a/lms/templates/instructor/instructor_dashboard_2/send_email.html b/lms/templates/instructor/instructor_dashboard_2/send_email.html index 1af83d4267..b19d1ef4ed 100644 --- a/lms/templates/instructor/instructor_dashboard_2/send_email.html +++ b/lms/templates/instructor/instructor_dashboard_2/send_email.html @@ -45,6 +45,19 @@ from openedx.core.djangolib.markup import HTML %endfor %endif + ## If there's only 1 course mode, everyone is in it and you should just use "All Learners" + %if len(section_data['course_modes']) > 1: + + %endif