diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index b8e91bbffc..809d5cc803 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -269,6 +269,53 @@ def get_available_providers(): return available_providers +class DiscussionTopicMapping(Dict): + """ + DiscussionTopicMapping field, which includes validation of the topic id + """ + + _default = {} + + def from_json(self, value): + """ + Return python dict type. Performs validation on id key for + each discussion topic. Whenever Discussion Topic Mapping field is saved or + uploaded from data, this function is called to validate input mapping + """ + if value is None: + return None + """ + if statement checks to make sure value is a dict, + each topic in dict has a key id and the id value only has valid characters + """ + if isinstance(value, dict): + for key in value: + if "id" in value[key].keys(): + if not self._validate_topic_id_value(value[key]["id"]): + raise ValueError("For discussion topic ids, the only characters that are supported are the English alphabet(a-z, A-Z), integers(0-9), underscore, hyphen, and period.") + else: + raise ValueError("All discussion topics must have an ID value set.") + else: + raise TypeError('Invalid JSON format found in Discussion Topic Mapping.' % type(value)) + return value + enforce_type = from_json + + def _validate_topic_id_value(self, value): + """ + Function validates discussion topic ids. Will return true if + only valid characters(a-z,A-Z,0-9,underscore(_), hyphen(-), and period(.)) + are used in id body + inputs: value(str) + """ + for character in value: + if not (character.isalnum() + or character == "-" + or character == "_" + or character == "."): + return False + return True + + class CourseFields(object): lti_passports = List( display_name=_("LTI Passports"), @@ -360,7 +407,7 @@ class CourseFields(object): ), scope=Scope.settings ) - discussion_topics = Dict( + discussion_topics = DiscussionTopicMapping( display_name=_("Discussion Topic Mapping"), help=_( 'Enter discussion categories in the following format: "CategoryName": ' diff --git a/common/test/acceptance/tests/studio/test_studio_settings.py b/common/test/acceptance/tests/studio/test_studio_settings.py index 7c71ae66a5..dac71fcf34 100644 --- a/common/test/acceptance/tests/studio/test_studio_settings.py +++ b/common/test/acceptance/tests/studio/test_studio_settings.py @@ -454,25 +454,6 @@ class AdvancedSettingsValidationTest(StudioCourseTest): "Button text should change to 'Show Deprecated Settings' after the click" ) - def test_multi_line_input(self): - """ - Scenario: Test that advanced settings correctly shows the multi-line input - Given I am on the Advanced Course Settings page in Studio - When I create a JSON object as a value for "Discussion Topic Mapping" - Then it is displayed as formatted - """ - - inputs = { - "key": "value", - "key_2": "value_2" - } - json_input = json.dumps(inputs) - self.advanced_settings.set('Discussion Topic Mapping', json_input) - self.assertEqual( - self.advanced_settings.get('Discussion Topic Mapping'), - '{\n "key": "value",\n "key_2": "value_2"\n}' - ) - def test_automatic_quoting_of_non_json_value(self): """ Scenario: Test that advanced settings automatically quotes the field input diff --git a/lms/djangoapps/discussion/django_comment_client/tests/utils.py b/lms/djangoapps/discussion/django_comment_client/tests/utils.py index 1abde68566..1b0c65e10d 100644 --- a/lms/djangoapps/discussion/django_comment_client/tests/utils.py +++ b/lms/djangoapps/discussion/django_comment_client/tests/utils.py @@ -120,7 +120,19 @@ def topic_name_to_id(course, name): """ Given a discussion topic name, return an id for that name (includes course and url_name). + The set of valid characters for topic id is a smaller subset of valid + characters for topic name. Topic id can only have + these characters: a-z, A-Z, 0-9, underscore, hyphen, and period + Whereas, topic name character choice is much larger. + This function goes through all the characters in name and removes all invalid + characters and returns the resulting topic id. """ + for index, indexed_character in enumerate(name): + if not (indexed_character.isalnum() + or indexed_character == "-" + or indexed_character == "_" + or indexed_character == "."): + name = name[:index] + "_" + name[index + 1:] return "{course}_{run}_{name}".format( course=course.location.course, run=course.url_name, diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 6c927b14fd..c753076be9 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -1928,7 +1928,7 @@ class CourseDiscussionSettingsAPIViewTest(APITestCase, UrlResetMixin, ModuleStor start=datetime.now() ) discussion_topics = { - "Topic B": {"id": "Topic B"}, + "Topic B": {"id": "Topic_B"}, } config_course_cohorts(self.course, is_cohorted=True) config_course_discussions( @@ -2073,7 +2073,7 @@ class CourseDiscussionSettingsAPIViewTest(APITestCase, UrlResetMixin, ModuleStor def test_update_course_wide_discussion_settings(self): """Test whether the 'divided_course_wide_discussions' setting is updated.""" discussion_topics = { - 'Topic B': {'id': 'Topic B'} + 'Topic B': {'id': 'Topic_B'} } config_course_cohorts(self.course, is_cohorted=True) config_course_discussions(self.course, discussion_topics=discussion_topics)