From c955fd0bce93934be07a917d3f73d083d8dac4b2 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Fri, 2 May 2014 17:50:13 +0500 Subject: [PATCH 1/2] When sending bulk email check if course is authorized. Also the idash_mode property is stored seperately for each course. LMS-2565 --- .../bulk_email/tests/test_course_optout.py | 2 +- lms/djangoapps/bulk_email/tests/test_email.py | 2 +- .../instructor/tests/test_legacy_email.py | 2 +- lms/djangoapps/instructor/views/api.py | 5 ++ .../instructor/views/instructor_dashboard.py | 4 +- lms/djangoapps/instructor/views/legacy.py | 59 ++++++++++--------- lms/djangoapps/instructor/views/tools.py | 23 ++++++++ 7 files changed, 64 insertions(+), 33 deletions(-) diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py index 5cd54686b1..23bcbde954 100644 --- a/lms/djangoapps/bulk_email/tests/test_course_optout.py +++ b/lms/djangoapps/bulk_email/tests/test_course_optout.py @@ -55,7 +55,7 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): # Select the Email view of the instructor dash session = self.client.session - session['idash_mode'] = 'Email' + session[u'idash_mode:{0}'.format(self.course.location.course_id)] = 'Email' session.save() response = self.client.get(url) selected_email_link = 'Email' diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index c96727dfcf..4531c7a374 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -76,7 +76,7 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): # Select the Email view of the instructor dash session = self.client.session - session['idash_mode'] = 'Email' + session[u'idash_mode:{0}'.format(self.course.location.course_id)] = 'Email' session.save() response = self.client.get(self.url) selected_email_link = 'Email' diff --git a/lms/djangoapps/instructor/tests/test_legacy_email.py b/lms/djangoapps/instructor/tests/test_legacy_email.py index c91d1cce2f..0e2642c829 100644 --- a/lms/djangoapps/instructor/tests/test_legacy_email.py +++ b/lms/djangoapps/instructor/tests/test_legacy_email.py @@ -50,7 +50,7 @@ class TestInstructorDashboardEmailView(ModuleStoreTestCase): # Select the Email view of the instructor dash session = self.client.session - session['idash_mode'] = 'Email' + session[u'idash_mode:{0}'.format(self.course.location.course_id)] = 'Email' session.save() response = self.client.get(self.url) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 293d141efe..479a246f02 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -66,6 +66,7 @@ from .tools import ( parse_datetime, set_due_date_extension, strip_if_string, + bulk_email_is_enabled_for_course, ) from xmodule.modulestore import Location @@ -1036,6 +1037,10 @@ def send_email(request, course_id): - 'subject' specifies email's subject - 'message' specifies email's content """ + + if not bulk_email_is_enabled_for_course(course_id): + return HttpResponseForbidden("Email is not enabled for this course.") + send_to = request.POST.get("send_to") subject = request.POST.get("subject") message = request.POST.get("message") diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index fef3e17be4..ed9520420b 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -25,7 +25,7 @@ from student.models import CourseEnrollment from bulk_email.models import CourseAuthorization from class_dashboard.dashboard_data import get_section_display_name, get_array_section_has_problem -from .tools import get_units_with_due_date, title_or_url +from .tools import get_units_with_due_date, title_or_url, bulk_email_is_enabled_for_course @ensure_csrf_cookie @@ -60,7 +60,7 @@ def instructor_dashboard_2(request, course_id): sections.insert(3, _section_extensions(course)) # Gate access to course email by feature flag & by course-specific authorization - if settings.FEATURES['ENABLE_INSTRUCTOR_EMAIL'] and is_studio_course and CourseAuthorization.instructor_email_enabled(course_id): + if bulk_email_is_enabled_for_course(course_id): sections.append(_section_send_email(course_id, access, course)) # Gate access to Metrics tab by featue flag and staff authorization diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index adf9eaa6c7..7f62501a12 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -48,7 +48,7 @@ from django_comment_common.models import ( ) from django_comment_client.utils import has_forum_access from instructor.offline_gradecalc import student_grades, offline_grades_available -from instructor.views.tools import strip_if_string +from instructor.views.tools import strip_if_string, bulk_email_is_enabled_for_course from instructor_task.api import ( get_running_instructor_tasks, get_instructor_task_history, @@ -110,10 +110,11 @@ def instructor_dashboard(request, course_id): # the instructor dashboard page is modal: grades, psychometrics, admin # keep that state in request.session (defaults to grades mode) idash_mode = request.POST.get('idash_mode', '') + idash_mode_key = u'idash_mode:{0}'.format(course_id) if idash_mode: - request.session['idash_mode'] = idash_mode + request.session[idash_mode_key] = idash_mode else: - idash_mode = request.session.get('idash_mode', 'Grades') + idash_mode = request.session.get(idash_mode_key, 'Grades') enrollment_number = CourseEnrollment.num_enrolled_in(course_id) @@ -760,33 +761,36 @@ def instructor_dashboard(request, course_id): email_subject = request.POST.get("subject") html_message = request.POST.get("message") - try: - # Create the CourseEmail object. This is saved immediately, so that - # any transaction that has been pending up to this point will also be - # committed. - email = CourseEmail.create(course_id, request.user, email_to_option, email_subject, html_message) + if bulk_email_is_enabled_for_course(course_id): + try: + # Create the CourseEmail object. This is saved immediately, so that + # any transaction that has been pending up to this point will also be + # committed. + email = CourseEmail.create(course_id, request.user, email_to_option, email_subject, html_message) - # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) - submit_bulk_course_email(request, course_id, email.id) # pylint: disable=E1101 + # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) + submit_bulk_course_email(request, course_id, email.id) # pylint: disable=E1101 - except Exception as err: - # Catch any errors and deliver a message to the user - error_msg = "Failed to send email! ({0})".format(err) - msg += "" + error_msg + "" - log.exception(error_msg) + except Exception as err: + # Catch any errors and deliver a message to the user + error_msg = "Failed to send email! ({0})".format(err) + msg += "" + error_msg + "" + log.exception(error_msg) - else: - # If sending the task succeeds, deliver a success message to the user. - if email_to_option == "all": - text = _( - "Your email was successfully queued for sending. " - "Please note that for large classes, it may take up to an hour " - "(or more, if other courses are simultaneously sending email) " - "to send all emails." - ) else: - text = _('Your email was successfully queued for sending.') - email_msg = '

