Merge pull request #5449 from Stanford-Online/njdup/bulk_emails_keyword_sub

Implements keyword sub feature for bulk emails

common/djangoapps/util/keyword_substitution.py

@@ -0,0 +1,82 @@
+"""
+keyword_substitution.py
+
+Contains utility functions to help substitute keywords in a text body with
+the appropriate user / course data.
+
+Supported:
+    LMS:
+        - %%USER_ID%% => anonymous user id
+        - %%USER_FULLNAME%% => User's full name
+        - %%COURSE_DISPLAY_NAME%% => display name of the course
+        - %%COURSE_END_DATE%% => end date of the course
+
+Usage:
+    KEYWORD_FUNCTION_MAP must be supplied in startup.py, so that it lives
+    above other modules in the dependency tree and acts like a global var.
+    Then we can call substitute_keywords_with_data where substitution is
+    needed. Currently called in:
+        - LMS: Announcements + Bulk emails
+        - CMS: Not called
+"""
+
+from django.contrib.auth.models import User
+from xmodule.modulestore.django import modulestore
+
+KEYWORD_FUNCTION_MAP = {}
+
+
+def keyword_function_map_is_empty():
+    """
+    Checks if the keyword function map has been filled
+    """
+    return not bool(KEYWORD_FUNCTION_MAP)
+
+
+def add_keyword_function_map(mapping):
+    """
+    Attaches the given keyword-function mapping to the existing one
+    """
+    KEYWORD_FUNCTION_MAP.update(mapping)
+
+
+def substitute_keywords(string, user=None, course=None):
+    """
+    Replaces all %%-encoded words using KEYWORD_FUNCTION_MAP mapping functions
+
+    Iterates through all keywords that must be substituted and replaces
+    them by calling the corresponding functions stored in KEYWORD_FUNCTION_MAP.
+
+    Functions stored in KEYWORD_FUNCTION_MAP must return a replacement string.
+    Also, functions imported from other modules must be wrapped in a
+    new function if they don't take in user_id and course_id. This simplifies
+    the loop below, and reduces the need for piling up if elif else statements
+    when the keyword pool grows.
+    """
+    if user is None or course is None:
+        # Cannot proceed without course and user information
+        return string
+
+    for key, func in KEYWORD_FUNCTION_MAP.iteritems():
+        if key in string:
+            substitutor = func(user, course)
+            string = string.replace(key, substitutor)
+
+    return string
+
+
+def substitute_keywords_with_data(string, user_id=None, course_id=None):
+    """
+    Given user and course ids, replaces all %%-encoded words in the given string
+    """
+
+    # Do not proceed without parameters: Compatibility check with existing tests
+    # that do not supply these parameters
+    if user_id is None or course_id is None:
+        return string
+
+    # Grab user objects
+    user = User.objects.get(id=user_id)
+    course = modulestore().get_course(course_id, depth=0)
+
+    return substitute_keywords(string, user, course)

common/djangoapps/util/tests/fixtures/test_keyword_anonid_sub.json

@@ -0,0 +1,14 @@
+{
+    "standard_test": {
+        "test_string": "This is a test string. sub this: %%USER_ID%% into anon_id",
+        "expected": "This is a test string. sub this: 123456789 into anon_id"
+    },
+    "multiple_subs": {
+        "test_string": "There are a lot of anonymous ids here: %%USER_ID%% %%USER_ID%% %%USER_ID%% %%USER_ID%%",
+        "expected": "There are a lot of anonymous ids here: 123456789 123456789 123456789 123456789"
+    },
+    "sub_with_html": {
+        "test_string": "There is html in this guy <b>%%USER_ID%%</b>",
+        "expected": "There is html in this guy <b>123456789</b>"
+    }
+}

common/djangoapps/util/tests/fixtures/test_keyword_coursename_sub.json

