From 8584aae8666d20ef50ce3cdea5b71fed89c6ff97 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 23 Jul 2013 15:30:32 -0400 Subject: [PATCH 1/2] It seems like self.tabs is getting set to an empty set [] now rather than None. This causes the tab defaulting to not populate, leading to crashes in the LMS if someone makes a new course with additional tabs (e.g. static tabs or pdf-textbooks) --- common/lib/xmodule/xmodule/course_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index d597221dec..1f74b6c7a9 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -410,7 +410,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): continue # TODO check that this is still needed here and can't be by defaults. - if self.tabs is None: + if not self.tabs or (isinstance(self.tabs, list) and len(self.tabs) == 0): # When calling the various _tab methods, can omit the 'type':'blah' from the # first arg, since that's only used for dispatch tabs = [] From 0b6932f4b879c20ea8caf87eaf460d900d664da9 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 23 Jul 2013 16:15:39 -0400 Subject: [PATCH 2/2] add new test to assert that course creation will populate default tabs as expected. Also update factory to not override defaults on tabs array. Also simplfy self.tab test condition. --- .../contentstore/tests/test_contentstore.py | 17 +++++++++++++++++ common/lib/xmodule/xmodule/course_module.py | 2 +- .../xmodule/modulestore/tests/factories.py | 10 ---------- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index ce7e886220..a51110163d 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -312,6 +312,23 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertGreater(len(course.textbooks), 0) + def test_default_tabs_on_create_course(self): + module_store = modulestore('direct') + CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') + course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None]) + + course = module_store.get_item(course_location) + + expected_tabs = [] + expected_tabs.append({u'type': u'courseware'}) + expected_tabs.append({u'type': u'course_info', u'name': u'Course Info'}) + expected_tabs.append({u'type': u'textbooks'}) + expected_tabs.append({u'type': u'discussion', u'name': u'Discussion'}) + expected_tabs.append({u'type': u'wiki', u'name': u'Wiki'}) + expected_tabs.append({u'type': u'progress', u'name': u'Progress'}) + + self.assertEqual(course.tabs, expected_tabs) + def test_static_tab_reordering(self): module_store = modulestore('direct') CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 1f74b6c7a9..3c9f5c0683 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -410,7 +410,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): continue # TODO check that this is still needed here and can't be by defaults. - if not self.tabs or (isinstance(self.tabs, list) and len(self.tabs) == 0): + if not self.tabs: # When calling the various _tab methods, can omit the 'type':'blah' from the # first arg, since that's only used for dispatch tabs = [] diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 8c3b5d59dd..f2e4017114 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -38,16 +38,6 @@ class XModuleCourseFactory(Factory): new_course.display_name = display_name new_course.lms.start = datetime.datetime.now(UTC).replace(microsecond=0) - new_course.tabs = kwargs.pop( - 'tabs', - [ - {"type": "courseware"}, - {"type": "course_info", "name": "Course Info"}, - {"type": "discussion", "name": "Discussion"}, - {"type": "wiki", "name": "Wiki"}, - {"type": "progress", "name": "Progress"} - ] - ) # The rest of kwargs become attributes on the course: for k, v in kwargs.iteritems():