From d779466f09500a19f7c67fa409a10dfcd98418bd Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Fri, 19 Jun 2015 10:11:14 -0400 Subject: [PATCH] Make course tab refreshing safer. --- .../tests/test_course_settings.py | 51 ++++++++++++------- cms/djangoapps/contentstore/views/course.py | 26 ++++++++-- lms/djangoapps/course_wiki/tab.py | 1 + lms/djangoapps/courseware/tabs.py | 6 +++ lms/djangoapps/courseware/tests/test_tabs.py | 1 + .../django_comment_client/forum/views.py | 1 + 6 files changed, 65 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 2ecb83f802..31afe010d5 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/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) + self.assertNotIn(self.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_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) - def test_advanced_components_munge_tabs_hidden_tabs(self): - updated_tabs = self.mark_wiki_as_hidden(self.course.tabs) - self.course.tabs = updated_tabs + 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): diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 10ef8f2167..becfcfffdf 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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: - # update the course tabs if required by any setting changes - _refresh_course_tabs(request, course_module) + 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" diff --git a/lms/djangoapps/course_wiki/tab.py b/lms/djangoapps/course_wiki/tab.py index eecc082dcb..cf681eaa4a 100644 --- a/lms/djangoapps/course_wiki/tab.py +++ b/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): diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index 94b4b25d88..cc2d6916d8 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/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): diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index 9e52a73c72..b50d4d793a 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/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 diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 9c9d362f37..8470cb0b36 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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):