@@ -0,0 +1,14 @@
+{
+    "simple_test": {
+        "test_string": "Course display name: %%COURSE_DISPLAY_NAME%%",
+        "expected": "Course display name: test_course"
+    },
+    "Multiple_test": {
+        "test_string": "We display %%COURSE_DISPLAY_NAME%% a lot here: %%COURSE_DISPLAY_NAME%% %%COURSE_DISPLAY_NAME%%",
+        "expected": "We display test_course a lot here: test_course test_course"
+    },
+    "Html_test": {
+        "test_string": "This string has some html: <h1>%%COURSE_DISPLAY_NAME%%</h1>",
+        "expected": "This string has some html: <h1>test_course</h1>"
+    }
+}

common/djangoapps/util/tests/fixtures/test_keywordsub_multiple_tags.json

@@ -0,0 +1,10 @@
+{
+    "basic_test": {
+        "test_string": "The user with id %%USER_ID%% is named %%USER_FULLNAME%%",
+        "expected": "The user with id 123456789 is named Test User"
+    },
+    "invalid_tags": {
+        "test_string": "The user with id %%user-id%% is named %%USER_FULLNAME%% and is in %%COURSE_DISPLAY_NAME",
+        "expected": "The user with id %%user-id%% is named Test User and is in test_course"
+    }
+}

common/djangoapps/util/tests/test_keyword_sub_utils.py

