refactor tab modification by components

cms/djangoapps/contentstore/views/course.py

@@ -868,87 +868,70 @@ def _remove_tab(request, tab_type, course_module):
     return False
 
 
-# pylint: disable=invalid-name
-def _config_course_advanced_components(request, course_module):
-    """
-    Check to see if the user instantiated any advanced components. This
-    is a hack that does the following :
-    1) adds/removes the open ended panel tab to a course automatically
-    if the user has indicated that they want to edit the
-    combinedopendended or peergrading module
-    2) adds/removes the notes panel tab to a course automatically if
-    the user has indicated that they want the notes module enabled in
-    their course
-    """
-    # TODO refactor the above into distinct advanced policy settings
-    filter_tabs = True  # Exceptional conditions will pull this to False
-    if ADVANCED_COMPONENT_POLICY_KEY in request.json:  # Maps tab types to components
-        tab_component_map = {
-            'open_ended': OPEN_ENDED_COMPONENT_TYPES,
-            'notes': NOTE_COMPONENT_TYPES,
-        }
-        # Check to see if the user instantiated any notes or open ended components
-        for tab_type in tab_component_map.keys():
-            component_types = tab_component_map.get(tab_type)
-            found_ac_type = False
-            for ac_type in component_types:
-                # Check if the user has incorrectly failed to put the value in an iterable.
-                new_advanced_component_list = request.json[ADVANCED_COMPONENT_POLICY_KEY]['value']
-                if hasattr(new_advanced_component_list, '__iter__'):
-                    if ac_type in new_advanced_component_list and ac_type in ADVANCED_COMPONENT_TYPES:
-                        if _add_tab(request, tab_type, course_module):
-                            # Set this flag to avoid the tab removal code below.
-                            filter_tabs = False
-                        found_ac_type = True  # break
-                else:
-                    # If not iterable, return immediately and let validation handle.
-                    return
+def is_advanced_component_present(request, advanced_components):
+    """
+    Return True when one of `advanced_components` is present in the request.
+
+    raises TypeError
+    when request.ADVANCED_COMPONENT_POLICY_KEY is malformed (not iterable)
+    """
+    if ADVANCED_COMPONENT_POLICY_KEY not in request.json:
+        return False
+
+    new_advanced_component_list = request.json[ADVANCED_COMPONENT_POLICY_KEY]['value']
+    for ac_type in advanced_components:
+        if ac_type in new_advanced_component_list and ac_type in ADVANCED_COMPONENT_TYPES:
+            return True
 
-            # If we did not find a module type in the advanced settings,
-            # we may need to remove the tab from the course.
-            if not found_ac_type:  # Remove tab from the course if needed
-                if _remove_tab(request, tab_type, course_module):
-                    # Indicate that tabs should *not* be filtered out of
-                    # the metadata
-                    filter_tabs = False
 
-    return filter_tabs
+def is_field_value_true(request, field_list):
+    """
+    Return True when one of field values is set to True by request
+    """
+    return any([request.json.get(field, {}).get('value') for field in field_list])
 
 
 # pylint: disable=invalid-name
-def _config_course_settings(request, course_module, filter_tabs=True):
+def _modify_tabs_to_components(request, course_module):
     """
-    Check to see if the user enabled some advanced settings (boolean).
-    This is a hack that does the following :
-    1) adds/removes the edx notes panel tab to a course automatically if
-    the user has indicated that they want the notes module enabled in
-    their course
+    Automatically adds/removes tabs if user indicated that they want
+    respective modules enabled in the course
+
+    Return True when tab configuration has been modified.
     """
     tab_component_map = {
-        'edxnotes': ['edxnotes']
+        # 'tab_type': (check_function, list_of_checked_components_or_values),
+
+        # open ended tab by combinedopendended or peergrading module
+        'open_ended': (is_advanced_component_present, OPEN_ENDED_COMPONENT_TYPES),
+        # notes tab
+        'notes': (is_advanced_component_present, NOTE_COMPONENT_TYPES),
+        # student notes tab
+        'edxnotes': (is_field_value_true, ['edxnotes'])
     }
-    # Check to see if the user instantiated any notes or open ended components
+
+    tabs_changed = False
     for tab_type in tab_component_map.keys():
-        if tab_type in request.json:
-            component_types = tab_component_map.get(tab_type)
-            found_ac_type = False
-            for ac_type in component_types:
-                field_value = request.json[ac_type]['value']
-                if field_value is True:
-                    if _add_tab(request, ac_type, course_module):
-                        # Set this flag to avoid the tab removal code below.
-                        filter_tabs = False
-                    found_ac_type = True  # break
-
-                # If we did not find a module type in the advanced settings,
-                # we may need to remove the tab from the course.
-                if not found_ac_type:  # Remove tab from the course if needed
-                    if _remove_tab(request, ac_type, course_module):
-                        # Indicate that tabs should *not* be filtered out of
-                        # the metadata
-                        filter_tabs = False
-
-    return filter_tabs
+        check, component_types = tab_component_map[tab_type]
+        try:
+            tab_enabled = check(request, component_types)
+        except TypeError:
+            # user has failed to put iterable value into advanced component list.
+            # return immediately and let validation handle.
+            return
+
+        if tab_enabled:
+            # check passed, some of this component_types are present, adding tab
+            if _add_tab(request, tab_type, course_module):
+                # tab indeed was added, the change needs to propagate
+                tabs_changed = True
+        else:
+            # the tab should not be present (anymore)
+            if _remove_tab(request, tab_type, course_module):
+                # tab indeed was removed, the change needs to propagate
+                tabs_changed = True
+
+    return tabs_changed
 
 
 @login_required
@@ -980,9 +963,8 @@ def advanced_settings_handler(request, course_key_string):
                 return JsonResponse(CourseMetadata.fetch(course_module))
             else:
                 try:
-                    # Whether or not to filter the tabs key out of the settings metadata
-                    filter_tabs = _config_course_advanced_components(request, course_module)
-                    filter_tabs = _config_course_settings(request, course_module, filter_tabs)
+                    # do not process tabs unless they were modified according to course metadata
+                    filter_tabs = not _modify_tabs_to_components(request, course_module)
 
                     # validate data formats and update
                     is_valid, errors, updated_data = CourseMetadata.validate_and_update_from_json(