diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index ac5475d213..f7be9c2450 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -97,25 +97,31 @@ BULK_EMAIL_FAILURE_ERRORS = ( ) -def _get_course_email_context(course): +def _get_course_email_context(course, is_secure): """ Returns context arguments to apply to all emails, independent of recipient. + + Inputs are: + * `course`: Course related to the current email + * `is_secure`: Set to True to return https URLs, False to use http. """ course_id = course.id.to_deprecated_string() course_title = course.display_name course_end_date = get_default_time_display(course.end) - course_url = 'https://{}{}'.format( - settings.SITE_NAME, + scheme = u'https' if is_secure else u'http' + base_url = '{}://{}'.format(scheme, settings.SITE_NAME) + course_url = '{}{}'.format( + base_url, reverse('course_root', kwargs={'course_id': course_id}) ) - image_url = u'https://{}{}'.format(settings.SITE_NAME, course_image_url(course)) + image_url = u'{}{}'.format(base_url, course_image_url(course)) email_context = { 'course_title': course_title, 'course_url': course_url, 'course_image_url': image_url, 'course_end_date': course_end_date, - 'account_settings_url': 'https://{}{}'.format(settings.SITE_NAME, reverse('account_settings')), - 'email_settings_url': 'https://{}{}'.format(settings.SITE_NAME, reverse('dashboard')), + 'account_settings_url': '{}{}'.format(base_url, reverse('account_settings')), + 'email_settings_url': '{}{}'.format(base_url, reverse('dashboard')), 'platform_name': configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME), } return email_context @@ -170,9 +176,12 @@ def perform_delegate_email_batches(entry_id, course_id, task_input, action_name) # Fetch the course object. course = get_course(course_id) + # Emails use https URLs by default, to maintain backwards compatibility. + is_secure = task_input.get('is_secure', True) + # Get arguments that will be passed to every subtask. targets = email_obj.targets.all() - global_email_context = _get_course_email_context(course) + global_email_context = _get_course_email_context(course, is_secure) recipient_qsets = [ target.get_users(course_id, user_id) diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 062da01ad9..be33e2b94f 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -3,24 +3,25 @@ Unit tests for sending course email """ import json +import ddt from markupsafe import escape from mock import patch, Mock from nose.plugins.attrib import attr import os from unittest import skipIf -from django.conf import settings from django.core import mail from django.core.mail.message import forbid_multi_line_headers from django.core.urlresolvers import reverse from django.core.management import call_command from django.test.utils import override_settings -from bulk_email.models import Optout, BulkEmailFlag -from bulk_email.tasks import _get_source_address +from bulk_email.models import Optout, BulkEmailFlag, CourseEmail, SEND_TO_MYSELF +from bulk_email.tasks import _get_source_address, _get_course_email_context, perform_delegate_email_batches from openedx.core.djangoapps.course_groups.models import CourseCohort from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort from courseware.tests.factories import StaffFactory, InstructorFactory +from instructor_task.models import InstructorTask from instructor_task.subtasks import update_subtask_status from student.roles import CourseStaffRole from student.models import CourseEnrollment @@ -476,3 +477,87 @@ class TestEmailSendFromDashboard(EmailSendFromDashboardTestCase): message_body = mail.outbox[0].body self.assertIn(uni_message, message_body) + + +@ddt.ddt +class TestCourseEmailContext(SharedModuleStoreTestCase): + """ + Test the course email context hash used to send bulk emails. + """ + + @classmethod + def setUpClass(cls): + """ + Create a course, InstructorTask, and CourseEmail shared by all tests. + """ + super(TestCourseEmailContext, cls).setUpClass() + cls.course_title = u"Финансовое программирование и политика, часть 1: макроэкономические счета и анализ" + cls.course_org = 'IMF' + cls.course_number = "FPP.1x" + cls.course_run = "2016" + cls.course = CourseFactory.create( + display_name=cls.course_title, + org=cls.course_org, + number=cls.course_number, + run=cls.course_run, + ) + cls.user = UserFactory() + cls.course_email = CourseEmail.create( + course_id=cls.course.id, + sender=cls.user, + targets=[SEND_TO_MYSELF], + subject='subject', + html_message='message', + ) + cls.email_task = InstructorTask.create( + course_id=cls.course.id, + task_type='bulk_course_email', + task_key='key', + task_input={}, + requester=cls.user, + ) + + @ddt.data( + (False, 'http'), + (True, 'https'), + ) + @ddt.unpack + def test_context_urls(self, is_secure, scheme): + """ + This test tests that the bulk email context uses http or https urls as appropriate. + """ + email_context = _get_course_email_context(self.course, is_secure) + self.assertEquals(email_context['platform_name'], 'edX') + self.assertEquals(email_context['course_title'], self.course_title) + self.assertEquals(email_context['course_url'], + '{}://edx.org/courses/{}/{}/{}/'.format(scheme, + self.course_org, + self.course_number, + self.course_run)) + self.assertEquals(email_context['course_image_url'], + '{}://edx.org/c4x/{}/{}/asset/images_course_image.jpg'.format(scheme, + self.course_org, + self.course_number)) + self.assertEquals(email_context['email_settings_url'], '{}://edx.org/dashboard'.format(scheme)) + self.assertEquals(email_context['account_settings_url'], '{}://edx.org/account/settings'.format(scheme)) + + @ddt.data( + None, + False, + True, + ) + @patch('bulk_email.tasks._get_course_email_context') + def test_task_input_is_secure(self, is_secure, get_course_email_context): + """ + Test the effect of different task_input hashes on perform_delegate_email_batches, + to ensure that the correct http or https URL schemes are used in generated emails. + """ + task_input = {'email_id': self.course_email.id} + if is_secure is not None: + task_input['is_secure'] = is_secure + else: + # https URLs used by default to maintain backwards compatibility + is_secure = True + + perform_delegate_email_batches(self.email_task.id, self.course.id, task_input, 'emailed') + get_course_email_context.assert_called_with(self.course, is_secure) diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 6301b49bf0..6d1c8e6c8c 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -290,7 +290,7 @@ def submit_bulk_course_email(request, course_key, email_id): task_type = 'bulk_course_email' task_class = send_bulk_course_email - task_input = {'email_id': email_id, 'to_option': targets} + task_input = {'email_id': email_id, 'to_option': targets, 'is_secure': request.is_secure()} task_key_stub = str(email_id) # create the key value by using MD5 hash: task_key = hashlib.md5(task_key_stub).hexdigest() diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 51ec2b5dff..84bc65ea63 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -1,7 +1,7 @@ """ Test for LMS instructor background task queue management """ -from mock import patch, Mock, MagicMock +from mock import patch, Mock, MagicMock, ANY from nose.plugins.attrib import attr from bulk_email.models import CourseEmail, SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS from courseware.tests.factories import UserFactory @@ -30,7 +30,7 @@ from instructor_task.api import ( from instructor_task.api_helper import AlreadyRunningError from instructor_task.models import InstructorTask, PROGRESS -from instructor_task.tasks import export_ora2_data +from instructor_task.tasks import export_ora2_data, send_bulk_course_email from instructor_task.tests.test_base import ( InstructorTaskTestCase, InstructorTaskCourseTestCase, @@ -221,6 +221,23 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa ) self._test_resubmission(api_call) + def test_submit_bulk_email_task_input(self): + """ + Tests the task_input hash created by submit_bulk_course_email. + """ + request = self.create_task_request(self.instructor) + email_id = self._define_course_email() + task_input = dict(email_id=email_id, + to_option=[SEND_TO_MYSELF, SEND_TO_LEARNERS, SEND_TO_STAFF], + is_secure=request.is_secure()) + + with patch('instructor_task.api.submit_task') as mock_submit_task: + mock_submit_task.return_value = MagicMock() + submit_bulk_course_email(request, self.course.id, email_id) + + mock_submit_task.assert_called_once_with( + request, 'bulk_course_email', send_bulk_course_email, self.course.id, task_input, ANY) + def test_submit_calculate_problem_responses(self): api_call = lambda: submit_calculate_problem_responses_csv( self.create_task_request(self.instructor),