@@ -0,0 +1,152 @@
+"""
+Tests for keyword_substitution.py
+"""
+
+from student.tests.factories import UserFactory
+from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from ddt import ddt, file_data
+from mock import patch
+
+
+from util.date_utils import get_default_time_display
+from util import keyword_substitution as Ks
+
+
+@ddt
+class KeywordSubTest(ModuleStoreTestCase):
+    """ Tests for the keyword substitution feature """
+
+    def setUp(self):
+        self.user = UserFactory.create(
+            email="testuser@edx.org",
+            username="testuser",
+            profile__name="Test User"
+        )
+        self.course = CourseFactory.create(
+            org='edx',
+            course='999',
+            display_name='test_course'
+        )
+
+        # Mimic monkeypatching done in startup.py
+        Ks.KEYWORD_FUNCTION_MAP = self.get_keyword_function_map()
+
+    def get_keyword_function_map(self):
+        """
+        Generates a mapping from keywords to functions for testing
+
+        The keyword sub functions should not return %% encoded strings. This
+        would lead to ugly and inconsistent behavior due to the way in which
+        keyword subbing is handled.
+        """
+
+        def user_fullname_sub(user, course):  # pylint: disable=unused-argument
+            """ Returns the user's name """
+            return user.profile.name
+
+        def course_display_name_sub(user, course):  # pylint: disable=unused-argument
+            """ Returns the course name """
+            return course.display_name
+
+        return {
+            '%%USER_FULLNAME%%': user_fullname_sub,
+            '%%COURSE_DISPLAY_NAME%%': course_display_name_sub,
+        }
+
+    @file_data('fixtures/test_keyword_coursename_sub.json')
+    def test_course_name_sub(self, test_info):
+        """ Tests subbing course name in various scenarios """
+        course_name = self.course.display_name
+        result = Ks.substitute_keywords_with_data(test_info['test_string'], self.user.id, self.course.id)
+
+        self.assertIn(course_name, result)
+        self.assertEqual(result, test_info['expected'])
+
+    @file_data('fixtures/test_keyword_anonid_sub.json')
+    def test_anonymous_id_subs(self, test_info):
+        """ Tests subbing anon user id in various scenarios """
+        anon_id = '123456789'
+        with patch.dict(Ks.KEYWORD_FUNCTION_MAP, {'%%USER_ID%%': lambda x, y: anon_id}):
+            result = Ks.substitute_keywords_with_data(test_info['test_string'], self.user.id, self.course.id)
+
+            self.assertIn(anon_id, result)
+            self.assertEqual(result, test_info['expected'])
+
+    def test_name_sub(self):
+        test_string = "This is the test string. subthis:  %%USER_FULLNAME%% into user name"
+        user_name = self.user.profile.name
+        result = Ks.substitute_keywords_with_data(test_string, self.user.id, self.course.id)
+
+        self.assertNotIn('%%USER_FULLNAME%%', result)
+        self.assertIn(user_name, result)
+
+    def test_illegal_subtag(self):
+        """
+        Test that sub-ing doesn't ocurr with illegal tags
+        """
+        test_string = "%%user_id%%"
+        result = Ks.substitute_keywords_with_data(test_string, self.user.id, self.course.id)
+
+        self.assertEquals(test_string, result)
+
+    def test_should_not_sub(self):
+        """
+        Test that sub-ing doesn't work without subtags
+        """
+        test_string = "this string has no subtags"
+        result = Ks.substitute_keywords_with_data(test_string, self.user.id, self.course.id)
+
+        self.assertEquals(test_string, result)
+
+    @file_data('fixtures/test_keywordsub_multiple_tags.json')
+    def test_sub_mutiltple_tags(self, test_info):
+        """ Test that subbing works with multiple subtags """
+        anon_id = '123456789'
+        patched_dict = {
+            '%%USER_ID%%': lambda x, y: anon_id,
+            '%%USER_FULLNAME%%': lambda x, y: self.user.profile.name,
+            '%%COURSE_DISPLAY_NAME': lambda x, y: self.course.display_name,
+            '%%COURSE_END_DATE': lambda x, y: get_default_time_display(self.course.end)
+        }
+
+        with patch.dict(Ks.KEYWORD_FUNCTION_MAP, patched_dict):
+            result = Ks.substitute_keywords_with_data(test_info['test_string'], self.user.id, self.course.id)
+            self.assertEqual(result, test_info['expected'])
+
+    def test_no_subbing_empty_subtable(self):
+        """
+        Test that no sub-ing occurs when the sub table is empty.
+        """
+        # Set the keyword sub mapping to be empty
+        Ks.KEYWORD_FUNCTION_MAP = {}
+
+        test_string = 'This user\'s name is %%USER_FULLNAME%%'
+        result = Ks.substitute_keywords_with_data(test_string, self.user.id, self.course.id)
+
+        self.assertNotIn(self.user.profile.name, result)
+        self.assertIn('%%USER_FULLNAME%%', result)
+
+    def test_subbing_no_userid_or_courseid(self):
+        """
+        Tests that no subbing occurs if no user_id or no course_id is given.
+        """
+        test_string = 'This string should not be subbed here %%USER_ID%%'
+
+        result = Ks.substitute_keywords_with_data(test_string, None, self.course.id)
+        self.assertEqual(test_string, result)
+
+        result = Ks.substitute_keywords_with_data(test_string, self.user.id, None)
+        self.assertEqual(test_string, result)
+
+    def test_subbing_no_user_or_course(self):
+        """
+        Tests that no subbing occurs if no user or no course is given
+        """
+        test_string = "This string should not be subbed here %%USER_ID%%"
+
+        result = Ks.substitute_keywords(test_string, course=self.course, user=None)
+        self.assertEqual(test_string, result)
+
+        result = Ks.substitute_keywords(test_string, self.user, None)
+        self.assertEqual(test_string, result)

lms/djangoapps/bulk_email/models.py

@@ -20,6 +20,7 @@ from html_to_text import html_to_text
 from mail_utils import wrap_message
 
 from xmodule_django.models import CourseKeyField
+from util.keyword_substitution import substitute_keywords_with_data
 
 log = logging.getLogger(__name__)
 
@@ -185,21 +186,21 @@ class CourseEmailTemplate(models.Model):
         using the provided template.  The template is a format string,
         which is rendered using format() with the provided `context` dict.
 