{text}

'.format(text=text) + # If sending the task succeeds, deliver a success message to the user. + if email_to_option == "all": + text = _( + "Your email was successfully queued for sending. " + "Please note that for large classes, it may take up to an hour " + "(or more, if other courses are simultaneously sending email) " + "to send all emails." + ) + else: + text = _('Your email was successfully queued for sending.') + email_msg = '

{text}

'.format(text=text) + else: + msg += "Email is not enabled for this course." elif "Show Background Email Task History" in action: message, datatable = get_background_task_table(course_id, task_type='bulk_course_email') @@ -895,8 +899,7 @@ def instructor_dashboard(request, course_id): # 1. Feature flag is on # 2. We have explicitly enabled email for the given course via django-admin # 3. It is NOT an XML course - if settings.FEATURES['ENABLE_INSTRUCTOR_EMAIL'] and \ - CourseAuthorization.instructor_email_enabled(course_id) and is_studio_course: + if bulk_email_is_enabled_for_course(course_id): show_email_tab = True # display course stats only if there is no other table to display: diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index 703610cb64..322ef2c898 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -4,6 +4,7 @@ Tools for the instructor dashboard import dateutil import json +from django.conf import settings from django.contrib.auth.models import User from django.http import HttpResponseBadRequest from django.utils.timezone import utc @@ -11,6 +12,10 @@ from django.utils.translation import ugettext as _ from courseware.models import StudentModule from xmodule.fields import Date +from xmodule.modulestore import XML_MODULESTORE_TYPE +from xmodule.modulestore.django import modulestore + +from bulk_email.models import CourseAuthorization DATE_FIELD = Date() @@ -45,6 +50,24 @@ def handle_dashboard_error(view): return wrapper +def bulk_email_is_enabled_for_course(course_id): + """ + Staff can only send bulk email for a course if all the following conditions are true: + 1. Bulk email feature flag is on. + 2. It is a studio course. + 3. Bulk email is enabled for the course. + """ + + bulk_email_enabled_globally = (settings.FEATURES['ENABLE_INSTRUCTOR_EMAIL'] == True) + is_studio_course = (modulestore().get_modulestore_type(course_id) != XML_MODULESTORE_TYPE) + bulk_email_enabled_for_course = CourseAuthorization.instructor_email_enabled(course_id) + + if bulk_email_enabled_globally and is_studio_course and bulk_email_enabled_for_course: + return True + + return False + + def strip_if_string(value): if isinstance(value, basestring): return value.strip() From 8ee682d44bfe7ca47cd142090fe5b71547f6c5b4 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Fri, 2 May 2014 19:53:26 +0500 Subject: [PATCH 2/2] Fixed and added more tests for bulk email. LMS-2565 --- lms/djangoapps/bulk_email/tests/test_email.py | 15 ++++++++ .../bulk_email/tests/test_err_handling.py | 1 + lms/djangoapps/instructor/tests/test_api.py | 3 ++ .../instructor/tests/test_legacy_email.py | 35 +++++++++++++++++++ 4 files changed, 54 insertions(+) diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 4531c7a374..d1b8e380e9 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -41,6 +41,7 @@ class MockCourseEmailResult(object): @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +@patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) class TestEmailSendFromDashboard(ModuleStoreTestCase): """ Test that emails send correctly. @@ -88,6 +89,20 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): """ patch.stopall() + @patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': True}) + def test_email_disabled(self): + """ + Test response when email is disabled for course. + """ + test_email = { + 'action': 'Send email', + 'to_option': 'myself', + 'subject': 'test subject for myself', + 'message': 'test message for myself' + } + response = self.client.post(self.url, test_email) + self.assertContains(response, "Email is not enabled for this course.") + def test_send_to_self(self): """ Make sure email send to myself goes to myself. diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index f9365c86ef..f7b8b32d1f 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -38,6 +38,7 @@ class EmailTestException(Exception): @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +@patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) class TestEmailErrors(ModuleStoreTestCase): """ Test that errors from sending email are handled properly. diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index daf46a3db5..e84c79cfce 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -11,6 +11,7 @@ from urllib import quote from django.test import TestCase from nose.tools import raises from mock import Mock, patch +from django.conf import settings from django.test.utils import override_settings from django.core.urlresolvers import reverse from django.http import HttpRequest, HttpResponse @@ -99,6 +100,7 @@ class TestCommonExceptions400(unittest.TestCase): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +@patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Ensure that users cannot access endpoints they shouldn't be able to. @@ -1570,6 +1572,7 @@ class TestInstructorAPIRegradeTask(ModuleStoreTestCase, LoginEnrollmentTestCase) @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +@patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) class TestInstructorSendEmail(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Checks that only instructors have access to email endpoints, and that diff --git a/lms/djangoapps/instructor/tests/test_legacy_email.py b/lms/djangoapps/instructor/tests/test_legacy_email.py index 0e2642c829..823d112b3f 100644 --- a/lms/djangoapps/instructor/tests/test_legacy_email.py +++ b/lms/djangoapps/instructor/tests/test_legacy_email.py @@ -108,3 +108,38 @@ class TestInstructorDashboardEmailView(ModuleStoreTestCase): # Assert that the URL for the email view is not in the response response = self.client.get(self.url) self.assertFalse(self.email_link in response.content) + + @patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True}) + def test_send_mail_unauthorized(self): + """ Test 'Send email' action returns an error if course is not authorized to send email. """ + + response = self.client.post( + self.url, { + 'action': 'Send email', + 'to_option': 'all', + 'subject': "Welcome to the course!", + 'message': "Lets start with an introduction!" + } + ) + self.assertContains(response, "Email is not enabled for this course.") + + @patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True}) + def test_send_mail_authorized(self): + """ Test 'Send email' action when course is authorized to send email. """ + + course_authorization = CourseAuthorization(course_id=self.course.id, email_enabled=True) + course_authorization.save() + + session = self.client.session + session[u'idash_mode:{0}'.format(self.course.location.course_id)] = 'Email' + session.save() + + response = self.client.post( + self.url, { + 'action': 'Send email', + 'to_option': 'all', + 'subject': 'Welcome to the course!', + 'message': 'Lets start with an introduction!', + } + ) + self.assertContains(response, "Your email was successfully queued for sending.")