diff --git a/common/djangoapps/util/keyword_substitution.py b/common/djangoapps/util/keyword_substitution.py index 4f206890c7..f9a9aa34e0 100644 --- a/common/djangoapps/util/keyword_substitution.py +++ b/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(): +def anonymous_id_from_user_id(user_id): """ - Checks if the keyword function map has been filled + Gets a user's anonymous id from their user id """ - return not bool(KEYWORD_FUNCTION_MAP) + user = User.objects.get(id=user_id) + return anonymous_id_for_user(user, None) -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): +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: + user_id = context.get('user_id') + course_title = context.get('course_title') + + if user_id is None or course_title 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) + return substitute_keywords(string, user_id, context) diff --git a/common/djangoapps/util/tests/fixtures/test_keyword_anonid_sub.json b/common/djangoapps/util/tests/fixtures/test_keyword_anonid_sub.json deleted file mode 100644 index 303a028104..0000000000 --- a/common/djangoapps/util/tests/fixtures/test_keyword_anonid_sub.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "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 %%USER_ID%%", - "expected": "There is html in this guy 123456789" - } -} diff --git a/common/djangoapps/util/tests/fixtures/test_keywordsub_multiple_tags.json b/common/djangoapps/util/tests/fixtures/test_keywordsub_multiple_tags.json index dc2f1b0353..2105b9a45a 100644 --- a/common/djangoapps/util/tests/fixtures/test_keywordsub_multiple_tags.json +++ 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" } } diff --git a/common/djangoapps/util/tests/test_keyword_sub_utils.py b/common/djangoapps/util/tests/test_keyword_sub_utils.py index 2cb20aa96f..e9d2381f5c 100644 --- a/common/djangoapps/util/tests/test_keyword_sub_utils.py +++ b/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_string = "This is the test string. subthis: %%USER_FULLNAME%% into user name" + """ + 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) + 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_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) + 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) diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index d315db6d15..0842f93958 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/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) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 8c53c561d0..fda5b33ae5 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/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, } diff --git a/lms/startup.py b/lms/startup.py index 6d95d629dc..3add027f15 100644 --- a/lms/startup.py +++ b/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 diff --git a/lms/tests.py b/lms/tests.py index 36c0aa05ae..09946a9fbb 100644 --- a/lms/tests.py +++ b/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)