-        This doesn't insert user's text into template, until such time we can
-        support proper error handling due to errors in the message body
-        (e.g. due to the use of curly braces).
-
-        Instead, for now, we insert the message body *after* the substitutions
-        have been performed, so that anything in the message body that might
-        interfere will be innocently returned as-is.
+        Any keywords encoded in the form %%KEYWORD%% found in the message
+        body are subtituted with user data before the body is inserted into
+        the template.
 
         Output is returned as a unicode string.  It is not encoded as utf-8.
         Such encoding is left to the email code, which will use the value
         of settings.DEFAULT_CHARSET to encode the message.
         """
-        # If we wanted to support substitution, we'd call:
-        # format_string = format_string.replace(COURSE_EMAIL_MESSAGE_BODY_TAG, message_body)
+
+        # Substitute all %%-encoded keywords in the message body
+        if 'user_id' in context and 'course_id' in context:
+            message_body = substitute_keywords_with_data(message_body, context['user_id'], context['course_id'])
+
         result = format_string.format(**context)
+
         # Note that the body tag in the template will now have been
         # "formatted", so we need to do the same to the tag being
         # searched for.

lms/djangoapps/bulk_email/tasks.py

@@ -463,6 +463,8 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas
             email = current_recipient['email']
             email_context['email'] = email
             email_context['name'] = current_recipient['profile__name']
+            email_context['user_id'] = current_recipient['pk']
+            email_context['course_id'] = course_email.course_id
 
             # Construct message content using templates and context:
             plaintext_msg = course_email_template.render_plaintext(course_email.text_message, email_context)

lms/startup.py

@@ -2,6 +2,8 @@
 Module for code that should run during LMS startup
 """
 
+# pylint: disable=unused-argument
+
 from django.conf import settings
 
 # Force settings to run so that the python path is modified
@@ -12,6 +14,8 @@ import edxmako
 import logging
 from monkey_patch import django_utils_translation
 import analytics
+from util import keyword_substitution
+
 
 log = logging.getLogger(__name__)
 
@@ -40,6 +44,12 @@ def run():
     if settings.FEATURES.get('SEGMENT_IO_LMS') and hasattr(settings, 'SEGMENT_IO_LMS_KEY'):
         analytics.init(settings.SEGMENT_IO_LMS_KEY, flush_at=50)
 
+    # Monkey patch the keyword function map
+    if keyword_substitution.keyword_function_map_is_empty():
+        keyword_substitution.add_keyword_function_map(get_keyword_function_map())
+        # Once keyword function map is set, make update function do nothing
+        keyword_substitution.add_keyword_function_map = lambda x: None
+
 
 def add_mimetypes():
     """
@@ -139,3 +149,51 @@ def enable_third_party_auth():
 
     from third_party_auth import settings as auth_settings
     auth_settings.apply_settings(settings.THIRD_PARTY_AUTH, settings)
+
+
+def get_keyword_function_map():
+    """
+    Define the mapping of keywords and filtering functions
+
+    The functions are used to filter html, text and email strings
+    before rendering them.
+
+    The generated map will be monkey-patched onto the keyword_substitution
+    module so that it persists along with the running server.
+
+    Each function must take: user & course as parameters
+    """
+
+    from student.models import anonymous_id_for_user
+    from util.date_utils import get_default_time_display
+
+    def user_id_sub(user, course):
+        """
+        Gives the anonymous id for the given user
+
+        For compatibility with the existing anon_ids, return anon_id without course_id
+        """
+        return anonymous_id_for_user(user, None)
+
+    def user_fullname_sub(user, course=None):
+        """ Returns the given user's name """
+        return user.profile.name
+
+    def course_display_name_sub(user, course):
+        """ Returns the course's display name """
+        return course.display_name
+
+    def course_end_date_sub(user, course):
+        """ Returns the course end date in the default display """
+        return get_default_time_display(course.end)
+
+    # Define keyword -> function map
+    # Take care that none of these functions return %% encoded keywords
+    kf_map = {
+        '%%USER_ID%%': user_id_sub,
+        '%%USER_FULLNAME%%': user_fullname_sub,
+        '%%COURSE_DISPLAY_NAME%%': course_display_name_sub,
+        '%%COURSE_END_DATE%%': course_end_date_sub
+    }
+
+    return kf_map

