Merge pull request #3552 from edx/usman/lms2565-bulk-email-display

When sending bulk email check if course is authorized.

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 = '<a href="#" onclick="goto(\'Email\')" class="selectedmode">Email</a>'

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.
@@ -76,7 +77,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 = '<a href="#" onclick="goto(\'Email\')" class="selectedmode">Email</a>'
@@ -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.

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.

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

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)
 
@@ -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.")

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")

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

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 += "<font color='red'>" + error_msg + "</font>"
-            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 += "<font color='red'>" + error_msg + "</font>"
+                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 = '<div class="msg msg-confirm"><p class="copy">{text}</p></div>'.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 = '<div class="msg msg-confirm"><p class="copy">{text}</p></div>'.format(text=text)
+        else:
+            msg += "<font color='red'>Email is not enabled for this course.</font>"
 
     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:

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()