Merge pull request #8583 from edx/diana/course-tab-cleanup

Make course tab refreshing safer.

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -19,6 +19,7 @@ from xmodule.modulestore.tests.factories import CourseFactory
 
 from models.settings.course_metadata import CourseMetadata
 from xmodule.fields import Date
+from xmodule.tabs import InvalidTabsException
 
 from .utils import CourseTestCase
 from xmodule.modulestore.django import modulestore
@@ -617,6 +618,7 @@ class CourseGradingTest(CourseTestCase):
         self.assertEqual(json.loads(response.content).get('graderType'), u'notgraded')
 
 
+@ddt.ddt
 class CourseMetadataEditingTest(CourseTestCase):
     """
     Tests for CourseMetadata.
@@ -626,6 +628,7 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.fullcourse = CourseFactory.create()
         self.course_setting_url = get_url(self.course.id, 'advanced_settings_handler')
         self.fullcourse_setting_url = get_url(self.fullcourse.id, 'advanced_settings_handler')
+        self.notes_tab = {"type": "notes", "name": "My Notes"}
 
     def test_fetch_initial_fields(self):
         test_model = CourseMetadata.fetch(self.course)
@@ -930,12 +933,11 @@ class CourseMetadataEditingTest(CourseTestCase):
         """
         open_ended_tab = {"type": "open_ended", "name": "Open Ended Panel"}
         peer_grading_tab = {"type": "peer_grading", "name": "Peer grading"}
-        notes_tab = {"type": "notes", "name": "My Notes"}
 
         # First ensure that none of the tabs are visible
         self.assertNotIn(open_ended_tab, self.course.tabs)
         self.assertNotIn(peer_grading_tab, self.course.tabs)