lms/static/coffee/src/instructor_dashboard/send_email.coffee

@@ -13,6 +13,7 @@ PendingInstructorTasks = -> window.InstructorDashboard.util.PendingInstructorTas
 create_task_list_table = -> window.InstructorDashboard.util.create_task_list_table.apply this, arguments
 create_email_content_table = -> window.InstructorDashboard.util.create_email_content_table.apply this, arguments
 create_email_message_views = -> window.InstructorDashboard.util.create_email_message_views.apply this, arguments
+KeywordValidator = -> window.InstructorDashboard.util.KeywordValidator
 
 class SendEmail
   constructor: (@$container) ->
@@ -42,6 +43,14 @@ class SendEmail
         alert gettext("Your message cannot be blank.")
 
       else
+        # Validation for keyword substitution
+        validation = KeywordValidator().validate_string @$emailEditor.save()['data']
+        if not validation.is_valid
+          message = gettext("There are invalid keywords in your email. Please check the following keywords and try again:")
+          message += "\n" + validation.invalid_keywords.join('\n')
+          alert message
+          return
+
         success_message = gettext("Your email was successfully queued for sending.")
         send_to = @$send_to.val().toLowerCase()
         if send_to == "myself"

lms/static/coffee/src/instructor_dashboard/util.coffee

@@ -300,6 +300,31 @@ class PendingInstructorTasks
       error: std_ajax_err => console.error "Error finding pending instructor tasks to display"
     ### /Pending Instructor Tasks Section ####
 
+class KeywordValidator
+
+    @keyword_regex = /%%+[^%]+%%/g
+    @keywords = ['%%USER_ID%%', '%%USER_FULLNAME%%', '%%COURSE_DISPLAY_NAME%%', '%%COURSE_END_DATE%%']
+
+    @validate_string: (string) =>
+      regex_match = string.match(@keyword_regex)
+      found_keywords = if regex_match == null then [] else regex_match
+      invalid_keywords = []
+      is_valid = true
+      keywords = @keywords
+
+      for found_keyword in found_keywords
+        do (found_keyword) ->
+          if found_keyword not in keywords
+            invalid_keywords.push found_keyword
+      
+      if invalid_keywords.length != 0
+        is_valid = false
+      
+      return {
+        is_valid: is_valid,
+        invalid_keywords: invalid_keywords
+      }
+
 # export for use
 # create parent namespaces if they do not already exist.
 # abort if underscore can not be found.
@@ -314,3 +339,4 @@ if _?
     create_email_content_table: create_email_content_table
     create_email_message_views: create_email_message_views
     PendingInstructorTasks: PendingInstructorTasks
+    KeywordValidator: KeywordValidator

lms/tests.py

@@ -9,6 +9,7 @@ from django.core.urlresolvers import reverse
 from edxmako import add_lookup, LOOKUP
 from lms import startup
 from xmodule.modulestore.tests.factories import CourseFactory
+from util import keyword_substitution
 
 
 class LmsModuleTests(TestCase):
@@ -55,3 +56,21 @@ class HelpModalTests(TestCase):
         url = reverse('info', args=[self.course.id.to_deprecated_string()])
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 200)
+
+
+class KeywordSubConfigTests(TestCase):
+    """ Tests for configuring keyword substitution feature """
+
+    def test_keyword_map_not_empty(self):
+        """ Ensure that the keyword subsitution map is non-empty """
+        self.assertFalse(keyword_substitution.keyword_function_map_is_empty())
+
+    def test_adding_keyword_map_is_noop(self):
+        """ Test that trying to add a new keyword mapping is a no-op """
+
+        existing_map = keyword_substitution.KEYWORD_FUNCTION_MAP
+        keyword_substitution.add_keyword_function_map({
+            '%%USER_ID%%': lambda x: x,
+            '%%USER_FULLNAME%%': lambda x: x,
+        })
+        self.assertDictEqual(existing_map, keyword_substitution.KEYWORD_FUNCTION_MAP)