use email context for keyword substitution, cutting down on db queries (TNL-1591)

common/djangoapps/util/keyword_substitution.py

@@ -21,26 +21,19 @@ Usage:
 """
 
 from django.contrib.auth.models import User
+from student.models import anonymous_id_for_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):
+def anonymous_id_from_user_id(user_id):
     """
-    Attaches the given keyword-function mapping to the existing one
+    Gets a user's anonymous id from their user id
     """
-    KEYWORD_FUNCTION_MAP.update(mapping)
+    user = User.objects.get(id=user_id)
+    return anonymous_id_for_user(user, None)
 
 
-def substitute_keywords(string, user=None, course=None):
+def substitute_keywords(string, user_id, context):
     """
     Replaces all %%-encoded words using KEYWORD_FUNCTION_MAP mapping functions
 
@@ -48,35 +41,37 @@ def substitute_keywords(string, user=None, course=None):
     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():
+    # do this lazily to avoid unneeded database hits
+    KEYWORD_FUNCTION_MAP = {
+        '%%USER_ID%%': lambda: anonymous_id_from_user_id(user_id),
+        '%%USER_FULLNAME%%': lambda: context.get('name'),
+        '%%COURSE_DISPLAY_NAME%%': lambda: context.get('course_title'),
+        '%%COURSE_END_DATE%%': lambda: context.get('course_end_date'),
+    }
+
+    for key in KEYWORD_FUNCTION_MAP.keys():
         if key in string:
-            substitutor = func(user, course)
-            string = string.replace(key, substitutor)
+            substitutor = KEYWORD_FUNCTION_MAP[key]
+            string = string.replace(key, substitutor())
 
     return string
 
 
-def substitute_keywords_with_data(string, user_id=None, course_id=None):
+def substitute_keywords_with_data(string, context):
     """
-    Given user and course ids, replaces all %%-encoded words in the given string
+    Given an email context, replaces all %%-encoded words in the given string
+    `context` is a dictionary that should include `user_id` and `course_title`
+    keys
     """
 
     # 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
+    user_id = context.get('user_id')
+    course_title = context.get('course_title')
 
-    # Grab user objects
-    user = User.objects.get(id=user_id)
-    course = modulestore().get_course(course_id, depth=0)
+    if user_id is None or course_title is None:
+        return string
 
-    return substitute_keywords(string, user, course)
+    return substitute_keywords(string, user_id, context)

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_keywordsub_multiple_tags.json

@@ -5,6 +5,6 @@
     },
     "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"
+        "expected": "The user with id %%user-id%% is named Test User and is in %%COURSE_DISPLAY_NAME"
     }
 }

common/djangoapps/util/tests/test_keyword_sub_utils.py

@@ -30,54 +30,45 @@ class KeywordSubTest(ModuleStoreTestCase):
             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,
+        self.context = {
+            'user_id': self.user.id,
+            'course_title': self.course.display_name,
+            'name': self.user.profile.name,
+            'course_end_date': get_default_time_display(self.course.end),
         }
 
     @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)
+        result = Ks.substitute_keywords_with_data(
+            test_info['test_string'], self.context,
+        )
 
         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_anonymous_id_sub(self):
+        """
+        Test that anonymous_id is subbed
+        """
+        test_string = "Turn %%USER_ID%% into anonymous id"
+        anonymous_id = Ks.anonymous_id_from_user_id(self.user.id)
+        result = Ks.substitute_keywords_with_data(
+            test_string, self.context,
+        )
+        self.assertNotIn('%%USER_ID%%', result)
+        self.assertIn(anonymous_id, result)
 
     def test_name_sub(self):
+        """
+        Test that the user's full name is correctly subbed
+        """
         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)
+        result = Ks.substitute_keywords_with_data(
+            test_string, self.context,
+        )
 
         self.assertNotIn('%%USER_FULLNAME%%', result)
         self.assertIn(user_name, result)
@@ -87,7 +78,9 @@ class KeywordSubTest(ModuleStoreTestCase):
         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)
+        result = Ks.substitute_keywords_with_data(
+            test_string, self.context,
+        )
 
         self.assertEquals(test_string, result)
 
@@ -96,58 +89,37 @@ class KeywordSubTest(ModuleStoreTestCase):
         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)
+        result = Ks.substitute_keywords_with_data(
+            test_string, self.context,
+        )
 
         self.assertEquals(test_string, result)
 
     @file_data('fixtures/test_keywordsub_multiple_tags.json')
-    def test_sub_mutiltple_tags(self, test_info):
+    def test_sub_multiple_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)
+        with patch('util.keyword_substitution.anonymous_id_from_user_id', lambda user_id: anon_id):
+            result = Ks.substitute_keywords_with_data(
+                test_info['test_string'], self.context,
+            )
             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)
+        no_course_context = dict(
+            (key, value) for key, value in self.context.iteritems() if key != 'course_title'
+        )
+        result = Ks.substitute_keywords_with_data(test_string, no_course_context)
         self.assertEqual(test_string, result)
 
-        result = Ks.substitute_keywords(test_string, self.user, None)
+        no_user_id_context = dict(
+            (key, value) for key, value in self.context.iteritems() if key != 'user_id'
+        )
+        result = Ks.substitute_keywords_with_data(test_string, no_user_id_context)
         self.assertEqual(test_string, result)

lms/djangoapps/bulk_email/models.py

@@ -197,7 +197,7 @@ class CourseEmailTemplate(models.Model):
 
         # 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'])
+            message_body = substitute_keywords_with_data(message_body, context)
 
         result = format_string.format(**context)
 

lms/djangoapps/bulk_email/tasks.py

@@ -48,6 +48,7 @@ from instructor_task.subtasks import (
     update_subtask_status,
 )
 from util.query import use_read_replica_if_available
+from util.date_utils import get_default_time_display
 
 log = logging.getLogger('edx.celery.task')
 
@@ -146,6 +147,7 @@ def _get_course_email_context(course):
     """
     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,
         reverse('course_root', kwargs={'course_id': course_id})
@@ -155,6 +157,7 @@ def _get_course_email_context(course):
         '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('dashboard')),
         'platform_name': settings.PLATFORM_NAME,
     }

lms/startup.py

@@ -14,7 +14,6 @@ import edxmako
 import logging
 from monkey_patch import django_utils_translation
 import analytics
-from util import keyword_substitution
 
 
 log = logging.getLogger(__name__)
@@ -44,12 +43,6 @@ 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():
     """
@@ -149,51 +142,3 @@ 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/tests.py

@@ -58,21 +58,3 @@ class HelpModalTests(ModuleStoreTestCase):
         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)