-        self.assertNotIn(notes_tab, self.course.tabs)
+        self.assertNotIn(self.notes_tab, self.course.tabs)
 
         # Now add the "combinedopenended" component and verify that the tab has been added
         self.client.ajax_post(self.course_setting_url, {
@@ -944,7 +946,7 @@ class CourseMetadataEditingTest(CourseTestCase):
         course = modulestore().get_course(self.course.id)
         self.assertIn(open_ended_tab, course.tabs)
         self.assertIn(peer_grading_tab, course.tabs)
-        self.assertNotIn(notes_tab, course.tabs)
+        self.assertNotIn(self.notes_tab, course.tabs)
 
         # Now enable student notes and verify that the "My Notes" tab has also been added
         self.client.ajax_post(self.course_setting_url, {
@@ -953,7 +955,7 @@ class CourseMetadataEditingTest(CourseTestCase):
         course = modulestore().get_course(self.course.id)
         self.assertIn(open_ended_tab, course.tabs)
         self.assertIn(peer_grading_tab, course.tabs)
-        self.assertIn(notes_tab, course.tabs)
+        self.assertIn(self.notes_tab, course.tabs)
 
         # Now remove the "combinedopenended" component and verify that the tab is gone
         self.client.ajax_post(self.course_setting_url, {
@@ -962,7 +964,7 @@ class CourseMetadataEditingTest(CourseTestCase):
         course = modulestore().get_course(self.course.id)
         self.assertNotIn(open_ended_tab, course.tabs)
         self.assertNotIn(peer_grading_tab, course.tabs)
-        self.assertIn(notes_tab, course.tabs)
+        self.assertIn(self.notes_tab, course.tabs)
 
         # Finally disable student notes and verify that the "My Notes" tab is gone
         self.client.ajax_post(self.course_setting_url, {
@@ -971,25 +973,40 @@ class CourseMetadataEditingTest(CourseTestCase):
         course = modulestore().get_course(self.course.id)
         self.assertNotIn(open_ended_tab, course.tabs)
         self.assertNotIn(peer_grading_tab, course.tabs)
-        self.assertNotIn(notes_tab, course.tabs)
-
-    def mark_wiki_as_hidden(self, tabs):
-        """ Mark the wiki tab as hidden. """
-        for tab in tabs:
-            if tab.type == 'wiki':
-                tab['is_hidden'] = True
-        return tabs
-
-    def test_advanced_components_munge_tabs_hidden_tabs(self):
-        updated_tabs = self.mark_wiki_as_hidden(self.course.tabs)
-        self.course.tabs = updated_tabs
+        self.assertNotIn(self.notes_tab, course.tabs)
+
+    def test_advanced_components_munge_tabs_validation_failure(self):
+        with patch('contentstore.views.course._refresh_course_tabs', side_effect=InvalidTabsException):
+            resp = self.client.ajax_post(self.course_setting_url, {
+                ADVANCED_COMPONENT_POLICY_KEY: {"value": ["notes"]}
+            })
+            self.assertEqual(resp.status_code, 400)
+
+            error_msg = [
+                {
+                    'message': 'An error occurred while trying to save your tabs',
+                    'model': {'display_name': 'Tabs Exception'}
+                }
+            ]
+            self.assertEqual(json.loads(resp.content), error_msg)
+
+            # verify that the course wasn't saved into the modulestore
+            course = modulestore().get_course(self.course.id)
+            self.assertNotIn("notes", course.advanced_modules)
+
+    @ddt.data(
+        [{'type': 'courseware'}, {'type': 'course_info'}, {'type': 'wiki', 'is_hidden': True}],
+        [{'type': 'courseware', 'name': 'Courses'}, {'type': 'course_info', 'name': 'Info'}],
+    )
+    def test_course_tab_configurations(self, tab_list):
+        self.course.tabs = tab_list
         modulestore().update_item(self.course, self.user.id)
         self.client.ajax_post(self.course_setting_url, {
             ADVANCED_COMPONENT_POLICY_KEY: {"value": ["notes"]}
         })
         course = modulestore().get_course(self.course.id)
-        notes_tab = {"type": "notes", "name": "My Notes"}
-        self.assertIn(notes_tab, course.tabs)
+        tab_list.append(self.notes_tab)
+        self.assertEqual(tab_list, course.tabs)
 
 
 class CourseGraderUpdatesTest(CourseTestCase):

cms/djangoapps/contentstore/views/course.py

@@ -5,6 +5,7 @@ import copy
 from django.shortcuts import redirect
 import json
 import random
+import logging
 import string  # pylint: disable=deprecated-module
 from django.utils.translation import ugettext as _
 import django.utils
@@ -22,7 +23,7 @@ from xmodule.course_module import DEFAULT_START_DATE
 from xmodule.error_module import ErrorDescriptor
 from xmodule.modulestore.django import modulestore
 from xmodule.contentstore.content import StaticContent
-from xmodule.tabs import CourseTab
+from xmodule.tabs import CourseTab, CourseTabList, InvalidTabsException
 from openedx.core.lib.course_tabs import CourseTabPluginManager
 from openedx.core.djangoapps.credit.api import is_credit_course, get_credit_requirements
 from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements
@@ -87,6 +88,8 @@ from util.milestones_helpers import (
     is_valid_course_key
 )
 
+log = logging.getLogger(__name__)
+
 __all__ = ['course_info_handler', 'course_handler', 'course_listing',
            'course_info_update_handler', 'course_search_index_handler',
            'course_rerun_handler',
@@ -1024,6 +1027,9 @@ def grading_handler(request, course_key_string, grader_index=None):
 def _refresh_course_tabs(request, course_module):
     """
     Automatically adds/removes tabs if changes to the course require them.
+
+    Raises:
+        InvalidTabsException: raised if there's a problem with the new version of the tabs.
     """
 
     def update_tab(tabs, tab_type, tab_enabled):
@@ -1047,6 +1053,8 @@ def _refresh_course_tabs(request, course_module):
             tab_enabled = tab_type.is_enabled(course_module, user=request.user)
             update_tab(course_tabs, tab_type, tab_enabled)
 
+    CourseTabList.validate_tabs(course_tabs)
+
     # Save the tabs into the course if they have been changed
     if course_tabs != course_module.tabs:
         course_module.tabs = course_tabs
@@ -1090,8 +1098,18 @@ def advanced_settings_handler(request, course_key_string):
                     )
 
                     if is_valid:
+                        try:
                             # update the course tabs if required by any setting changes
                             _refresh_course_tabs(request, course_module)
+                        except InvalidTabsException as err:
+                            log.exception(err.message)
+                            response_message = [
+                                {
+                                    'message': _('An error occurred while trying to save your tabs'),
+                                    'model': {'display_name': _('Tabs Exception')}
+                                }
+                            ]
+                            return JsonResponseBadRequest(response_message)
 
                         # now update mongo
                         modulestore().update_item(course_module, request.user.id)
@@ -1101,7 +1119,7 @@ def advanced_settings_handler(request, course_key_string):
                         return JsonResponseBadRequest(errors)
 
                 # Handle all errors that validation doesn't catch
-                except (TypeError, ValueError) as err:
+                except (TypeError, ValueError, InvalidTabsException) as err:
                     return HttpResponseBadRequest(
                         django.utils.html.escape(err.message),
                         content_type="text/plain"

lms/djangoapps/course_wiki/tab.py

@@ -18,6 +18,7 @@ class WikiTab(EnrolledTab):
     title = _('Wiki')
     view_name = "course_wiki"
     is_hideable = True
+    is_default = False
 
     @classmethod
     def is_enabled(cls, course, user=None):

lms/djangoapps/courseware/tabs.py

@@ -32,6 +32,7 @@ class CoursewareTab(EnrolledTab):
     priority = 10
     view_name = 'courseware'
     is_movable = False
+    is_default = False
 
 
 class CourseInfoTab(CourseTab):
@@ -44,6 +45,7 @@ class CourseInfoTab(CourseTab):
     view_name = 'info'
     tab_id = 'info'
     is_movable = False
+    is_default = False
 
     @classmethod
     def is_enabled(cls, course, user=None):
@@ -59,6 +61,7 @@ class SyllabusTab(EnrolledTab):
     priority = 30
     view_name = 'syllabus'
     allow_multiple = True
+    is_default = False
 
     @classmethod
     def is_enabled(cls, course, user=None):  # pylint: disable=unused-argument
@@ -76,6 +79,7 @@ class ProgressTab(EnrolledTab):
     priority = 40
     view_name = 'progress'
     is_hideable = True
+    is_default = False
 
     @classmethod
     def is_enabled(cls, course, user=None):  # pylint: disable=unused-argument
@@ -91,6 +95,7 @@ class TextbookTabsBase(CourseTab):
     # Translators: 'Textbooks' refers to the tab in the course that leads to the course' textbooks
     title = _("Textbooks")
     is_collection = True
+    is_default = False
 
     @classmethod
     def is_enabled(cls, course, user=None):  # pylint: disable=unused-argument
@@ -222,6 +227,7 @@ class ExternalDiscussionCourseTab(LinkTab):
     # Translators: 'Discussion' refers to the tab in the courseware that leads to the discussion forums
     title = _('Discussion')
     priority = None
+    is_default = False
 
     @classmethod
     def validate(cls, tab_dict, raise_error=True):

lms/djangoapps/courseware/tests/test_tabs.py

@@ -480,6 +480,7 @@ class TabListTestCase(TabTestCase):
             [{'type': CoursewareTab.type}, {'type': 'discussion', 'name': 'fake_name'}],
             # incorrect order
             [{'type': CourseInfoTab.type, 'name': 'fake_name'}, {'type': CoursewareTab.type}],
+            [{'type': 'unknown_type'}]
         ]
 
         # tab types that should appear only once

lms/djangoapps/django_comment_client/forum/views.py

@@ -59,6 +59,7 @@ class DiscussionTab(EnrolledTab):
     priority = None
     view_name = 'django_comment_client.forum.views.forum_form_discussion'
     is_hideable = settings.FEATURES.get('ALLOW_HIDING_DISCUSSION_TAB', False)
+    is_default = False
 
     @classmethod
     def is_enabled(cls, course, user=None):