diff --git a/common/djangoapps/util/keyword_substitution.py b/common/djangoapps/util/keyword_substitution.py new file mode 100644 index 0000000000..4f206890c7 --- /dev/null +++ b/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) diff --git a/common/djangoapps/util/tests/fixtures/test_keyword_anonid_sub.json b/common/djangoapps/util/tests/fixtures/test_keyword_anonid_sub.json new file mode 100644 index 0000000000..303a028104 --- /dev/null +++ b/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 %%USER_ID%%", + "expected": "There is html in this guy 123456789" + } +} diff --git a/common/djangoapps/util/tests/fixtures/test_keyword_coursename_sub.json b/common/djangoapps/util/tests/fixtures/test_keyword_coursename_sub.json new file mode 100644 index 0000000000..59c127820f --- /dev/null +++ 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:

%%COURSE_DISPLAY_NAME%%

", + "expected": "This string has some html:

test_course

" + } +} diff --git a/common/djangoapps/util/tests/fixtures/test_keywordsub_multiple_tags.json b/common/djangoapps/util/tests/fixtures/test_keywordsub_multiple_tags.json new file mode 100644 index 0000000000..dc2f1b0353 --- /dev/null +++ b/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" + } +} diff --git a/common/djangoapps/util/tests/test_keyword_sub_utils.py b/common/djangoapps/util/tests/test_keyword_sub_utils.py new file mode 100644 index 0000000000..69940f895c --- /dev/null +++ b/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) diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 045edd99bb..df59c8fc1d 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/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. diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index c3b17339c3..22101b46c5 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/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) diff --git a/lms/startup.py b/lms/startup.py index 1e02c56144..3ac34d07dd 100644 --- a/lms/startup.py +++ b/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 diff --git a/lms/static/coffee/src/instructor_dashboard/send_email.coffee b/lms/static/coffee/src/instructor_dashboard/send_email.coffee index d26ed4a287..9851658b5a 100644 --- a/lms/static/coffee/src/instructor_dashboard/send_email.coffee +++ b/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" diff --git a/lms/static/coffee/src/instructor_dashboard/util.coffee b/lms/static/coffee/src/instructor_dashboard/util.coffee index 8957af6e78..a6f4bfd14e 100644 --- a/lms/static/coffee/src/instructor_dashboard/util.coffee +++ b/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 diff --git a/lms/tests.py b/lms/tests.py index 447523b707..020fdbb93f 100644 --- a/lms/tests.py +++ b/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)