Implements keyword sub feature for bulk emails

This commit pulls in changes from #4487 that implements keyword
substitution for bulk emails. With these changes, an instructor can
include keywords in their bulk emails which will be automatically substituted
with the corresponding value for the recepient of the email. Keywords are
of the form %%keyword%%, and the keywords implemented in this commit include:

%%USER_ID%% => anonymous_user_id
%%USER_FULLNAME%% => user profile name
%%COURSE_DISPLAY_NAME%% => display name of the course
%%COURSE_END_DATE%% => end date of the course

Client-side validations have also been implemented to ensure that only emails
with well-formed keywords can be sent.
The architecture is designed such that adding in new keywords in the future
would be relatively straight-forward.

common/djangoapps/util/keyword_substitution.py

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

+{
+    "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

+{
+    "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

+{
+    "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

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